upgrades: remove upgrade granting CREATELOGIN role opt#92597
Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom Nov 28, 2022
Merged
upgrades: remove upgrade granting CREATELOGIN role opt#92597craig[bot] merged 1 commit intocockroachdb:masterfrom
craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
This patch removes a permanent upgrade that was granting the CREATELOGIN role option to users that had the CREATEROLE option. This upgrade was introduced in 20.2, meant to grant the then-new CREATELOGIN option to users created in 20.1 and prior that had the CREATEROLE option (CREATELOGIN was being split from CREATEROLE). This code stayed around as a startupmigration since it was introduced, even though it didn't make sense for it to stay around after 20.2. Technically, I think this startup migration should have had the flag `IncludedInBootstrap=v20.2`, since we don't want it to run for clusters created at or after 20.2; this migration is not idempotent in the general sense, since it potentially picks up new, unintended users when it runs. Since 22.2, this migration would fail to run on anything but an empty system.role_options table because it would attempt to put NULLs into a non-nullable column. This was all benign since the startupmigrations had protection in place, preventing them for running a 2nd time after a completed attempt. So, for upgraded cluster, the migration only when the first 20.2 node came up, and for new clusters it would be a no-op since the system.role_options table starts empty. This migration became problematic in cockroachdb#91627 when I've turned the startupmigrations into upgrades. These upgrades run once when upgrading to 23.1; I'm relying on their idempotence. Fixes cockroachdb#92230 Fixes cockroachdb#92371 Fixes cockroachdb#92569 Release note: None
Contributor
Author
|
bors r+ |
Member
|
This change is |
Contributor
|
Build failed (retrying...): |
Contributor
|
Build succeeded: |
andreimatei
added a commit
to andreimatei/cockroach
that referenced
this pull request
Dec 8, 2022
This patch makes the "primordial" permanent upgrades not run in clusters being upgraded from 22.2 or lower. Startup migrations were recently replaced with "permanent upgrades", in cockroachdb#91627. The permanent new upgrades were running on node startup regardless of whether it was a "new cluster" or an existing one - i.e. regardless of whether the previous startupmigrations had also run, on the argument that startupmigrations and upgrades are supposed to be idempotent anyway. But, alas, there's idempotency and then there's idempotency: startupmigrations/upgrades are supposed to be OK with running multiple times at the same BinaryVersion in a "new cluster", but not necessarily across BinaryVersions and in a cluster that has been in use. There are different things that can go wrong when one of these migrations runs multiple times in these general conditions, because the migration might makes implicit assumptions about the schema and data of the cluster it's running in: 1. A migration might assume that all the data in a system table was created before a CRDB version changed some semantics. For example, the migration deleted in cockroachdb#92597 was assuming that `CREATEROLE` role option had a certain meaning for all the users who have it and was doing a backfill across all these users. Running this migration again at an arbitrary later point would affect unintended new users. 2. A migration assumes the schema of system tables it operates on. As this schema (which is part of the bootstrap image for new cluster) changes with new versions, the code of the upgrade changes also. This means that the upgrade is no longer suitable for running against clusters that don't have the upgraded schema - i.e. clusters that have not yet been upgraded to the latest schema. This is a real problem because we're adding a column to system.users, and the addRootUser upgrade can no longer run against 22.2 clusters. This patch guards all upgrades on checking that the old corresponding startupmigration has not run, thus enabling the mentioned system schema change in 23.1. Release note: None Epic: None
andreimatei
added a commit
to andreimatei/cockroach
that referenced
this pull request
Dec 8, 2022
This patch makes the "primordial" permanent upgrades not run in clusters being upgraded from 22.2 or lower. Startup migrations were recently replaced with "permanent upgrades", in cockroachdb#91627. The permanent new upgrades were running on node startup regardless of whether it was a "new cluster" or an existing one - i.e. regardless of whether the previous startupmigrations had also run, on the argument that startupmigrations and upgrades are supposed to be idempotent anyway. But, alas, there's idempotency and then there's idempotency: startupmigrations/upgrades are supposed to be OK with running multiple times at the same BinaryVersion in a "new cluster", but not necessarily across BinaryVersions and in a cluster that has been in use. There are different things that can go wrong when one of these migrations runs multiple times in these general conditions, because the migration might makes implicit assumptions about the schema and data of the cluster it's running in: 1. A migration might assume that all the data in a system table was created before a CRDB version changed some semantics. For example, the migration deleted in cockroachdb#92597 was assuming that `CREATEROLE` role option had a certain meaning for all the users who have it and was doing a backfill across all these users. Running this migration again at an arbitrary later point would affect unintended new users. 2. A migration assumes the schema of system tables it operates on. As this schema (which is part of the bootstrap image for new cluster) changes with new versions, the code of the upgrade changes also. This means that the upgrade is no longer suitable for running against clusters that don't have the upgraded schema - i.e. clusters that have not yet been upgraded to the latest schema. This is a real problem because we're adding a column to system.users, and the addRootUser upgrade can no longer run against 22.2 clusters. This patch guards all upgrades on checking that the old corresponding startupmigration has not run, thus enabling the mentioned system schema change in 23.1. Release note: None Epic: None
craig bot
pushed a commit
that referenced
this pull request
Dec 8, 2022
92363: sql,streamingest: introduce ALTER TENANT ... PAUSE/RESUME REPLICATION r=lidorcarmel a=adityamaru This change introduces: `ALTER TENANT <name> RESUME REPLICATION` `ALTER TENANT <name> PAUSE REPLICATION` These statements can be used to pause and resume the replication job if the tenant is being replicated into. Fixes: #91246 Release note: None 93071: upgrade: don't blindly run permanent migrations r=andreimatei a=andreimatei This patch makes the "primordial" permanent upgrades not run in clusters being upgraded from 22.2 or lower. Startup migrations were recently replaced with "permanent upgrades", in #91627. The permanent new upgrades were running on node startup regardless of whether it was a "new cluster" or an existing one - i.e. regardless of whether the previous startupmigrations had also run, on the argument that startupmigrations and upgrades are supposed to be idempotent anyway. But, alas, there's idempotency and then there's idempotency: startupmigrations/upgrades are supposed to be OK with running multiple times at the same BinaryVersion in a "new cluster", but not necessarily across BinaryVersions and in a cluster that has been in use. There are different things that can go wrong when one of these migrations runs multiple times in these general conditions, because the migration might makes implicit assumptions about the schema and data of the cluster it's running in: 1. A migration might assume that all the data in a system table was created before a CRDB version changed some semantics. For example, the migration deleted in #92597 was assuming that `CREATEROLE` role option had a certain meaning for all the users who have it and was doing a backfill across all these users. Running this migration again at an arbitrary later point would affect unintended new users. 2. A migration assumes the schema of system tables it operates on. As this schema (which is part of the bootstrap image for new cluster) changes with new versions, the code of the upgrade changes also. This means that the upgrade is no longer suitable for running against clusters that don't have the upgraded schema - i.e. clusters that have not yet been upgraded to the latest schema. This is a real problem because we're adding a column to system.users, and the addRootUser upgrade can no longer run against 22.2 clusters. This patch guards all upgrades on checking that the old corresponding startupmigration has not run, thus enabling the mentioned system schema change in 23.1. Release note: None Epic: None Co-authored-by: adityamaru <adityamaru@gmail.com> Co-authored-by: Andrei Matei <andrei@cockroachlabs.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This patch removes a permanent upgrade that was granting the CREATELOGIN role option to users that had the CREATEROLE option. This upgrade was introduced in 20.2, meant to grant the then-new CREATELOGIN option to users created in 20.1 and prior that had the CREATEROLE option (CREATELOGIN was being split from CREATEROLE).
This code stayed around as a startupmigration since it was introduced, even though it didn't make sense for it to stay around after 20.2. Technically, I think this startup migration should have had the flag
IncludedInBootstrap=v20.2, since we don't want it to run for clusters created at or after 20.2; this migration is not idempotent in the general sense, since it potentially picks up new, unintended users when it runs. Since 22.2, this migration would fail to run on anything but an empty system.role_options table because it would attempt to put NULLs into a non-nullable column. This was all benign since the startupmigrations had protection in place, preventing them for running a 2nd time after a completed attempt. So, for upgraded cluster, the migration only when the first 20.2 node came up, and for new clusters it would be a no-op since the system.role_options table starts empty.This migration became problematic in #91627 when I've turned the startupmigrations into upgrades. These upgrades run once when upgrading to 23.1; I'm relying on their idempotence.
Fixes #92230
Fixes #92371
Fixes #92569
Release note: None
Epic: None