Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

schemadiff: reject non-deterministic function in new column's default value #16684

Merged
merged 4 commits into from
Sep 2, 2024
Merged

schemadiff: reject non-deterministic function in new column's default value #16684

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6133fbc
schemadiff: new NonDeterministicDefaultStrategy, can reject non-deter…
shlomi-noach Aug 29, 2024
d588cd8
recursion is not needed
shlomi-noach Sep 2, 2024
88895d7
formatting directive
shlomi-noach Sep 2, 2024
2cd96bc
Removing NonDeterministicDefaultStrategy. It is non-optional as MySQL…
shlomi-noach Sep 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions go/vt/schemadiff/errors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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: %s", sqlescape.EscapeID(e.Table), sqlescape.EscapeID(e.Column), e.Function)
}
38 changes: 38 additions & 0 deletions go/vt/schemadiff/table.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1731,6 +1731,33 @@ 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
}
}
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.
Expand Down Expand Up @@ -1852,6 +1879,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 {
Expand Down
124 changes: 105 additions & 19 deletions go/vt/schemadiff/table_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}{
Expand Down Expand Up @@ -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())",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this always errors on MySQL, should it even be a flag on schemadiff or should we also always use error?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. Removed the option altogether, the issue title is now misleading and I'll fix it. Confirmed that MySQL rejects this kind of statement even for empty tables (as opposed to another issue we found the other day where the success depends on whether the table is empty or not). This is a bit of inconsistent behavior for MySQL, but at least we're consistent with it...

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",
Expand Down Expand Up @@ -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))
Expand Down
32 changes: 19 additions & 13 deletions go/vt/schemadiff/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
Loading