cherry pick 15.0: OnlineDDL: support --unsafe-allow-foreign-keys stra…

…tegy flag #11976 (#1462) * resolve conflict Signed-off-by: Shlomi Noach <[email protected]> * complete the merge Signed-off-by: Shlomi Noach <[email protected]> Signed-off-by: Shlomi Noach <[email protected]>
vitessio · Jan 9, 2023 · 5b1c399 · 5b1c399
1 parent f4a9ac7
commit 5b1c399
Show file tree

Hide file tree

Showing 4 changed files with 100 additions and 28 deletions.
diff --git a/go/vt/schema/ddl_strategy.go b/go/vt/schema/ddl_strategy.go
@@ -39,6 +39,7 @@ const (
 	preferInstantDDL       = "prefer-instant-ddl"
 	fastRangeRotationFlag  = "fast-range-rotation"
 	vreplicationTestSuite  = "vreplication-test-suite"
+	allowForeignKeysFlag   = "unsafe-allow-foreign-keys"
 )
 
 // DDLStrategy suggests how an ALTER TABLE should run (e.g. "direct", "online", "gh-ost" or "pt-osc")
@@ -123,56 +124,61 @@ func (setting *DDLStrategySetting) hasFlag(name string) bool {
 	return false
 }
 
-// IsDeclarative checks if strategy options include -declarative
+// IsDeclarative checks if strategy options include --declarative
 func (setting *DDLStrategySetting) IsDeclarative() bool {
 	return setting.hasFlag(declarativeFlag)
 }
 
-// IsSingleton checks if strategy options include -singleton
+// IsSingleton checks if strategy options include --singleton
 func (setting *DDLStrategySetting) IsSingleton() bool {
 	return setting.hasFlag(singletonFlag)
 }
 
-// IsSingletonContext checks if strategy options include -singleton-context
+// IsSingletonContext checks if strategy options include --singleton-context
 func (setting *DDLStrategySetting) IsSingletonContext() bool {
 	return setting.hasFlag(singletonContextFlag)
 }
 
-// IsAllowZeroInDateFlag checks if strategy options include -allow-zero-in-date
+// IsAllowZeroInDateFlag checks if strategy options include --allow-zero-in-date
 func (setting *DDLStrategySetting) IsAllowZeroInDateFlag() bool {
 	return setting.hasFlag(allowZeroInDateFlag)
 }
 
-// IsPostponeLaunch checks if strategy options include -postpone-launch
+// IsPostponeLaunch checks if strategy options include --postpone-launch
 func (setting *DDLStrategySetting) IsPostponeLaunch() bool {
 	return setting.hasFlag(postponeLaunchFlag)
 }
 
-// IsPostponeCompletion checks if strategy options include -postpone-completion
+// IsPostponeCompletion checks if strategy options include --postpone-completion
 func (setting *DDLStrategySetting) IsPostponeCompletion() bool {
 	return setting.hasFlag(postponeCompletionFlag)
 }
 
-// IsAllowConcurrent checks if strategy options include -allow-concurrent
+// IsAllowConcurrent checks if strategy options include --allow-concurrent
 func (setting *DDLStrategySetting) IsAllowConcurrent() bool {
 	return setting.hasFlag(allowConcurrentFlag)
 }
 
-// IsPreferInstantDDL checks if strategy options include -prefer-instant-ddl
+// IsPreferInstantDDL checks if strategy options include --prefer-instant-ddl
 func (setting *DDLStrategySetting) IsPreferInstantDDL() bool {
 	return setting.hasFlag(preferInstantDDL)
 }
 
-// IsFastRangeRotationFlag checks if strategy options include -fast-range-rotation
+// IsFastRangeRotationFlag checks if strategy options include --fast-range-rotation
 func (setting *DDLStrategySetting) IsFastRangeRotationFlag() bool {
 	return setting.hasFlag(fastRangeRotationFlag)
 }
 
-// IsVreplicationTestSuite checks if strategy options include -vreplicatoin-test-suite
+// IsVreplicationTestSuite checks if strategy options include --vreplicatoin-test-suite
 func (setting *DDLStrategySetting) IsVreplicationTestSuite() bool {
 	return setting.hasFlag(vreplicationTestSuite)
 }
 
