Skip to content

Commit

Permalink
sql/schemachanger: add ability to run pre-checks on alter table commands
Browse files Browse the repository at this point in the history
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
ajwerner committed Aug 16, 2022

Verified

This commit was signed with the committer’s verified signature.
matthewnessworthy Matthew Nessworthy
1 parent f9ac069 commit 5ff11de
Showing 4 changed files with 82 additions and 35 deletions.
6 changes: 3 additions & 3 deletions pkg/bench/rttanalysis/testdata/benchmark_expectations
Original file line number Diff line number Diff line change
@@ -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
55 changes: 49 additions & 6 deletions pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table.go
Original file line number Diff line number Diff line change
@@ -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))
}
}
Original file line number Diff line number Diff line change
@@ -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(
48 changes: 26 additions & 22 deletions pkg/sql/schemachanger/scbuild/internal/scbuildstmt/process.go
Original file line number Diff line number Diff line change
@@ -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.

0 comments on commit 5ff11de

Please sign in to comment.