From 5ff11deeee5bd44345604fe8cd30594318f8efd2 Mon Sep 17 00:00:00 2001 From: Andrew Werner Date: Mon, 15 Aug 2022 22:04:25 -0400 Subject: [PATCH] sql/schemachanger: add ability to run pre-checks on alter table commands This way we can introspect the AST prior to deciding that we need to go resolve the descriptor. This allows us to claw back some round-trips in cases where we do not support the relevant feature. Along the way, the change also refactors some of the supported statement code just a tad. Release note: None --- .../testdata/benchmark_expectations | 6 +- .../internal/scbuildstmt/alter_table.go | 55 +++++++++++++++++-- .../scbuildstmt/alter_table_add_constraint.go | 8 +-- .../scbuild/internal/scbuildstmt/process.go | 48 ++++++++-------- 4 files changed, 82 insertions(+), 35 deletions(-) diff --git a/pkg/bench/rttanalysis/testdata/benchmark_expectations b/pkg/bench/rttanalysis/testdata/benchmark_expectations index 609a97b4ccea..c982902bd868 100644 --- a/pkg/bench/rttanalysis/testdata/benchmark_expectations +++ b/pkg/bench/rttanalysis/testdata/benchmark_expectations @@ -2,9 +2,9 @@ exp,benchmark 12,AlterRole/alter_role_with_1_option 15,AlterRole/alter_role_with_2_options 19,AlterRole/alter_role_with_3_options -15,AlterTableAddCheckConstraint/alter_table_add_1_check_constraint -15,AlterTableAddCheckConstraint/alter_table_add_2_check_constraints -15,AlterTableAddCheckConstraint/alter_table_add_3_check_constraints +13,AlterTableAddCheckConstraint/alter_table_add_1_check_constraint +13,AlterTableAddCheckConstraint/alter_table_add_2_check_constraints +13,AlterTableAddCheckConstraint/alter_table_add_3_check_constraints 18,AlterTableAddColumn/alter_table_add_1_column 18,AlterTableAddColumn/alter_table_add_2_columns 18,AlterTableAddColumn/alter_table_add_3_columns diff --git a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table.go b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table.go index 5cd636797d2d..5457f220ddd7 100644 --- a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table.go +++ b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table.go @@ -24,17 +24,37 @@ import ( "github.com/cockroachdb/errors" ) +// supportedAlterTableCommand tracks metadata for ALTER TABLE commands that are +// implemented by the new schema changer. +type supportedAlterTableCommand struct { + // fn is a function to perform a schema change. + fn interface{} + // on indicates that this statement is on by default. + on bool + // extraChecks contains a function to determine whether the command is + // supported. These extraChecks are important to be able to avoid any + // extra round-trips to resolve the descriptor and its elements if we know + // that we cannot process the command. + extraChecks interface{} +} + // supportedAlterTableStatements tracks alter table operations fully supported by // declarative schema changer. Operations marked as non-fully supported can // only be with the use_declarative_schema_changer session variable. -var supportedAlterTableStatements = map[reflect.Type]supportedStatement{ - reflect.TypeOf((*tree.AlterTableAddColumn)(nil)): {alterTableAddColumn, true}, - reflect.TypeOf((*tree.AlterTableDropColumn)(nil)): {alterTableDropColumn, true}, - reflect.TypeOf((*tree.AlterTableAlterPrimaryKey)(nil)): {alterTableAlterPrimaryKey, true}, - reflect.TypeOf((*tree.AlterTableAddConstraint)(nil)): {alterTableAddConstraint, true}, +var supportedAlterTableStatements = map[reflect.Type]supportedAlterTableCommand{ + reflect.TypeOf((*tree.AlterTableAddColumn)(nil)): {fn: alterTableAddColumn, on: true}, + reflect.TypeOf((*tree.AlterTableDropColumn)(nil)): {fn: alterTableDropColumn, on: true}, + reflect.TypeOf((*tree.AlterTableAlterPrimaryKey)(nil)): {fn: alterTableAlterPrimaryKey, on: true}, + reflect.TypeOf((*tree.AlterTableAddConstraint)(nil)): {fn: alterTableAddConstraint, on: true, extraChecks: func( + t *tree.AlterTableAddConstraint, + ) bool { + d, ok := t.ConstraintDef.(*tree.UniqueConstraintTableDef) + return ok && d.PrimaryKey && t.ValidationBehavior == tree.ValidationDefault + }}, } func init() { + boolType := reflect.TypeOf((*bool)(nil)).Elem() // Check function signatures inside the supportedAlterTableStatements map. for statementType, statementEntry := range supportedAlterTableStatements { callBackType := reflect.TypeOf(statementEntry.fn) @@ -50,6 +70,20 @@ func init() { panic(errors.AssertionFailedf("%v entry for alter table statement "+ "does not have a valid signature got %v", statementType, callBackType)) } + if statementEntry.extraChecks != nil { + extraChecks := reflect.TypeOf(statementEntry.extraChecks) + if extraChecks.Kind() != reflect.Func { + panic(errors.AssertionFailedf("%v extra checks for statement is "+ + "not a function", statementType)) + } + if extraChecks.NumIn() != 1 || + extraChecks.In(0) != statementType || + extraChecks.NumOut() != 1 || + extraChecks.Out(0) != boolType { + panic(errors.AssertionFailedf("%v extra checks for alter table statement "+ + "does not have a valid signature got %v", statementType, callBackType)) + } + } } } @@ -72,7 +106,16 @@ func AlterTable(b BuildCtx, n *tree.AlterTable) { // Check if partially supported operations are allowed next. If an // operation is not fully supported will not allow it to be run in // the declarative schema changer until its fully supported. - if !info.IsFullySupported(b.EvalCtx().SessionData().NewSchemaChangerMode) { + if !isFullySupported( + info.on, b.EvalCtx().SessionData().NewSchemaChangerMode, + ) { + panic(scerrors.NotImplementedError(cmd)) + } + + // Run the extraChecks to see if we should avoid even resolving the + // descriptor. + if info.extraChecks != nil && !reflect.ValueOf(info.extraChecks). + Call([]reflect.Value{reflect.ValueOf(cmd)})[0].Bool() { panic(scerrors.NotImplementedError(cmd)) } } diff --git a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_add_constraint.go b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_add_constraint.go index dc481b92c716..8209dbb62fd1 100644 --- a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_add_constraint.go +++ b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_add_constraint.go @@ -19,10 +19,10 @@ import ( func alterTableAddConstraint( b BuildCtx, tn *tree.TableName, tbl *scpb.Table, t *tree.AlterTableAddConstraint, ) { - d, ok := t.ConstraintDef.(*tree.UniqueConstraintTableDef) - if !ok || !d.PrimaryKey || t.ValidationBehavior != tree.ValidationDefault { - panic(scerrors.NotImplementedError(t)) - } + // Extra checks before reaching this point ensured that the command is + // an ALTER TABLE ... ADD PRIMARY KEY command. + d := t.ConstraintDef.(*tree.UniqueConstraintTableDef) + // Ensure that there is a default rowid column. oldPrimaryIndex := mustRetrievePrimaryIndexElement(b, tbl.TableID) if getPrimaryIndexDefaultRowIDColumn( diff --git a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/process.go b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/process.go index d70466a0919a..5f4008e6cd47 100644 --- a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/process.go +++ b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/process.go @@ -22,21 +22,23 @@ import ( // supportedStatement tracks metadata for statements that are // implemented by the new schema changer. type supportedStatement struct { - fn interface{} - fullySupported bool + // fn is a function to perform a schema change. + fn interface{} + // on indicates that this statement is on by default. + on bool } -// IsFullySupported returns if this statement type is supported, where the +// isFullySupported returns if this statement type is supported, where the // mode of the new schema changer can force unsupported statements to be // supported. -func (s supportedStatement) IsFullySupported(mode sessiondatapb.NewSchemaChangerMode) bool { +func isFullySupported(onByDefault bool, mode sessiondatapb.NewSchemaChangerMode) bool { // If the unsafe modes of the new schema changer are used then any implemented // operation will be exposed. if mode == sessiondatapb.UseNewSchemaChangerUnsafeAlways || mode == sessiondatapb.UseNewSchemaChangerUnsafe { return true } - return s.fullySupported + return onByDefault } // Tracks operations which are fully supported when the declarative schema @@ -46,23 +48,23 @@ var supportedStatements = map[reflect.Type]supportedStatement{ // Alter table will have commands individually whitelisted via the // supportedAlterTableStatements list, so wwe will consider it fully supported // here. - reflect.TypeOf((*tree.AlterTable)(nil)): {AlterTable, true}, - reflect.TypeOf((*tree.CreateIndex)(nil)): {CreateIndex, false}, - reflect.TypeOf((*tree.DropDatabase)(nil)): {DropDatabase, true}, - reflect.TypeOf((*tree.DropOwnedBy)(nil)): {DropOwnedBy, true}, - reflect.TypeOf((*tree.DropSchema)(nil)): {DropSchema, true}, - reflect.TypeOf((*tree.DropSequence)(nil)): {DropSequence, true}, - reflect.TypeOf((*tree.DropTable)(nil)): {DropTable, true}, - reflect.TypeOf((*tree.DropType)(nil)): {DropType, true}, - reflect.TypeOf((*tree.DropView)(nil)): {DropView, true}, - reflect.TypeOf((*tree.CommentOnDatabase)(nil)): {CommentOnDatabase, true}, - reflect.TypeOf((*tree.CommentOnSchema)(nil)): {CommentOnSchema, true}, - reflect.TypeOf((*tree.CommentOnTable)(nil)): {CommentOnTable, true}, - reflect.TypeOf((*tree.CommentOnColumn)(nil)): {CommentOnColumn, true}, - reflect.TypeOf((*tree.CommentOnIndex)(nil)): {CommentOnIndex, true}, - reflect.TypeOf((*tree.CommentOnConstraint)(nil)): {CommentOnConstraint, true}, + reflect.TypeOf((*tree.AlterTable)(nil)): {fn: AlterTable, on: true}, + reflect.TypeOf((*tree.CreateIndex)(nil)): {fn: CreateIndex, on: false}, + reflect.TypeOf((*tree.DropDatabase)(nil)): {fn: DropDatabase, on: true}, + reflect.TypeOf((*tree.DropOwnedBy)(nil)): {fn: DropOwnedBy, on: true}, + reflect.TypeOf((*tree.DropSchema)(nil)): {fn: DropSchema, on: true}, + reflect.TypeOf((*tree.DropSequence)(nil)): {fn: DropSequence, on: true}, + reflect.TypeOf((*tree.DropTable)(nil)): {fn: DropTable, on: true}, + reflect.TypeOf((*tree.DropType)(nil)): {fn: DropType, on: true}, + reflect.TypeOf((*tree.DropView)(nil)): {fn: DropView, on: true}, + reflect.TypeOf((*tree.CommentOnDatabase)(nil)): {fn: CommentOnDatabase, on: true}, + reflect.TypeOf((*tree.CommentOnSchema)(nil)): {fn: CommentOnSchema, on: true}, + reflect.TypeOf((*tree.CommentOnTable)(nil)): {fn: CommentOnTable, on: true}, + reflect.TypeOf((*tree.CommentOnColumn)(nil)): {fn: CommentOnColumn, on: true}, + reflect.TypeOf((*tree.CommentOnIndex)(nil)): {fn: CommentOnIndex, on: true}, + reflect.TypeOf((*tree.CommentOnConstraint)(nil)): {fn: CommentOnConstraint, on: true}, // TODO (Xiang): turn on `DROP INDEX` as fully supported. - reflect.TypeOf((*tree.DropIndex)(nil)): {DropIndex, false}, + reflect.TypeOf((*tree.DropIndex)(nil)): {fn: DropIndex, on: false}, } func init() { @@ -94,7 +96,9 @@ func Process(b BuildCtx, n tree.Statement) { // Check if partially supported operations are allowed next. If an // operation is not fully supported will not allow it to be run in // the declarative schema changer until its fully supported. - if !info.IsFullySupported(b.EvalCtx().SessionData().NewSchemaChangerMode) { + if !isFullySupported( + info.on, b.EvalCtx().SessionData().NewSchemaChangerMode, + ) { panic(scerrors.NotImplementedError(n)) } // Next invoke the callback function, with the concrete types.