sql, featureflag: add schema change feature flag by angelapwen · Pull Request #57040 · cockroachdb/cockroach

angelapwen · 2020-11-24T00:30:39Z

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;

cockroach-teamcity · 2020-11-24T00:30:47Z

This change is

otan

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?

otan

oops, premature accept.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @angelapwen and @pbardea)

angelapwen

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!

otan

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;

angelapwen

Thanks! I think I've addressed the two failures just now.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @otan)

angelapwen · 2020-11-25T03:53:18Z

bors r=otan

otan

lgtm

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @otan)

craig · 2020-11-25T04:54:37Z

Build failed:

GitHub CI (Cockroach)

angelapwen · 2020-11-25T06:50:41Z

Flaky roachtest, giving this another go 🙏

bors r=otan

craig · 2020-11-25T07:43:04Z

Build succeeded:

GitHub CI (Cockroach)

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 <angelaw@cockroachlabs.com>

angelapwen added the do-not-merge bors won't merge a PR with this label. label Nov 24, 2020

angelapwen requested review from a team and otan November 24, 2020 00:30

angelapwen self-assigned this Nov 24, 2020

angelapwen requested review from pbardea and removed request for a team November 24, 2020 00:30

otan approved these changes Nov 24, 2020

View reviewed changes

otan suggested changes Nov 24, 2020

View reviewed changes

angelapwen commented Nov 24, 2020

View reviewed changes

angelapwen force-pushed the add-schema-change-feature-flag branch from b71c2db to 93fd5af Compare November 24, 2020 01:34

blathers-crl bot requested a review from otan November 24, 2020 01:34

angelapwen force-pushed the add-schema-change-feature-flag branch from 93fd5af to d1bf094 Compare November 24, 2020 01:43

pbardea removed their request for review November 24, 2020 20:40

angelapwen force-pushed the add-schema-change-feature-flag branch 6 times, most recently from 11ddecf to a402346 Compare November 25, 2020 00:40

angelapwen changed the title ~~sql, featureflag: [WIP] add schema change feature flag~~ sql, featureflag: add schema change feature flag Nov 25, 2020

otan removed the do-not-merge bors won't merge a PR with this label. label Nov 25, 2020

otan reviewed Nov 25, 2020

View reviewed changes

angelapwen force-pushed the add-schema-change-feature-flag branch from a402346 to 1166346 Compare November 25, 2020 02:32

blathers-crl bot requested a review from otan November 25, 2020 02:32

angelapwen commented Nov 25, 2020

View reviewed changes

otan approved these changes Nov 25, 2020

View reviewed changes

craig bot merged commit 82ae151 into cockroachdb:master Nov 25, 2020

angelapwen mentioned this pull request Nov 25, 2020

docs: edit release note for schema change feature flag #57143

Merged

jseldess mentioned this pull request Dec 9, 2020

sql, featureflag: add schema change feature flag cockroachdb/docs#9121

Closed

angelapwen deleted the add-schema-change-feature-flag branch February 5, 2021 00:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql, featureflag: add schema change feature flag#57040

sql, featureflag: add schema change feature flag#57040
craig[bot] merged 1 commit intocockroachdb:masterfrom
angelapwen:add-schema-change-feature-flag

angelapwen commented Nov 24, 2020 •

edited

Loading

Uh oh!

cockroach-teamcity commented Nov 24, 2020

Uh oh!

otan left a comment

Uh oh!

otan left a comment

Uh oh!

angelapwen left a comment

Uh oh!

otan left a comment

Uh oh!

angelapwen left a comment

Uh oh!

angelapwen commented Nov 25, 2020

Uh oh!

otan left a comment

Uh oh!

craig bot commented Nov 25, 2020

Uh oh!

angelapwen commented Nov 25, 2020

Uh oh!

craig bot commented Nov 25, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

angelapwen commented Nov 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cockroach-teamcity commented Nov 24, 2020

Uh oh!

otan left a comment

Choose a reason for hiding this comment

Uh oh!

otan left a comment

Choose a reason for hiding this comment

Uh oh!

angelapwen left a comment

Choose a reason for hiding this comment

Uh oh!

otan left a comment

Choose a reason for hiding this comment

Uh oh!

angelapwen left a comment

Choose a reason for hiding this comment

Uh oh!

angelapwen commented Nov 25, 2020

Uh oh!

otan left a comment

Choose a reason for hiding this comment

Uh oh!

craig bot commented Nov 25, 2020

Uh oh!

angelapwen commented Nov 25, 2020

Uh oh!

craig bot commented Nov 25, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

angelapwen commented Nov 24, 2020 •

edited

Loading