+// IsAllowForeignKeysFlag checks if strategy options include --unsafe-allow-foreign-keys
+func (setting *DDLStrategySetting) IsAllowForeignKeysFlag() bool {
+	return setting.hasFlag(allowForeignKeysFlag)
+}
+
 // RuntimeOptions returns the options used as runtime flags for given strategy, removing any internal hint options
 func (setting *DDLStrategySetting) RuntimeOptions() []string {
 	opts, _ := shlex.Split(setting.Options)
@@ -190,6 +196,7 @@ func (setting *DDLStrategySetting) RuntimeOptions() []string {
 		case isFlag(opt, preferInstantDDL):
 		case isFlag(opt, fastRangeRotationFlag):
 		case isFlag(opt, vreplicationTestSuite):
+		case isFlag(opt, allowForeignKeysFlag):
 		default:
 			validOpts = append(validOpts, opt)
 		}

diff --git a/go/vt/schema/ddl_strategy_test.go b/go/vt/schema/ddl_strategy_test.go
@@ -49,6 +49,7 @@ func TestParseDDLStrategy(t *testing.T) {
 		isAllowConcurrent    bool
 		fastOverRevertible   bool
 		fastRangeRotation    bool
+		allowForeignKeys     bool
 		runtimeOptions       string
 		err                  error
 	}{
@@ -145,22 +146,32 @@ func TestParseDDLStrategy(t *testing.T) {
 			runtimeOptions:    "",
 			fastRangeRotation: true,
 		},
+		{
+			strategyVariable: "vitess --unsafe-allow-foreign-keys",
+			strategy:         DDLStrategyVitess,
+			options:          "--unsafe-allow-foreign-keys",
+			runtimeOptions:   "",
+			allowForeignKeys: true,
+		},
 	}
 	for _, ts := range tt {
-		setting, err := ParseDDLStrategy(ts.strategyVariable)
-		assert.NoError(t, err)
-		assert.Equal(t, ts.strategy, setting.Strategy)
-		assert.Equal(t, ts.options, setting.Options)
-		assert.Equal(t, ts.isDeclarative, setting.IsDeclarative())
-		assert.Equal(t, ts.isSingleton, setting.IsSingleton())
-		assert.Equal(t, ts.isPostponeCompletion, setting.IsPostponeCompletion())
-		assert.Equal(t, ts.isPostponeLaunch, setting.IsPostponeLaunch())
-		assert.Equal(t, ts.isAllowConcurrent, setting.IsAllowConcurrent())
-		assert.Equal(t, ts.fastOverRevertible, setting.IsPreferInstantDDL())
-		assert.Equal(t, ts.fastRangeRotation, setting.IsFastRangeRotationFlag())
+		t.Run(ts.strategyVariable, func(t *testing.T) {
+			setting, err := ParseDDLStrategy(ts.strategyVariable)
+			assert.NoError(t, err)
+			assert.Equal(t, ts.strategy, setting.Strategy)
+			assert.Equal(t, ts.options, setting.Options)
+			assert.Equal(t, ts.isDeclarative, setting.IsDeclarative())
+			assert.Equal(t, ts.isSingleton, setting.IsSingleton())
+			assert.Equal(t, ts.isPostponeCompletion, setting.IsPostponeCompletion())
+			assert.Equal(t, ts.isPostponeLaunch, setting.IsPostponeLaunch())
+			assert.Equal(t, ts.isAllowConcurrent, setting.IsAllowConcurrent())
+			assert.Equal(t, ts.fastOverRevertible, setting.IsPreferInstantDDL())
+			assert.Equal(t, ts.fastRangeRotation, setting.IsFastRangeRotationFlag())
+			assert.Equal(t, ts.allowForeignKeys, setting.IsAllowForeignKeysFlag())
 
-		runtimeOptions := strings.Join(setting.RuntimeOptions(), " ")
-		assert.Equal(t, ts.runtimeOptions, runtimeOptions)
+			runtimeOptions := strings.Join(setting.RuntimeOptions(), " ")
+			assert.Equal(t, ts.runtimeOptions, runtimeOptions)
+		})
 	}
 	{
 		_, err := ParseDDLStrategy("other")

diff --git a/go/vt/schema/online_ddl.go b/go/vt/schema/online_ddl.go
@@ -54,13 +54,15 @@ const (
 )
 
 // when validateWalk returns true, then the child nodes are also visited
-func validateWalk(node sqlparser.SQLNode) (kontinue bool, err error) {
+func validateWalk(node sqlparser.SQLNode, allowForeignKeys bool) (kontinue bool, err error) {
 	switch node.(type) {
 	case *sqlparser.CreateTable, *sqlparser.AlterTable,
 		*sqlparser.TableSpec, *sqlparser.AddConstraintDefinition, *sqlparser.ConstraintDefinition:
 		return true, nil
 	case *sqlparser.ForeignKeyDefinition:
-		return false, ErrForeignKeyFound
+		if !allowForeignKeys {
+			return false, ErrForeignKeyFound
+		}
 	case *sqlparser.RenameTableName:
 		return false, ErrRenameTableFound
 	}
@@ -118,7 +120,7 @@ func ParseOnlineDDLStatement(sql string) (ddlStmt sqlparser.DDLStatement, action
 	return ddlStmt, action, vterrors.Errorf(vtrpcpb.Code_INVALID_ARGUMENT, "unsupported query type: %s", sql)
 }
 
-func onlineDDLStatementSanity(sql string, ddlStmt sqlparser.DDLStatement) error {
+func onlineDDLStatementSanity(sql string, ddlStmt sqlparser.DDLStatement, ddlStrategySetting *DDLStrategySetting) error {
 	// SQL statement sanity checks:
 	if !ddlStmt.IsFullyParsed() {
 		if _, err := sqlparser.ParseStrictDDL(sql); err != nil {
@@ -128,7 +130,10 @@ func onlineDDLStatementSanity(sql string, ddlStmt sqlparser.DDLStatement) error
 		return vterrors.NewErrorf(vtrpcpb.Code_INVALID_ARGUMENT, vterrors.SyntaxError, "cannot parse statement: %v", sql)
 	}
 
-	if err := sqlparser.Walk(validateWalk, ddlStmt); err != nil {
+	walkFunc := func(node sqlparser.SQLNode) (kontinue bool, err error) {
+		return validateWalk(node, ddlStrategySetting.IsAllowForeignKeysFlag())
+	}
+	if err := sqlparser.Walk(walkFunc, ddlStmt); err != nil {
 		switch err {
 		case ErrForeignKeyFound:
 			return vterrors.Errorf(vtrpcpb.Code_ABORTED, "foreign key constraints are not supported in online DDL, see https://vitess.io/blog/2021-06-15-online-ddl-why-no-fk/")
@@ -142,7 +147,7 @@ func onlineDDLStatementSanity(sql string, ddlStmt sqlparser.DDLStatement) error
 // NewOnlineDDLs takes a single DDL statement, normalizes it (potentially break down into multiple statements), and generates one or more OnlineDDL instances, one for each normalized statement
 func NewOnlineDDLs(keyspace string, sql string, ddlStmt sqlparser.DDLStatement, ddlStrategySetting *DDLStrategySetting, migrationContext string, providedUUID string) (onlineDDLs [](*OnlineDDL), err error) {
 	appendOnlineDDL := func(tableName string, ddlStmt sqlparser.DDLStatement) error {
-		if err := onlineDDLStatementSanity(sql, ddlStmt); err != nil {
+		if err := onlineDDLStatementSanity(sql, ddlStmt, ddlStrategySetting); err != nil {
 			return err
 		}
 		onlineDDL, err := NewOnlineDDL(keyspace, tableName, sqlparser.String(ddlStmt), ddlStrategySetting, migrationContext, providedUUID)

diff --git a/go/vt/schema/online_ddl_test.go b/go/vt/schema/online_ddl_test.go
@@ -18,6 +18,7 @@ package schema
 
 import (
 	"encoding/hex"
+	"fmt"
 	"strings"
 	"testing"
 
@@ -323,6 +324,54 @@ func TestNewOnlineDDLs(t *testing.T) {
 	}
 }
 
+func TestNewOnlineDDLsForeignKeys(t *testing.T) {
+	type expect struct {
+		sqls            []string
+		notDDL          bool
+		parseError      bool
+		isError         bool
+		expectErrorText string
+		isView          bool
+	}
+	queries := []string{
+		"alter table corder add FOREIGN KEY my_fk(customer_id) references customer(customer_id)",
+		"create table t1 (id int primary key, i int, foreign key (i) references parent(id))",
+	}
+
+	migrationContext := "354b-11eb-82cd-f875a4d24e90"
+	for _, query := range queries {
+		t.Run(query, func(t *testing.T) {
+			for _, allowForeignKeys := range []bool{false, true} {
+				testName := fmt.Sprintf("%t", allowForeignKeys)
+				t.Run(testName, func(t *testing.T) {
+					stmt, err := sqlparser.Parse(query)
+					require.NoError(t, err)
+					ddlStmt, ok := stmt.(sqlparser.DDLStatement)
+					require.True(t, ok)
+
+					flags := ""
+					if allowForeignKeys {
+						flags = "--unsafe-allow-foreign-keys"
+					}
+					onlineDDLs, err := NewOnlineDDLs("test_ks", query, ddlStmt, NewDDLStrategySetting(DDLStrategyVitess, flags), migrationContext, "")
+					if allowForeignKeys {
+						assert.NoError(t, err)
+					} else {
+						assert.Error(t, err)
+						assert.Contains(t, err.Error(), "foreign key constraints are not supported")
+					}
+
+					for _, onlineDDL := range onlineDDLs {
+						sql, err := onlineDDL.sqlWithoutComments()
+						assert.NoError(t, err)
+						assert.NotEmpty(t, sql)
+					}
+				})
+			}
+		})
+	}
+}
+
 func TestOnlineDDLFromCommentedStatement(t *testing.T) {
 	queries := []string{
 		`create table t (id int primary key)`,