From a34ef5804c1da38f68738c03c59cb3c9dcf400cd Mon Sep 17 00:00:00 2001 From: Rafi Shamim Date: Sat, 17 Jun 2023 12:00:30 -0400 Subject: [PATCH 1/2] sql: use featureflag check for new schema changer Now the check uses the statement tag to determine which operation is running. I took the opportunity to make sure the statement tags matched with the ones Postgres returns. Release note: None --- .../testdata/logic_test/multi_region | 2 +- .../logictest/testdata/logic_test/event_log | 10 ++--- .../testdata/logic_test/event_log_legacy | 10 ++--- .../logic_test/schema_change_feature_flags | 14 +++--- pkg/sql/schema_change_plan_node.go | 5 ++- pkg/sql/sem/tree/stmt.go | 45 ++++++++++++------- 6 files changed, 51 insertions(+), 35 deletions(-) diff --git a/pkg/ccl/logictestccl/testdata/logic_test/multi_region b/pkg/ccl/logictestccl/testdata/logic_test/multi_region index e77a368bf6df..ce788aeb82ae 100644 --- a/pkg/ccl/logictestccl/testdata/logic_test/multi_region +++ b/pkg/ccl/logictestccl/testdata/logic_test/multi_region @@ -1088,7 +1088,7 @@ SELECT "reportingID", info::JSONB - 'Timestamp' - 'DescriptorID' FROM system.eventlog WHERE "eventType" = 'alter_database_drop_region' ---- -1 {"DatabaseName": "drop_region_db", "EventType": "alter_database_drop_region", "RegionName": "\"us-east-1\"", "Statement": "ALTER DATABASE drop_region_db DROP REGION IF EXISTS \"us-east-1\"", "Tag": "ALTER DATABASE DROP REGION", "User": "root"} +1 {"DatabaseName": "drop_region_db", "EventType": "alter_database_drop_region", "RegionName": "\"us-east-1\"", "Statement": "ALTER DATABASE drop_region_db DROP REGION IF EXISTS \"us-east-1\"", "Tag": "ALTER DATABASE", "User": "root"} query T noticetrace ALTER DATABASE drop_region_db DROP REGION IF EXISTS "us-east-1" diff --git a/pkg/sql/logictest/testdata/logic_test/event_log b/pkg/sql/logictest/testdata/logic_test/event_log index 3f61c6bc9078..832eb797d840 100644 --- a/pkg/sql/logictest/testdata/logic_test/event_log +++ b/pkg/sql/logictest/testdata/logic_test/event_log @@ -852,9 +852,9 @@ SELECT "reportingID", "eventType", info::JSONB - 'Timestamp' - 'DescriptorID' WHERE "eventType" LIKE '%_owner' ORDER BY "timestamp", info ---- -1 alter_database_owner {"DatabaseName": "atest", "EventType": "alter_database_owner", "Owner": "u", "Statement": "ALTER DATABASE atest OWNER TO u", "Tag": "ALTER DATABASE OWNER", "User": "root"} +1 alter_database_owner {"DatabaseName": "atest", "EventType": "alter_database_owner", "Owner": "u", "Statement": "ALTER DATABASE atest OWNER TO u", "Tag": "ALTER DATABASE", "User": "root"} 1 alter_schema_owner {"EventType": "alter_schema_owner", "Owner": "u", "SchemaName": "atest.sc", "Statement": "ALTER SCHEMA atest.sc OWNER TO u", "Tag": "ALTER SCHEMA", "User": "root"} -1 alter_table_owner {"EventType": "alter_table_owner", "Owner": "u", "Statement": "ALTER TABLE atest.sc.t OWNER TO u", "TableName": "atest.sc.t", "Tag": "ALTER TABLE OWNER", "User": "root"} +1 alter_table_owner {"EventType": "alter_table_owner", "Owner": "u", "Statement": "ALTER TABLE atest.sc.t OWNER TO u", "TableName": "atest.sc.t", "Tag": "ALTER TABLE", "User": "root"} 1 alter_type_owner {"EventType": "alter_type_owner", "Owner": "u", "Statement": "ALTER TYPE atest.sc.ty OWNER TO u", "Tag": "ALTER TYPE", "TypeName": "atest.sc.ty", "User": "root"} 1 alter_type_owner {"EventType": "alter_type_owner", "Owner": "u", "Statement": "ALTER TYPE atest.sc.ty OWNER TO u", "Tag": "ALTER TYPE", "TypeName": "atest.sc._ty", "User": "root"} @@ -1046,9 +1046,9 @@ ORDER BY "timestamp", info ---- 1 {"DescriptorName": "defaultdb.public.eventlog", "DescriptorType": "type", "EventType": "set_schema", "NewDescriptorName": "defaultdb.testing.eventlog", "Statement": "ALTER TYPE defaultdb.public.eventlog SET SCHEMA testing", "Tag": "ALTER TYPE", "User": "root"} 1 {"DescriptorName": "defaultdb.testing.eventlog", "DescriptorType": "type", "EventType": "set_schema", "NewDescriptorName": "defaultdb.public.eventlog", "Statement": "ALTER TYPE defaultdb.testing.eventlog SET SCHEMA public", "Tag": "ALTER TYPE", "User": "root"} -1 {"DescriptorName": "defaultdb.public.a", "DescriptorType": "table", "EventType": "set_schema", "NewDescriptorName": "defaultdb.testing.a", "Statement": "ALTER TABLE a SET SCHEMA testing", "Tag": "ALTER TABLE SET SCHEMA", "User": "root"} -1 {"DescriptorName": "defaultdb.public.s", "DescriptorType": "sequence", "EventType": "set_schema", "NewDescriptorName": "defaultdb.testing.s", "Statement": "ALTER SEQUENCE s SET SCHEMA testing", "Tag": "ALTER TABLE SET SCHEMA", "User": "root"} -1 {"DescriptorName": "defaultdb.public.v", "DescriptorType": "view", "EventType": "set_schema", "NewDescriptorName": "defaultdb.test_sc.v", "Statement": "ALTER VIEW v SET SCHEMA test_sc", "Tag": "ALTER TABLE SET SCHEMA", "User": "root"} +1 {"DescriptorName": "defaultdb.public.a", "DescriptorType": "table", "EventType": "set_schema", "NewDescriptorName": "defaultdb.testing.a", "Statement": "ALTER TABLE a SET SCHEMA testing", "Tag": "ALTER TABLE", "User": "root"} +1 {"DescriptorName": "defaultdb.public.s", "DescriptorType": "sequence", "EventType": "set_schema", "NewDescriptorName": "defaultdb.testing.s", "Statement": "ALTER SEQUENCE s SET SCHEMA testing", "Tag": "ALTER SEQUENCE", "User": "root"} +1 {"DescriptorName": "defaultdb.public.v", "DescriptorType": "view", "EventType": "set_schema", "NewDescriptorName": "defaultdb.test_sc.v", "Statement": "ALTER VIEW v SET SCHEMA test_sc", "Tag": "ALTER VIEW", "User": "root"} # Test the event logs generated by commands that drop views. diff --git a/pkg/sql/logictest/testdata/logic_test/event_log_legacy b/pkg/sql/logictest/testdata/logic_test/event_log_legacy index e93b961e5f02..4ccbfec07378 100644 --- a/pkg/sql/logictest/testdata/logic_test/event_log_legacy +++ b/pkg/sql/logictest/testdata/logic_test/event_log_legacy @@ -853,9 +853,9 @@ SELECT "reportingID", "eventType", info::JSONB - 'Timestamp' - 'DescriptorID' WHERE "eventType" LIKE '%_owner' ORDER BY "timestamp", info ---- -1 alter_database_owner {"DatabaseName": "atest", "EventType": "alter_database_owner", "Owner": "u", "Statement": "ALTER DATABASE atest OWNER TO u", "Tag": "ALTER DATABASE OWNER", "User": "root"} +1 alter_database_owner {"DatabaseName": "atest", "EventType": "alter_database_owner", "Owner": "u", "Statement": "ALTER DATABASE atest OWNER TO u", "Tag": "ALTER DATABASE", "User": "root"} 1 alter_schema_owner {"EventType": "alter_schema_owner", "Owner": "u", "SchemaName": "atest.sc", "Statement": "ALTER SCHEMA atest.sc OWNER TO u", "Tag": "ALTER SCHEMA", "User": "root"} -1 alter_table_owner {"EventType": "alter_table_owner", "Owner": "u", "Statement": "ALTER TABLE atest.sc.t OWNER TO u", "TableName": "atest.sc.t", "Tag": "ALTER TABLE OWNER", "User": "root"} +1 alter_table_owner {"EventType": "alter_table_owner", "Owner": "u", "Statement": "ALTER TABLE atest.sc.t OWNER TO u", "TableName": "atest.sc.t", "Tag": "ALTER TABLE", "User": "root"} 1 alter_type_owner {"EventType": "alter_type_owner", "Owner": "u", "Statement": "ALTER TYPE atest.sc.ty OWNER TO u", "Tag": "ALTER TYPE", "TypeName": "atest.sc.ty", "User": "root"} 1 alter_type_owner {"EventType": "alter_type_owner", "Owner": "u", "Statement": "ALTER TYPE atest.sc.ty OWNER TO u", "Tag": "ALTER TYPE", "TypeName": "atest.sc._ty", "User": "root"} @@ -1048,9 +1048,9 @@ ORDER BY "timestamp", info ---- 1 {"DescriptorName": "defaultdb.public.eventlog", "DescriptorType": "type", "EventType": "set_schema", "NewDescriptorName": "defaultdb.testing.eventlog", "Statement": "ALTER TYPE defaultdb.public.eventlog SET SCHEMA testing", "Tag": "ALTER TYPE", "User": "root"} 1 {"DescriptorName": "defaultdb.testing.eventlog", "DescriptorType": "type", "EventType": "set_schema", "NewDescriptorName": "defaultdb.public.eventlog", "Statement": "ALTER TYPE defaultdb.testing.eventlog SET SCHEMA public", "Tag": "ALTER TYPE", "User": "root"} -1 {"DescriptorName": "defaultdb.public.a", "DescriptorType": "table", "EventType": "set_schema", "NewDescriptorName": "defaultdb.testing.a", "Statement": "ALTER TABLE a SET SCHEMA testing", "Tag": "ALTER TABLE SET SCHEMA", "User": "root"} -1 {"DescriptorName": "defaultdb.public.s", "DescriptorType": "sequence", "EventType": "set_schema", "NewDescriptorName": "defaultdb.testing.s", "Statement": "ALTER SEQUENCE s SET SCHEMA testing", "Tag": "ALTER TABLE SET SCHEMA", "User": "root"} -1 {"DescriptorName": "defaultdb.public.v", "DescriptorType": "view", "EventType": "set_schema", "NewDescriptorName": "defaultdb.test_sc.v", "Statement": "ALTER VIEW v SET SCHEMA test_sc", "Tag": "ALTER TABLE SET SCHEMA", "User": "root"} +1 {"DescriptorName": "defaultdb.public.a", "DescriptorType": "table", "EventType": "set_schema", "NewDescriptorName": "defaultdb.testing.a", "Statement": "ALTER TABLE a SET SCHEMA testing", "Tag": "ALTER TABLE", "User": "root"} +1 {"DescriptorName": "defaultdb.public.s", "DescriptorType": "sequence", "EventType": "set_schema", "NewDescriptorName": "defaultdb.testing.s", "Statement": "ALTER SEQUENCE s SET SCHEMA testing", "Tag": "ALTER SEQUENCE", "User": "root"} +1 {"DescriptorName": "defaultdb.public.v", "DescriptorType": "view", "EventType": "set_schema", "NewDescriptorName": "defaultdb.test_sc.v", "Statement": "ALTER VIEW v SET SCHEMA test_sc", "Tag": "ALTER VIEW", "User": "root"} # Test the event logs generated by commands that drop views. diff --git a/pkg/sql/logictest/testdata/logic_test/schema_change_feature_flags b/pkg/sql/logictest/testdata/logic_test/schema_change_feature_flags index 38911e4179e0..a3196200f20f 100644 --- a/pkg/sql/logictest/testdata/logic_test/schema_change_feature_flags +++ b/pkg/sql/logictest/testdata/logic_test/schema_change_feature_flags @@ -73,7 +73,7 @@ statement error pq: feature ALTER DATABASE is part of the schema change category ALTER DATABASE d RENAME TO r # Test REPARENT DATABASE -statement error pq: cannot perform ALTER DATABASE CONVERT TO SCHEMA +statement error feature ALTER DATABASE is part of the schema change category, which was disabled by the database administrator ALTER DATABASE d CONVERT TO SCHEMA WITH PARENT test # Test ALTER TABLE PARTITION BY. @@ -117,11 +117,11 @@ statement error pq: feature CONFIGURE ZONE is part of the schema change category ALTER TABLE t1 CONFIGURE ZONE USING num_replicas=5 # Test RENAME TABLE. -statement error pq: feature RENAME TABLE/VIEW/SEQUENCE is part of the schema change category, which was disabled by the database administrator +statement error pq: feature ALTER TABLE is part of the schema change category, which was disabled by the database administrator ALTER TABLE t RENAME TO r # Test ALTER TABLE SET SCHEMA. -statement error pq: feature ALTER TABLE/VIEW/SEQUENCE SET SCHEMA is part of the schema change category, which was disabled by the database administrator +statement error pq: feature ALTER TABLE is part of the schema change category, which was disabled by the database administrator ALTER TABLE t SET SCHEMA s # Test ALTER TABLE SET LOCALITY REGIONAL BY ROW. @@ -149,7 +149,7 @@ statement error pq: feature CONFIGURE ZONE is part of the schema change category ALTER INDEX t1@i CONFIGURE ZONE DISCARD # Test RENAME INDEX -statement error pq: feature RENAME INDEX is part of the schema change category, which was disabled by the database administrator +statement error pq: feature ALTER INDEX is part of the schema change category, which was disabled by the database administrator ALTER INDEX t1@i RENAME TO r # TODO(angelaw): Test ALTER INDEX SPLIT AT. This does not throw error. @@ -193,11 +193,11 @@ ALTER TYPE s.typ SET SCHEMA s # Test ALTER SEQUENCE. # Test RENAME SEQUENCE. -statement error pq: feature RENAME TABLE/VIEW/SEQUENCE is part of the schema change category, which was disabled by the database administrator +statement error pq: feature ALTER SEQUENCE is part of the schema change category, which was disabled by the database administrator ALTER SEQUENCE seq RENAME TO something # Test ALTER SEQUENCE SET SCHEMA -statement error pq: feature ALTER TABLE/VIEW/SEQUENCE SET SCHEMA is part of the schema change category, which was disabled by the database administrator +statement error pq: feature ALTER SEQUENCE is part of the schema change category, which was disabled by the database administrator ALTER SEQUENCE seq SET SCHEMA s statement error pq: feature ALTER SEQUENCE is part of the schema change category, which was disabled by the database administrator @@ -234,7 +234,7 @@ DROP SEQUENCE seq # Test DROP VIEW. # Test ALTER VIEW SET SCHEMA -statement error pq: feature ALTER TABLE/VIEW/SEQUENCE SET SCHEMA is part of the schema change category, which was disabled by the database administrator +statement error pq: feature ALTER VIEW is part of the schema change category, which was disabled by the database administrator ALTER VIEW public.bar SET SCHEMA s statement error pq: feature DROP VIEW is part of the schema change category, which was disabled by the database administrator diff --git a/pkg/sql/schema_change_plan_node.go b/pkg/sql/schema_change_plan_node.go index 4319669032c5..434850cdf2a4 100644 --- a/pkg/sql/schema_change_plan_node.go +++ b/pkg/sql/schema_change_plan_node.go @@ -51,7 +51,10 @@ func (p *planner) FormatAstAsRedactableString( // SchemaChange provides the planNode for the new schema changer. func (p *planner) SchemaChange(ctx context.Context, stmt tree.Statement) (planNode, error) { - // TODO(ajwerner): Call featureflag.CheckEnabled appropriately. + err := checkSchemaChangeEnabled(ctx, p.ExecCfg(), p.stmt.AST.StatementTag()) + if err != nil { + return nil, err + } mode := p.extendedEvalCtx.SchemaChangerState.mode // When new schema changer is on we will not support it for explicit // transaction, since we don't know if subsequent statements don't diff --git a/pkg/sql/sem/tree/stmt.go b/pkg/sql/sem/tree/stmt.go index 686ccd1d1b0c..15271cdc2f68 100644 --- a/pkg/sql/sem/tree/stmt.go +++ b/pkg/sql/sem/tree/stmt.go @@ -253,7 +253,7 @@ func (*AlterDatabaseOwner) StatementReturnType() StatementReturnType { return DD func (*AlterDatabaseOwner) StatementType() StatementType { return TypeDCL } // StatementTag returns a short string identifying the type of statement. -func (*AlterDatabaseOwner) StatementTag() string { return "ALTER DATABASE OWNER" } +func (*AlterDatabaseOwner) StatementTag() string { return "ALTER DATABASE" } func (*AlterDatabaseOwner) hiddenFromShowQueries() {} @@ -264,7 +264,7 @@ func (*AlterDatabaseAddRegion) StatementReturnType() StatementReturnType { retur func (*AlterDatabaseAddRegion) StatementType() StatementType { return TypeDDL } // StatementTag returns a short string identifying the type of statement. -func (*AlterDatabaseAddRegion) StatementTag() string { return "ALTER DATABASE ADD REGION" } +func (*AlterDatabaseAddRegion) StatementTag() string { return "ALTER DATABASE" } func (*AlterDatabaseAddRegion) hiddenFromShowQueries() {} @@ -275,7 +275,7 @@ func (*AlterDatabaseDropRegion) StatementReturnType() StatementReturnType { retu func (*AlterDatabaseDropRegion) StatementType() StatementType { return TypeDDL } // StatementTag returns a short string identifying the type of statement. -func (*AlterDatabaseDropRegion) StatementTag() string { return "ALTER DATABASE DROP REGION" } +func (*AlterDatabaseDropRegion) StatementTag() string { return "ALTER DATABASE" } func (*AlterDatabaseDropRegion) hiddenFromShowQueries() {} @@ -286,7 +286,7 @@ func (*AlterDatabasePrimaryRegion) StatementReturnType() StatementReturnType { r func (*AlterDatabasePrimaryRegion) StatementType() StatementType { return TypeDDL } // StatementTag returns a short string identifying the type of statement. -func (*AlterDatabasePrimaryRegion) StatementTag() string { return "ALTER DATABASE PRIMARY REGION" } +func (*AlterDatabasePrimaryRegion) StatementTag() string { return "ALTER DATABASE" } func (*AlterDatabasePrimaryRegion) hiddenFromShowQueries() {} @@ -297,7 +297,7 @@ func (*AlterDatabaseSurvivalGoal) StatementReturnType() StatementReturnType { re func (*AlterDatabaseSurvivalGoal) StatementType() StatementType { return TypeDDL } // StatementTag returns a short string identifying the type of statement. -func (*AlterDatabaseSurvivalGoal) StatementTag() string { return "ALTER DATABASE SURVIVE" } +func (*AlterDatabaseSurvivalGoal) StatementTag() string { return "ALTER DATABASE" } func (*AlterDatabaseSurvivalGoal) hiddenFromShowQueries() {} @@ -308,7 +308,7 @@ func (*AlterDatabasePlacement) StatementReturnType() StatementReturnType { retur func (*AlterDatabasePlacement) StatementType() StatementType { return TypeDDL } // StatementTag returns a short string identifying the type of statement. -func (*AlterDatabasePlacement) StatementTag() string { return "ALTER DATABASE PLACEMENT" } +func (*AlterDatabasePlacement) StatementTag() string { return "ALTER DATABASE" } func (*AlterDatabasePlacement) hiddenFromShowQueries() {} @@ -319,7 +319,7 @@ func (*AlterDatabaseAddSuperRegion) StatementReturnType() StatementReturnType { func (*AlterDatabaseAddSuperRegion) StatementType() StatementType { return TypeDDL } // StatementTag returns a short string identifying the type of statement. -func (*AlterDatabaseAddSuperRegion) StatementTag() string { return "ALTER DATABASE ADD SUPER REGION" } +func (*AlterDatabaseAddSuperRegion) StatementTag() string { return "ALTER DATABASE" } func (*AlterDatabaseAddSuperRegion) hiddenFromShowQueries() {} @@ -330,7 +330,7 @@ func (*AlterDatabaseDropSuperRegion) StatementReturnType() StatementReturnType { func (*AlterDatabaseDropSuperRegion) StatementType() StatementType { return TypeDDL } // StatementTag returns a short string identifying the type of statement. -func (*AlterDatabaseDropSuperRegion) StatementTag() string { return "ALTER DATABASE DROP SUPER REGION" } +func (*AlterDatabaseDropSuperRegion) StatementTag() string { return "ALTER DATABASE" } func (*AlterDatabaseDropSuperRegion) hiddenFromShowQueries() {} @@ -342,7 +342,7 @@ func (*AlterDatabaseAlterSuperRegion) StatementType() StatementType { return Typ // StatementTag returns a short string identifying the type of statement. func (*AlterDatabaseAlterSuperRegion) StatementTag() string { - return "ALTER DATABASE ALTER SUPER REGION" + return "ALTER DATABASE" } func (*AlterDatabaseAlterSuperRegion) hiddenFromShowQueries() {} @@ -355,7 +355,7 @@ func (*AlterDatabaseSecondaryRegion) StatementType() StatementType { return Type // StatementTag returns a short string identifying the type of statement. func (*AlterDatabaseSecondaryRegion) StatementTag() string { - return "ALTER DATABASE SET SECONDARY REGION" + return "ALTER DATABASE" } func (*AlterDatabaseSecondaryRegion) hiddenFromShowQueries() {} @@ -368,7 +368,7 @@ func (*AlterDatabaseDropSecondaryRegion) StatementType() StatementType { return // StatementTag returns a short string identifying the type of statement. func (*AlterDatabaseDropSecondaryRegion) StatementTag() string { - return "ALTER DATABASE DROP SECONDARY REGION" + return "ALTER DATABASE" } func (*AlterDatabaseDropSecondaryRegion) hiddenFromShowQueries() {} @@ -381,7 +381,7 @@ func (*AlterDatabaseSetZoneConfigExtension) StatementType() StatementType { retu // StatementTag returns a short string identifying the type of statement. func (*AlterDatabaseSetZoneConfigExtension) StatementTag() string { - return "ALTER DATABASE ALTER LOCALITY CONFIGURE ZONE" + return "ALTER DATABASE" } func (*AlterDatabaseSetZoneConfigExtension) hiddenFromShowQueries() {} @@ -435,7 +435,7 @@ func (*AlterTableLocality) StatementReturnType() StatementReturnType { return DD func (*AlterTableLocality) StatementType() StatementType { return TypeDDL } // StatementTag returns a short string identifying the type of statement. -func (*AlterTableLocality) StatementTag() string { return "ALTER TABLE SET LOCALITY" } +func (*AlterTableLocality) StatementTag() string { return "ALTER TABLE" } func (*AlterTableLocality) hiddenFromShowQueries() {} @@ -446,7 +446,7 @@ func (*AlterTableOwner) StatementReturnType() StatementReturnType { return DDL } func (*AlterTableOwner) StatementType() StatementType { return TypeDCL } // StatementTag returns a short string identifying the type of statement. -func (*AlterTableOwner) StatementTag() string { return "ALTER TABLE OWNER" } +func (*AlterTableOwner) StatementTag() string { return "ALTER TABLE" } func (*AlterTableOwner) hiddenFromShowQueries() {} @@ -457,7 +457,17 @@ func (*AlterTableSetSchema) StatementReturnType() StatementReturnType { return D func (*AlterTableSetSchema) StatementType() StatementType { return TypeDDL } // StatementTag returns a short string identifying the type of statement. -func (*AlterTableSetSchema) StatementTag() string { return "ALTER TABLE SET SCHEMA" } +func (n *AlterTableSetSchema) StatementTag() string { + if n.IsView { + if n.IsMaterialized { + return "ALTER MATERIALIZED VIEW" + } + return "ALTER VIEW" + } else if n.IsSequence { + return "ALTER SEQUENCE" + } + return "ALTER TABLE" +} func (*AlterTableSetSchema) hiddenFromShowQueries() {} @@ -1266,7 +1276,7 @@ func (*ReparentDatabase) StatementReturnType() StatementReturnType { return DDL func (*ReparentDatabase) StatementType() StatementType { return TypeDDL } // StatementTag returns a short string identifying the type of statement. -func (*ReparentDatabase) StatementTag() string { return "CONVERT TO SCHEMA" } +func (*ReparentDatabase) StatementTag() string { return "ALTER DATABASE" } // StatementReturnType implements the Statement interface. func (*RenameIndex) StatementReturnType() StatementReturnType { return DDL } @@ -1286,6 +1296,9 @@ func (*RenameTable) StatementType() StatementType { return TypeDDL } // StatementTag returns a short string identifying the type of statement. func (n *RenameTable) StatementTag() string { if n.IsView { + if n.IsMaterialized { + return "ALTER MATERIALIZED VIEW" + } return "ALTER VIEW" } else if n.IsSequence { return "ALTER SEQUENCE" From 61921172a5868771b3c3c580dbf77332f6777ada Mon Sep 17 00:00:00 2001 From: Rafi Shamim Date: Sat, 17 Jun 2023 12:03:25 -0400 Subject: [PATCH 2/2] sql: add telemetry for schema changer mode This adds feature counter telemetry as well as log based telemetry to indicate which version of the schema changer was used. Release note: None --- docs/generated/eventlog.md | 1 + pkg/sql/exec_log.go | 1 + pkg/sql/instrumentation.go | 4 +++ pkg/sql/plan_opt.go | 11 +++++++ pkg/sql/schema_change_plan_node.go | 8 +++++ pkg/sql/schema_changer.go | 26 ++++++++++++++++ pkg/sql/sqltelemetry/schema.go | 8 +++++ pkg/sql/testdata/telemetry/schema | 31 ++++++++++++------- pkg/util/log/eventpb/json_encode_generated.go | 10 ++++++ pkg/util/log/eventpb/telemetry.proto | 6 +++- 10 files changed, 93 insertions(+), 13 deletions(-) diff --git a/docs/generated/eventlog.md b/docs/generated/eventlog.md index ea981633a906..3a20eea3c09f 100644 --- a/docs/generated/eventlog.md +++ b/docs/generated/eventlog.md @@ -2995,6 +2995,7 @@ contains common SQL event/execution details. | `MvccRangeKeyCount` | RangeKeyCount collects the count of range keys encountered during iteration. For details, see pebble.RangeKeyIteratorStats and https://github.com/cockroachdb/cockroach/blob/master/docs/tech-notes/mvcc-range-tombstones.md. | no | | `MvccRangeKeyContainedPoints` | RangeKeyContainedPoints collects the count of point keys encountered within the bounds of a range key. For details, see pebble.RangeKeyIteratorStats and https://github.com/cockroachdb/cockroach/blob/master/docs/tech-notes/mvcc-range-tombstones.md. | no | | `MvccRangeKeySkippedPoints` | RangeKeySkippedPoints collects the count of the subset of ContainedPoints point keys that were skipped during iteration due to range-key masking. For details, see pkg/storage/engine.go, pebble.RangeKeyIteratorStats, and https://github.com/cockroachdb/cockroach/blob/master/docs/tech-notes/mvcc-range-tombstones.md. | no | +| `SchemaChangerMode` | SchemaChangerMode is the mode that was used to execute the schema change, if any. | no | #### Common fields diff --git a/pkg/sql/exec_log.go b/pkg/sql/exec_log.go index 6da3e6717ceb..b3acfa0ad264 100644 --- a/pkg/sql/exec_log.go +++ b/pkg/sql/exec_log.go @@ -393,6 +393,7 @@ func (p *planner) maybeLogStatementInternal( MvccStepCountInternal: queryLevelStats.MvccStepsInternal, MvccStepCount: queryLevelStats.MvccSteps, MvccValueBytes: queryLevelStats.MvccValueBytes, + SchemaChangerMode: p.curPlan.instrumentation.schemaChangerMode.String(), } p.logOperationalEventsOnlyExternally(ctx, isCopy, &sampledQuery) diff --git a/pkg/sql/instrumentation.go b/pkg/sql/instrumentation.go index db70dbd7e062..77c1e133f386 100644 --- a/pkg/sql/instrumentation.go +++ b/pkg/sql/instrumentation.go @@ -201,6 +201,10 @@ type instrumentationHelper struct { // indexesUsed list the indexes used in the query with format tableID@indexID. indexesUsed []string + + // schemachangerMode indicates which schema changer mode was used to execute + // the query. + schemaChangerMode schemaChangerMode } // outputMode indicates how the statement output needs to be populated (for diff --git a/pkg/sql/plan_opt.go b/pkg/sql/plan_opt.go index 006724e175a2..d5bf0808886d 100644 --- a/pkg/sql/plan_opt.go +++ b/pkg/sql/plan_opt.go @@ -14,6 +14,7 @@ import ( "context" "strings" + "github.com/cockroachdb/cockroach/pkg/server/telemetry" "github.com/cockroachdb/cockroach/pkg/settings" "github.com/cockroachdb/cockroach/pkg/sql/catalog" "github.com/cockroachdb/cockroach/pkg/sql/catalog/colinfo" @@ -35,6 +36,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/sem/eval" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/sessiondatapb" + "github.com/cockroachdb/cockroach/pkg/sql/sqltelemetry" "github.com/cockroachdb/cockroach/pkg/util/errorutil" "github.com/cockroachdb/cockroach/pkg/util/log" "github.com/cockroachdb/cockroach/pkg/util/tracing" @@ -683,6 +685,15 @@ func (opc *optPlanningCtx) runExecBuilder( planTop.flags = opc.flags if bld.IsDDL { planTop.flags.Set(planFlagIsDDL) + + // The declarative schema changer mode would have already been set here, + // since all declarative schema changes are built opaquely. However, some + // DDLs (e.g. CREATE TABLE) are built non-opaquely, so we need to set the + // mode here if it wasn't already set. + if planTop.instrumentation.schemaChangerMode == schemaChangerModeNone { + telemetry.Inc(sqltelemetry.LegacySchemaChangerCounter) + planTop.instrumentation.schemaChangerMode = schemaChangerModeLegacy + } } if bld.ContainsFullTableScan { planTop.flags.Set(planFlagContainsFullTableScan) diff --git a/pkg/sql/schema_change_plan_node.go b/pkg/sql/schema_change_plan_node.go index 434850cdf2a4..b74cf815d090 100644 --- a/pkg/sql/schema_change_plan_node.go +++ b/pkg/sql/schema_change_plan_node.go @@ -19,6 +19,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/jobs/jobspb" "github.com/cockroachdb/cockroach/pkg/keys" "github.com/cockroachdb/cockroach/pkg/security/username" + "github.com/cockroachdb/cockroach/pkg/server/telemetry" "github.com/cockroachdb/cockroach/pkg/sql/catalog/catalogkeys" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descs" @@ -34,6 +35,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/sessiondata" "github.com/cockroachdb/cockroach/pkg/sql/sessiondatapb" + "github.com/cockroachdb/cockroach/pkg/sql/sqltelemetry" "github.com/cockroachdb/cockroach/pkg/util/log" "github.com/cockroachdb/cockroach/pkg/util/retry" "github.com/cockroachdb/cockroach/pkg/util/timeutil" @@ -81,6 +83,12 @@ func (p *planner) SchemaChange(ctx context.Context, stmt tree.Statement) (planNo } return nil, err } + + // If we successfully planned a schema change here, then update telemetry + // to indicate that we used the new schema changer. + telemetry.Inc(sqltelemetry.DeclarativeSchemaChangerCounter) + p.curPlan.instrumentation.schemaChangerMode = schemaChangerModeDeclarative + return &schemaChangePlanNode{ stmt: stmt, sql: p.stmt.SQL, diff --git a/pkg/sql/schema_changer.go b/pkg/sql/schema_changer.go index 17c07b272504..a1c1e997d3cf 100644 --- a/pkg/sql/schema_changer.go +++ b/pkg/sql/schema_changer.go @@ -75,6 +75,32 @@ var schemaChangeJobMaxRetryBackoff = settings.RegisterDurationSetting( settings.PositiveDuration, ) +type schemaChangerMode int + +const ( + // schemaChangerModeNone indicates that the schema changer was not used. + schemaChangerModeNone schemaChangerMode = iota + // schemaChangerModeLegacy indicates that the legacy schema changer was used. + schemaChangerModeLegacy + // schemaChangerModeDeclarative indicates that the declarative schema changer + // was used. + schemaChangerModeDeclarative +) + +// String returns a string representation of the schema changer mode. +func (m schemaChangerMode) String() string { + switch m { + case schemaChangerModeNone: + return "none" + case schemaChangerModeLegacy: + return "legacy" + case schemaChangerModeDeclarative: + return "declarative" + default: + return fmt.Sprintf("schemaChangerMode(%d)", m) + } +} + const ( // RunningStatusWaitingForMVCCGC is used for the GC job when it has cleared // the data but is waiting for MVCC GC to remove the data. diff --git a/pkg/sql/sqltelemetry/schema.go b/pkg/sql/sqltelemetry/schema.go index 04fc1c0c444e..a4f19b9962ea 100644 --- a/pkg/sql/sqltelemetry/schema.go +++ b/pkg/sql/sqltelemetry/schema.go @@ -195,3 +195,11 @@ func SetTableStorageParameter(param string) telemetry.Counter { func ResetTableStorageParameter(param string) telemetry.Counter { return telemetry.GetCounter("sql.schema.table_storage_parameter." + param + ".reset") } + +// DeclarativeSchemaChangerCounter is incremented whenever the declarative +// schema changer is used. +var DeclarativeSchemaChangerCounter = telemetry.GetCounterOnce("sql.schema.schema_changer_mode.declarative") + +// LegacySchemaChangerCounter is incremented whenever the legacy schema changer +// is used. +var LegacySchemaChangerCounter = telemetry.GetCounterOnce("sql.schema.schema_changer_mode.legacy") diff --git a/pkg/sql/testdata/telemetry/schema b/pkg/sql/testdata/telemetry/schema index ae1ea80757b6..968be5093bb0 100644 --- a/pkg/sql/testdata/telemetry/schema +++ b/pkg/sql/testdata/telemetry/schema @@ -46,6 +46,7 @@ CREATE TABLE y (a SERIAL2) sql.schema.create_table sql.schema.new_column.qualification.default_expr sql.schema.new_column_type.int8 +sql.schema.schema_changer_mode.legacy sql.schema.serial.rowid.int2 feature-usage @@ -57,6 +58,7 @@ sql.schema.alter_table.add_column.references sql.schema.alter_table.add_constraint sql.schema.get_virtual_table.pg_catalog.pg_attribute sql.schema.new_column_type.int8 +sql.schema.schema_changer_mode.declarative schema ---- @@ -78,27 +80,19 @@ table:_ ├── _:int default: _ └── _:int -feature-allowlist -sql.schema.create_unlogged_table ----- - feature-usage CREATE UNLOGGED TABLE unlogged_tbl(col int PRIMARY KEY) ---- +sql.schema.create_table sql.schema.create_unlogged_table - -feature-allowlist -sql.schema.create_or_replace_view ----- +sql.schema.new_column_type.int8 +sql.schema.schema_changer_mode.legacy feature-usage CREATE OR REPLACE VIEW cor_view AS SELECT 1 ---- sql.schema.create_or_replace_view - -feature-allowlist -sql.schema.* ----- +sql.schema.schema_changer_mode.legacy feature-usage CREATE TABLE on_update_t (a INT PRIMARY KEY, b INT8 DEFAULT 1 ON UPDATE 2) @@ -107,6 +101,7 @@ sql.schema.create_table sql.schema.new_column.qualification.default_expr sql.schema.new_column.qualification.on_update sql.schema.new_column_type.int8 +sql.schema.schema_changer_mode.legacy feature-usage ALTER TABLE on_update_t ADD COLUMN c INT DEFAULT 1 ON UPDATE 2; @@ -117,9 +112,21 @@ sql.schema.get_virtual_table.pg_catalog.pg_attribute sql.schema.new_column.qualification.default_expr sql.schema.new_column.qualification.on_update sql.schema.new_column_type.int8 +sql.schema.schema_changer_mode.declarative feature-usage ALTER TABLE on_update_t ALTER COLUMN b SET ON UPDATE 3 ---- sql.schema.alter_table sql.schema.alter_table.set_on_update +sql.schema.schema_changer_mode.legacy + +feature-usage +CREATE FUNCTION f() RETURNS INT AS $$ SELECT 1 $$ LANGUAGE SQL IMMUTABLE +---- +sql.schema.schema_changer_mode.declarative + +feature-usage +ALTER FUNCTION f() OWNER TO admin +---- +sql.schema.schema_changer_mode.legacy diff --git a/pkg/util/log/eventpb/json_encode_generated.go b/pkg/util/log/eventpb/json_encode_generated.go index b8db86f868f0..44a3fd6ad79d 100644 --- a/pkg/util/log/eventpb/json_encode_generated.go +++ b/pkg/util/log/eventpb/json_encode_generated.go @@ -4820,6 +4820,16 @@ func (m *SampledQuery) AppendJSONFields(printComma bool, b redact.RedactableByte b = strconv.AppendInt(b, int64(m.MvccRangeKeySkippedPoints), 10) } + if m.SchemaChangerMode != "" { + if printComma { + b = append(b, ',') + } + printComma = true + b = append(b, "\"SchemaChangerMode\":\""...) + b = redact.RedactableBytes(jsonbytes.EncodeString([]byte(b), string(m.SchemaChangerMode))) + b = append(b, '"') + } + return printComma, b } diff --git a/pkg/util/log/eventpb/telemetry.proto b/pkg/util/log/eventpb/telemetry.proto index 26bad242666b..b3088b8ad3fc 100644 --- a/pkg/util/log/eventpb/telemetry.proto +++ b/pkg/util/log/eventpb/telemetry.proto @@ -284,9 +284,13 @@ message SampledQuery { // https://github.com/cockroachdb/cockroach/blob/master/docs/tech-notes/mvcc-range-tombstones.md. int64 mvcc_range_key_skipped_points = 74 [(gogoproto.jsontag) = ",omitempty"]; + // SchemaChangerMode is the mode that was used to execute the schema change, + // if any. + string schema_changer_mode = 76 [(gogoproto.jsontag) = ',omitempty', (gogoproto.moretags) = "redact:\"nonsensitive\""]; + reserved 12; - // Next available ID: 76. + // Next available ID: 77. }