-
Notifications
You must be signed in to change notification settings - Fork 3.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
sql, featureflag: add schema change feature flag #57040
sql, featureflag: add schema change feature flag #57040
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is awesome! two small changes requested.
Reviewed 41 of 41 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @angelapwen and @pbardea)
pkg/sql/alter_database.go, line 36 at r1 (raw file):
if err := featureflag.CheckEnabled(featureSchemaChangeEnabled, &p.ExecCfg().Settings.SV, "ALTER DATABASE is part of the schema change category, which",
if we're including this message for all schema changes, might be worth adding a helper function so you only put ALTER DATABASE here. maybe put the function in this package so the featureflag
package is schema change agnostic.
pkg/sql/create_stats.go, line 55 at r1 (raw file):
if err := featureflag.CheckEnabled(featureSchemaChangeEnabled, &p.ExecCfg().Settings.SV, "ANALYZE/CREATE STATS is part of the schema change category, which"); err != nil {
i think these aren't schema changes -- might be worth creating a separate PR and put this as part of the CREATE_STATS or ANALYZE flag.
pkg/sql/schema_change_cluster_setting.go, line 24 at r1 (raw file):
// package? var featureSchemaChangeEnabled = settings.RegisterPublicBoolSetting( "feature.schemachange.enabled",
can we make this schema_change
since it is two words?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops, premature accept.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @angelapwen and @pbardea)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @otan and @pbardea)
pkg/sql/alter_database.go, line 36 at r1 (raw file):
Previously, otan (Oliver Tan) wrote…
if we're including this message for all schema changes, might be worth adding a helper function so you only put ALTER DATABASE here. maybe put the function in this package so the
featureflag
package is schema change agnostic.
Yeah, that makes sense. I'll put it under the schema_change_cluster_setting
package where I register the settings.
pkg/sql/create_stats.go, line 55 at r1 (raw file):
Previously, otan (Oliver Tan) wrote…
i think these aren't schema changes -- might be worth creating a separate PR and put this as part of the CREATE_STATS or ANALYZE flag.
Ah alright!
pkg/sql/schema_change_cluster_setting.go, line 24 at r1 (raw file):
Previously, otan (Oliver Tan) wrote…
can we make this
schema_change
since it is two words?
Yes!
b71c2db
to
93fd5af
Compare
93fd5af
to
d1bf094
Compare
11ddecf
to
a402346
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like two small failures left.
go test ./pkg/sql/pgwire -run TestPGTest/notice -rewrite
to fix the notice one!
Reviewed 42 of 42 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @otan)
This change adds a feature flag via cluster setting for all designated features that perform schema changes or DDLs. The feature is being introduced to address a Cockroach Cloud SRE use case: needing to disable certain categories of features, such as schema changes, in case of cluster failure. Release note (sql change): Adds a feature flag via cluster setting for all schema change-related features. If a user attempts to use these features while they are disabled, an error indicating that the database administrator has disabled the feature is surfaced. Example usage for the database administrator: SET CLUSTER SETTING feature.schemachange.enabled = FALSE; SET CLUSTER SETTING feature.schemachange.enabled = TRUE;
a402346
to
1166346
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I think I've addressed the two failures just now.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @otan)
bors r=otan |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @otan)
Build failed: |
Flaky roachtest, giving this another go 🙏 bors r=otan |
Build succeeded: |
Release note (sql change): This is an empty commit meant to correct a mistake in a merged release note in cockroachdb#57040. The original release note indicates that a database administrator should toggle this feature flag on and off using SET CLUSTER SETTING feature.schemachange.enabled, but there should be a '_' character so that the cluster setting would be: SET CLUSTER SETTING feature.schema_change.enabled. Below is the full original release note, with the typo fixed. Release note (sql change): Adds a feature flag via cluster setting for all schema change-related features. If a user attempts to use these features while they are disabled, an error indicating that the database administrator has disabled the feature is surfaced. Example usage for the database administrator: SET CLUSTER SETTING feature.schema_change.enabled = FALSE; SET CLUSTER SETTING feature.schema_change.enabled = TRUE;
57143: docs: edit release note for schema change feature flag r=otan a=angelapwen Release note (sql change): This is an empty commit meant to correct a mistake in a merged release note in #57040. The original release note indicates that a database administrator should toggle this feature flag on and off using SET CLUSTER SETTING feature.schemachange.enabled, but there should be a '_' character so that the cluster setting would be: SET CLUSTER SETTING feature.schema_change.enabled. Below is the full original release note, with the typo fixed. Release note (sql change): Adds a feature flag via cluster setting for all schema change-related features. If a user attempts to use these features while they are disabled, an error indicating that the database administrator has disabled the feature is surfaced. Example usage for the database administrator: SET CLUSTER SETTING feature.schema_change.enabled = FALSE; SET CLUSTER SETTING feature.schema_change.enabled = TRUE; Co-authored-by: angelapwen <[email protected]>
The following features have been tested and act as expected:
See #51643 for background.
ANALYZE/CREATE STATISTICS will not be considered a schema change and were added in a separate PR, #57076
The SPLIT/UNSPLIT features will also be resolved in a separate PR, as the execution path is different from the schema changes addressed in this PR.
---Commit message---
This change adds a feature flag via cluster setting for all
designated features that perform schema changes or DDLs. The
feature is being introduced to address a Cockroach Cloud SRE use
case: needing to disable certain categories of features, such as
schema changes, in case of cluster failure.
Release note (sql change): Adds a feature flag via cluster
setting for all schema change-related features. If a user attempts
to use these features while they are disabled, an error indicating
that the database administrator has disabled the feature is
surfaced.
Example usage for the database administrator:
SET CLUSTER SETTING feature.schemachange.enabled = FALSE;
SET CLUSTER SETTING feature.schemachange.enabled = TRUE;