sql: add session variable to set schema_locked by default on new tables #143892
Conversation
![blathers[crl]](https://avatars.githubusercontent.com/in/59700?s=40&u=95b789b1d594ac6b74f377aa7555d8d98d63240d&v=4){kind=avatar}
|
Your pull request contains more than 1000 changes. It is strongly encouraged to split big PRs into smaller chunks. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
This change is |
fqazi
force-pushed
the
createTableLockDefault
branch
18 times, most recently
from
dfe9a5a to
909c825
Compare
There was a problem hiding this comment.
nice work!
Reviewed 32 of 840 files at r4, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained
-- commits line 11 at r4:
this should get a release note that describes the session variable. (let's not mention the cluster setting)
pkg/ccl/schemachangerccl/testdata/end_to_end/add_column_subzones/add_column_subzones.side_effects line 631 at r4 (raw file):
begin transaction #12 ## PostCommitNonRevertiblePhase stage 3 of 4 with 5 MutationType ops upsert descriptor #104
just curious, this seems fine, but do you know why the order of side effects changed? is it because of the NonRevertible change?
pkg/sql/create_table.go line 2574 at r1 (raw file):
if !ret.IsView() && !ret.IsSequence() && params.p.SessionData().CreateTableWithSchemaLocked && params.p.IsActive(params.ctx, clusterversion.V25_2) {
if we add another check here for p.SessionData().Internal to skip this for internal executors, could we avoid having to disable it for backup/restore tests?
pkg/sql/schemachanger/sctest/decomp.go line 47 at r4 (raw file):
// We need to disable the declarative schema changer so that we don't end // up high-fiving ourselves here. tdb.Exec(t, `SET CLUSTER SETTING sql.defaults.use_declarative_schema_changer = 'off'`)
why is this getting removed? (and also, do you know why was this even here before?)
pkg/sql/catalog/rewrite/rewrite.go line 1180 at r4 (raw file):
} // Rewrite any function descriptors in the schema change state. if err := rewriteSchemaChangerState(fnDesc, descriptorRewrites); err != nil {
how did this turn up? was there a bug lurking here from before?
There was a problem hiding this comment.
Reviewable status:
Previously, rafiss (Rafi Shamim) wrote…
this should get a release note that describes the session variable. (let's not mention the cluster setting)
if you want to wait for the release ntoe until we toggle the default to true, that could be fine too
fqazi
force-pushed
the
createTableLockDefault
branch
2 times, most recently
from
ae070ef to
d26ec10
Compare
There was a problem hiding this comment.
Reviewable status:
Previously, rafiss (Rafi Shamim) wrote…
if you want to wait for the release ntoe until we toggle the default to true, that could be fine too
Done.
Added:
Release note (sql change): Added a new session variable create_table_with_schema_locked, which can be used to ensure all tables created by a session have the storage parameter schema_locked set.
pkg/sql/create_table.go line 2574 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
if we add another check here for
p.SessionData().Internalto skip this for internal executors, could we avoid having to disable it for backup/restore tests?
Done.
pkg/sql/schemachanger/sctest/decomp.go line 47 at r4 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
why is this getting removed? (and also, do you know why was this even here before?)
Its being removed so that the entire setup phase uses the declarative schema changer, previously it would run in the legacy schema changer. This prevents schema_locked errors from getting hit.
The test doesn't use any testing knobs, so this seems completely unnecessary. I'm guessing we had broken fallbacks or something, which is no longer an issue.
pkg/ccl/schemachangerccl/testdata/end_to_end/add_column_subzones/add_column_subzones.side_effects line 631 at r4 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
just curious, this seems fine, but do you know why the order of side effects changed? is it because of the NonRevertible change?
This is linked to schema_locked. Stage 3 is doing the same but not unsetting job state since its not the final stage. Stage 4 clears all the job stuff and flips schema_locked back on again
pkg/sql/catalog/rewrite/rewrite.go line 1180 at r4 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
how did this turn up? was there a bug lurking here from before?
Done.
I can't seem to hit this anymore and I wonder if it was just because of a bad testing know when I was developing. Unless we use transactions we really shouldn't need to persist anything in the function descriptor yet. So removing this for now.
There was a problem hiding this comment.
Reviewable status:
pkg/sql/catalog/rewrite/rewrite.go line 1180 at r4 (raw file):
Previously, fqazi (Faizan Qazi) wrote…
Done.
I can't seem to hit this anymore and I wonder if it was just because of a bad testing know when I was developing. Unless we use transactions we really shouldn't need to persist anything in the function descriptor yet. So removing this for now.
*bad testing knob
Previously, schema_locked had to be manually set on objects during creation. This meant that to gain performance improvements on CDC users would manually have to toggle this on. To address this, this patch adds a cluster setting and session variable for creating new tables with schema_locked set. Informs: cockroachdb#129694 Release note (sql change): Added a new session variable create_table_with_schema_locked, which can be used to ensure all tables created by a session have the storage parameter schema_locked set.
Previously, we would plan rollbacks starting from "PostCommitPhase", and relied on opgen to label things as non-revertible. This would ensure that majority of plans would get non-revertible implicitly. This can be problematic if we automatically set/unset schema_locked and certain plans will no longer start as non-revertible on rollback. To address this, this patch explicitly ensures that rollbacks start as non-revertible. Informs: cockroachdb#129694 Release note: None
Previously, trigger dependents used the NotImplementedForPublicObjects operation to indicate that no operations were needed. Sadly because triggers are not descriptors, this could lead to assertions in more complex plans, when schema_locked is involved. To address this, this patch modifies this operation to accept a trigger ID to detect if the trigger is dropped. Release note: None
Previously, there were individual tests for confirming if schema_locked worked correctly in the declarative schema changer. This patch flips create_table_with_schema_locked and update tests to handle this. Specifically it does the following: 1) All tables are created as schema_locked in the declarative schema changer tests 2) All setup statements are now run via the declarative schema changer so that schema_locked is almost never manually toggled 3) Updates all test outputs to expected schema_locked Informs: cockroachdb#129694 Release note: None
fqazi
force-pushed
the
createTableLockDefault
branch
from
d26ec10 to
dabb49e
Compare
This patch will add a new session variable / cluster setting for automatically setting schema_locked:
create_table_with_schema_lockedand cluster settingsql.defaults.create_table_with_schema_locked, which will create all new tables with schema_locked by defaultRelease note (sql change): The create_table_with_schema_locked session variable can be used to control if newly created tables should have schema_locked set.
Informs: #129694
Fixes: #143891