From 6133fbc0e6220199fcd94e4deb942c263f4c3665 Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Thu, 29 Aug 2024 16:15:27 +0300 Subject: [PATCH 1/4] schemadiff: new NonDeterministicDefaultStrategy, can reject non-deterministic function in column's default value Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- go/vt/schemadiff/errors.go | 10 +++ go/vt/schemadiff/table.go | 45 ++++++++++++ go/vt/schemadiff/table_test.go | 124 ++++++++++++++++++++++++++++----- go/vt/schemadiff/types.go | 32 +++++---- 4 files changed, 179 insertions(+), 32 deletions(-) diff --git a/go/vt/schemadiff/errors.go b/go/vt/schemadiff/errors.go index a941c406be0..d25a21c6c0f 100644 --- a/go/vt/schemadiff/errors.go +++ b/go/vt/schemadiff/errors.go @@ -487,3 +487,13 @@ type PartitionSpecNonExclusiveError struct { func (e *PartitionSpecNonExclusiveError) Error() string { return fmt.Sprintf("ALTER TABLE on %s, may only have a single partition spec change, and other changes are not allowed. Found spec: %s; and change: %s", sqlescape.EscapeID(e.Table), sqlparser.CanonicalString(e.PartitionSpec), e.ConflictingStatement) } + +type NonDeterministicDefaultError struct { + Table string + Column string + Function string +} + +func (e *NonDeterministicDefaultError) Error() string { + return fmt.Sprintf("column %s.%s default value uses non-deterministic function: %v", sqlescape.EscapeID(e.Table), sqlescape.EscapeID(e.Column), e.Function) +} diff --git a/go/vt/schemadiff/table.go b/go/vt/schemadiff/table.go index c326b2763b3..8cd0169d261 100644 --- a/go/vt/schemadiff/table.go +++ b/go/vt/schemadiff/table.go @@ -1731,6 +1731,40 @@ func evaluateColumnReordering(t1SharedColumns, t2SharedColumns []*sqlparser.Colu return minimalColumnReordering } +// This function looks for a non-deterministic function call in the given expression. +// If recurses into all function arguments. +// The known non-deterministic function we handle are: +// - UUID() +// - RAND() +// - SYSDATE() +func findNoNondeterministicFunction(expr sqlparser.Expr) (foundFunction string) { + _ = sqlparser.Walk(func(node sqlparser.SQLNode) (kontinue bool, err error) { + switch node := node.(type) { + case *sqlparser.CurTimeFuncExpr: + switch node.Name.Lowered() { + case "sysdate": + foundFunction = node.Name.String() + return false, nil + } + case *sqlparser.FuncExpr: + switch node.Name.Lowered() { + case "uuid", "rand": + foundFunction = node.Name.String() + return false, nil + default: + // recurse into function argument expressions + for _, exprArg := range node.Exprs { + if foundFunction = findNoNondeterministicFunction(exprArg); foundFunction != "" { + return false, nil + } + } + } + } + return true, nil + }, expr) + return foundFunction +} + // Diff compares this table statement with another table statement, and sees what it takes to // change this table to look like the other table. // It returns an AlterTable statement if changes are found, or nil if not. @@ -1852,6 +1886,17 @@ func (c *CreateTableEntity) diffColumns(alterTable *sqlparser.AlterTable, addColumn := &sqlparser.AddColumns{ Columns: []*sqlparser.ColumnDefinition{t2Col}, } + if hints.NonDeterministicDefaultStrategy == NonDeterministicDefaultReject { + if t2Col.Type.Options.Default != nil && !t2Col.Type.Options.DefaultLiteral { + if function := findNoNondeterministicFunction(t2Col.Type.Options.Default); function != "" { + return &NonDeterministicDefaultError{ + Table: c.Name(), + Column: t2Col.Name.String(), + Function: function, + } + } + } + } if t2ColIndex < expectAppendIndex { // This column is added somewhere in between existing columns, not appended at end of column list if t2ColIndex == 0 { diff --git a/go/vt/schemadiff/table_test.go b/go/vt/schemadiff/table_test.go index 389e55f447c..71e3a72853e 100644 --- a/go/vt/schemadiff/table_test.go +++ b/go/vt/schemadiff/table_test.go @@ -28,25 +28,28 @@ import ( func TestCreateTableDiff(t *testing.T) { tt := []struct { - name string - from string - to string - fromName string - toName string - diff string - diffs []string - cdiff string - cdiffs []string - errorMsg string - autoinc int - rotation int - fulltext int - colrename int - constraint int - charset int - algorithm int - enumreorder int - subsequent int + name string + from string + to string + fromName string + toName string + diff string + diffs []string + cdiff string + cdiffs []string + errorMsg string + // hints: + autoinc int + rotation int + fulltext int + colrename int + constraint int + charset int + algorithm int + enumreorder int + subsequent int + nondeterministic int + // textdiffs []string atomicdiffs []string }{ @@ -449,6 +452,88 @@ func TestCreateTableDiff(t *testing.T) { "+ `y` int,", }, }, + { + name: "added column with non deterministic expression, allow", + from: "create table t1 (id int primary key, a int)", + to: "create table t2 (id int primary key, a int, v varchar(36) not null default (uuid()))", + diff: "alter table t1 add column v varchar(36) not null default (uuid())", + cdiff: "ALTER TABLE `t1` ADD COLUMN `v` varchar(36) NOT NULL DEFAULT (uuid())", + textdiffs: []string{ + "+ `v` varchar(36) NOT NULL DEFAULT (uuid()),", + }, + nondeterministic: NonDeterministicDefaultAllow, + }, + { + name: "added column with non deterministic expression, uuid, reject", + from: "create table t1 (id int primary key, a int)", + to: "create table t2 (id int primary key, a int, v varchar(36) not null default (uuid()))", + nondeterministic: NonDeterministicDefaultReject, + errorMsg: (&NonDeterministicDefaultError{Table: "t1", Column: "v", Function: "uuid"}).Error(), + }, + { + name: "added column with non deterministic expression, UUID, reject", + from: "create table t1 (id int primary key, a int)", + to: "create table t2 (id int primary key, a int, v varchar(36) not null default (UUID()))", + nondeterministic: NonDeterministicDefaultReject, + errorMsg: (&NonDeterministicDefaultError{Table: "t1", Column: "v", Function: "UUID"}).Error(), + }, + { + name: "added column with non deterministic expression, uuid, spacing, reject", + from: "create table t1 (id int primary key, a int)", + to: "create table t2 (id int primary key, a int, v varchar(36) not null default (uuid ()))", + nondeterministic: NonDeterministicDefaultReject, + errorMsg: (&NonDeterministicDefaultError{Table: "t1", Column: "v", Function: "uuid"}).Error(), + }, + { + name: "added column with non deterministic expression, uuid, inner, reject", + from: "create table t1 (id int primary key, a int)", + to: "create table t2 (id int primary key, a int, v varchar(36) not null default (left(uuid(),10)))", + nondeterministic: NonDeterministicDefaultReject, + errorMsg: (&NonDeterministicDefaultError{Table: "t1", Column: "v", Function: "uuid"}).Error(), + }, + { + name: "added column with non deterministic expression, rand, reject", + from: "create table t1 (id int primary key, a int)", + to: "create table t2 (id int primary key, a int, v varchar(36) not null default (2.0 + rand()))", + nondeterministic: NonDeterministicDefaultReject, + errorMsg: (&NonDeterministicDefaultError{Table: "t1", Column: "v", Function: "rand"}).Error(), + }, + { + name: "added column with non deterministic expression, sysdate, reject", + from: "create table t1 (id int primary key, a int)", + to: "create table t2 (id int primary key, a int, v varchar(36) not null default (sysdate()))", + nondeterministic: NonDeterministicDefaultReject, + errorMsg: (&NonDeterministicDefaultError{Table: "t1", Column: "v", Function: "sysdate"}).Error(), + }, + { + name: "added column with non deterministic expression, sysdate, reject", + from: "create table t1 (id int primary key, a int)", + to: "create table t2 (id int primary key, a int, v varchar(36) not null default (to_days(sysdate())))", + nondeterministic: NonDeterministicDefaultReject, + errorMsg: (&NonDeterministicDefaultError{Table: "t1", Column: "v", Function: "sysdate"}).Error(), + }, + { + name: "added column with deterministic expression, now, reject does not apply", + from: "create table t1 (id int primary key, a int)", + to: "create table t2 (id int primary key, a int, v varchar(36) not null default (now()))", + diff: "alter table t1 add column v varchar(36) not null default (now())", + cdiff: "ALTER TABLE `t1` ADD COLUMN `v` varchar(36) NOT NULL DEFAULT (now())", + textdiffs: []string{ + "+ `v` varchar(36) NOT NULL DEFAULT (now()),", + }, + nondeterministic: NonDeterministicDefaultReject, + }, + { + name: "added column with deterministic expression, curdate, reject does not apply", + from: "create table t1 (id int primary key, a int)", + to: "create table t2 (id int primary key, a int, v varchar(36) not null default (to_days(curdate())))", + diff: "alter table t1 add column v varchar(36) not null default (to_days(curdate()))", + cdiff: "ALTER TABLE `t1` ADD COLUMN `v` varchar(36) NOT NULL DEFAULT (to_days(curdate()))", + textdiffs: []string{ + "+ `v` varchar(36) NOT NULL DEFAULT (to_days(curdate())),", + }, + nondeterministic: NonDeterministicDefaultReject, + }, // enum { name: "expand enum", @@ -2090,6 +2175,7 @@ func TestCreateTableDiff(t *testing.T) { hints.AlterTableAlgorithmStrategy = ts.algorithm hints.EnumReorderStrategy = ts.enumreorder hints.SubsequentDiffStrategy = ts.subsequent + hints.NonDeterministicDefaultStrategy = ts.nondeterministic alter, err := c.Diff(other, &hints) require.Equal(t, len(ts.diffs), len(ts.cdiffs)) diff --git a/go/vt/schemadiff/types.go b/go/vt/schemadiff/types.go index cb6bc09df85..3e6a86446ff 100644 --- a/go/vt/schemadiff/types.go +++ b/go/vt/schemadiff/types.go @@ -141,21 +141,27 @@ const ( SubsequentDiffStrategyReject ) +const ( + NonDeterministicDefaultAllow int = iota + NonDeterministicDefaultReject +) + // DiffHints is an assortment of rules for diffing entities type DiffHints struct { - StrictIndexOrdering bool - AutoIncrementStrategy int - RangeRotationStrategy int - ConstraintNamesStrategy int - ColumnRenameStrategy int - TableRenameStrategy int - FullTextKeyStrategy int - TableCharsetCollateStrategy int - TableQualifierHint int - AlterTableAlgorithmStrategy int - EnumReorderStrategy int - ForeignKeyCheckStrategy int - SubsequentDiffStrategy int + StrictIndexOrdering bool + AutoIncrementStrategy int + RangeRotationStrategy int + ConstraintNamesStrategy int + ColumnRenameStrategy int + TableRenameStrategy int + FullTextKeyStrategy int + TableCharsetCollateStrategy int + TableQualifierHint int + AlterTableAlgorithmStrategy int + EnumReorderStrategy int + ForeignKeyCheckStrategy int + SubsequentDiffStrategy int + NonDeterministicDefaultStrategy int } func EmptyDiffHints() *DiffHints { From d588cd8b36ae4900a05264d1e3a8da757ee7ef72 Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Mon, 2 Sep 2024 08:12:36 +0300 Subject: [PATCH 2/4] recursion is not needed Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- go/vt/schemadiff/table.go | 7 ------- 1 file changed, 7 deletions(-) diff --git a/go/vt/schemadiff/table.go b/go/vt/schemadiff/table.go index 8cd0169d261..a344285a34a 100644 --- a/go/vt/schemadiff/table.go +++ b/go/vt/schemadiff/table.go @@ -1751,13 +1751,6 @@ func findNoNondeterministicFunction(expr sqlparser.Expr) (foundFunction string) case "uuid", "rand": foundFunction = node.Name.String() return false, nil - default: - // recurse into function argument expressions - for _, exprArg := range node.Exprs { - if foundFunction = findNoNondeterministicFunction(exprArg); foundFunction != "" { - return false, nil - } - } } } return true, nil From 88895d74660d44c7635cc9f495313568014a2972 Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Mon, 2 Sep 2024 08:12:49 +0300 Subject: [PATCH 3/4] formatting directive Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- go/vt/schemadiff/errors.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go/vt/schemadiff/errors.go b/go/vt/schemadiff/errors.go index d25a21c6c0f..c938e736206 100644 --- a/go/vt/schemadiff/errors.go +++ b/go/vt/schemadiff/errors.go @@ -495,5 +495,5 @@ type NonDeterministicDefaultError struct { } func (e *NonDeterministicDefaultError) Error() string { - return fmt.Sprintf("column %s.%s default value uses non-deterministic function: %v", sqlescape.EscapeID(e.Table), sqlescape.EscapeID(e.Column), e.Function) + return fmt.Sprintf("column %s.%s default value uses non-deterministic function: %s", sqlescape.EscapeID(e.Table), sqlescape.EscapeID(e.Column), e.Function) } From 2cd96bc000bf5429846439e672c4348b36bdc66f Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Mon, 2 Sep 2024 08:46:56 +0300 Subject: [PATCH 4/4] Removing NonDeterministicDefaultStrategy. It is non-optional as MySQL's behavior reliably rejects adding columns with nondeterministic functions, even in the case of empty tables Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- go/vt/schemadiff/table.go | 15 +++--- go/vt/schemadiff/table_test.go | 96 +++++++++++++--------------------- go/vt/schemadiff/types.go | 32 +++++------- 3 files changed, 57 insertions(+), 86 deletions(-) diff --git a/go/vt/schemadiff/table.go b/go/vt/schemadiff/table.go index a344285a34a..d73259523b5 100644 --- a/go/vt/schemadiff/table.go +++ b/go/vt/schemadiff/table.go @@ -1879,14 +1879,13 @@ func (c *CreateTableEntity) diffColumns(alterTable *sqlparser.AlterTable, addColumn := &sqlparser.AddColumns{ Columns: []*sqlparser.ColumnDefinition{t2Col}, } - if hints.NonDeterministicDefaultStrategy == NonDeterministicDefaultReject { - if t2Col.Type.Options.Default != nil && !t2Col.Type.Options.DefaultLiteral { - if function := findNoNondeterministicFunction(t2Col.Type.Options.Default); function != "" { - return &NonDeterministicDefaultError{ - Table: c.Name(), - Column: t2Col.Name.String(), - Function: function, - } + // See whether this ADD COLUMN has a non-deterministic default value + if t2Col.Type.Options.Default != nil && !t2Col.Type.Options.DefaultLiteral { + if function := findNoNondeterministicFunction(t2Col.Type.Options.Default); function != "" { + return &NonDeterministicDefaultError{ + Table: c.Name(), + Column: t2Col.Name.String(), + Function: function, } } } diff --git a/go/vt/schemadiff/table_test.go b/go/vt/schemadiff/table_test.go index 71e3a72853e..c511343b1d6 100644 --- a/go/vt/schemadiff/table_test.go +++ b/go/vt/schemadiff/table_test.go @@ -39,16 +39,15 @@ func TestCreateTableDiff(t *testing.T) { cdiffs []string errorMsg string // hints: - autoinc int - rotation int - fulltext int - colrename int - constraint int - charset int - algorithm int - enumreorder int - subsequent int - nondeterministic int + autoinc int + rotation int + fulltext int + colrename int + constraint int + charset int + algorithm int + enumreorder int + subsequent int // textdiffs []string atomicdiffs []string @@ -453,64 +452,46 @@ func TestCreateTableDiff(t *testing.T) { }, }, { - name: "added column with non deterministic expression, allow", - from: "create table t1 (id int primary key, a int)", - to: "create table t2 (id int primary key, a int, v varchar(36) not null default (uuid()))", - diff: "alter table t1 add column v varchar(36) not null default (uuid())", - cdiff: "ALTER TABLE `t1` ADD COLUMN `v` varchar(36) NOT NULL DEFAULT (uuid())", - textdiffs: []string{ - "+ `v` varchar(36) NOT NULL DEFAULT (uuid()),", - }, - nondeterministic: NonDeterministicDefaultAllow, - }, - { - name: "added column with non deterministic expression, uuid, reject", - from: "create table t1 (id int primary key, a int)", - to: "create table t2 (id int primary key, a int, v varchar(36) not null default (uuid()))", - nondeterministic: NonDeterministicDefaultReject, - errorMsg: (&NonDeterministicDefaultError{Table: "t1", Column: "v", Function: "uuid"}).Error(), + name: "added column with non deterministic expression, uuid, reject", + from: "create table t1 (id int primary key, a int)", + to: "create table t2 (id int primary key, a int, v varchar(36) not null default (uuid()))", + errorMsg: (&NonDeterministicDefaultError{Table: "t1", Column: "v", Function: "uuid"}).Error(), }, { - name: "added column with non deterministic expression, UUID, reject", - from: "create table t1 (id int primary key, a int)", - to: "create table t2 (id int primary key, a int, v varchar(36) not null default (UUID()))", - nondeterministic: NonDeterministicDefaultReject, - errorMsg: (&NonDeterministicDefaultError{Table: "t1", Column: "v", Function: "UUID"}).Error(), + name: "added column with non deterministic expression, UUID, reject", + from: "create table t1 (id int primary key, a int)", + to: "create table t2 (id int primary key, a int, v varchar(36) not null default (UUID()))", + errorMsg: (&NonDeterministicDefaultError{Table: "t1", Column: "v", Function: "UUID"}).Error(), }, { - name: "added column with non deterministic expression, uuid, spacing, reject", - from: "create table t1 (id int primary key, a int)", - to: "create table t2 (id int primary key, a int, v varchar(36) not null default (uuid ()))", - nondeterministic: NonDeterministicDefaultReject, - errorMsg: (&NonDeterministicDefaultError{Table: "t1", Column: "v", Function: "uuid"}).Error(), + name: "added column with non deterministic expression, uuid, spacing, reject", + from: "create table t1 (id int primary key, a int)", + to: "create table t2 (id int primary key, a int, v varchar(36) not null default (uuid ()))", + errorMsg: (&NonDeterministicDefaultError{Table: "t1", Column: "v", Function: "uuid"}).Error(), }, { - name: "added column with non deterministic expression, uuid, inner, reject", - from: "create table t1 (id int primary key, a int)", - to: "create table t2 (id int primary key, a int, v varchar(36) not null default (left(uuid(),10)))", - nondeterministic: NonDeterministicDefaultReject, - errorMsg: (&NonDeterministicDefaultError{Table: "t1", Column: "v", Function: "uuid"}).Error(), + name: "added column with non deterministic expression, uuid, inner, reject", + from: "create table t1 (id int primary key, a int)", + to: "create table t2 (id int primary key, a int, v varchar(36) not null default (left(uuid(),10)))", + errorMsg: (&NonDeterministicDefaultError{Table: "t1", Column: "v", Function: "uuid"}).Error(), }, { - name: "added column with non deterministic expression, rand, reject", - from: "create table t1 (id int primary key, a int)", - to: "create table t2 (id int primary key, a int, v varchar(36) not null default (2.0 + rand()))", - nondeterministic: NonDeterministicDefaultReject, - errorMsg: (&NonDeterministicDefaultError{Table: "t1", Column: "v", Function: "rand"}).Error(), + name: "added column with non deterministic expression, rand, reject", + from: "create table t1 (id int primary key, a int)", + to: "create table t2 (id int primary key, a int, v varchar(36) not null default (2.0 + rand()))", + errorMsg: (&NonDeterministicDefaultError{Table: "t1", Column: "v", Function: "rand"}).Error(), }, { - name: "added column with non deterministic expression, sysdate, reject", - from: "create table t1 (id int primary key, a int)", - to: "create table t2 (id int primary key, a int, v varchar(36) not null default (sysdate()))", - nondeterministic: NonDeterministicDefaultReject, - errorMsg: (&NonDeterministicDefaultError{Table: "t1", Column: "v", Function: "sysdate"}).Error(), + name: "added column with non deterministic expression, sysdate, reject", + from: "create table t1 (id int primary key, a int)", + to: "create table t2 (id int primary key, a int, v varchar(36) not null default (sysdate()))", + errorMsg: (&NonDeterministicDefaultError{Table: "t1", Column: "v", Function: "sysdate"}).Error(), }, { - name: "added column with non deterministic expression, sysdate, reject", - from: "create table t1 (id int primary key, a int)", - to: "create table t2 (id int primary key, a int, v varchar(36) not null default (to_days(sysdate())))", - nondeterministic: NonDeterministicDefaultReject, - errorMsg: (&NonDeterministicDefaultError{Table: "t1", Column: "v", Function: "sysdate"}).Error(), + name: "added column with non deterministic expression, sysdate, reject", + from: "create table t1 (id int primary key, a int)", + to: "create table t2 (id int primary key, a int, v varchar(36) not null default (to_days(sysdate())))", + errorMsg: (&NonDeterministicDefaultError{Table: "t1", Column: "v", Function: "sysdate"}).Error(), }, { name: "added column with deterministic expression, now, reject does not apply", @@ -521,7 +502,6 @@ func TestCreateTableDiff(t *testing.T) { textdiffs: []string{ "+ `v` varchar(36) NOT NULL DEFAULT (now()),", }, - nondeterministic: NonDeterministicDefaultReject, }, { name: "added column with deterministic expression, curdate, reject does not apply", @@ -532,7 +512,6 @@ func TestCreateTableDiff(t *testing.T) { textdiffs: []string{ "+ `v` varchar(36) NOT NULL DEFAULT (to_days(curdate())),", }, - nondeterministic: NonDeterministicDefaultReject, }, // enum { @@ -2175,7 +2154,6 @@ func TestCreateTableDiff(t *testing.T) { hints.AlterTableAlgorithmStrategy = ts.algorithm hints.EnumReorderStrategy = ts.enumreorder hints.SubsequentDiffStrategy = ts.subsequent - hints.NonDeterministicDefaultStrategy = ts.nondeterministic alter, err := c.Diff(other, &hints) require.Equal(t, len(ts.diffs), len(ts.cdiffs)) diff --git a/go/vt/schemadiff/types.go b/go/vt/schemadiff/types.go index 3e6a86446ff..cb6bc09df85 100644 --- a/go/vt/schemadiff/types.go +++ b/go/vt/schemadiff/types.go @@ -141,27 +141,21 @@ const ( SubsequentDiffStrategyReject ) -const ( - NonDeterministicDefaultAllow int = iota - NonDeterministicDefaultReject -) - // DiffHints is an assortment of rules for diffing entities type DiffHints struct { - StrictIndexOrdering bool - AutoIncrementStrategy int - RangeRotationStrategy int - ConstraintNamesStrategy int - ColumnRenameStrategy int - TableRenameStrategy int - FullTextKeyStrategy int - TableCharsetCollateStrategy int - TableQualifierHint int - AlterTableAlgorithmStrategy int - EnumReorderStrategy int - ForeignKeyCheckStrategy int - SubsequentDiffStrategy int - NonDeterministicDefaultStrategy int + StrictIndexOrdering bool + AutoIncrementStrategy int + RangeRotationStrategy int + ConstraintNamesStrategy int + ColumnRenameStrategy int + TableRenameStrategy int + FullTextKeyStrategy int + TableCharsetCollateStrategy int + TableQualifierHint int + AlterTableAlgorithmStrategy int + EnumReorderStrategy int + ForeignKeyCheckStrategy int + SubsequentDiffStrategy int } func EmptyDiffHints() *DiffHints {