From 3204350e6ed2e6023b29b2147f240e3a8d76071c Mon Sep 17 00:00:00 2001 From: Harshit Gangal Date: Thu, 1 Apr 2021 22:48:08 +0530 Subject: [PATCH 1/5] added fk executor test Signed-off-by: Harshit Gangal --- go/vt/vtgate/executor_test.go | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/go/vt/vtgate/executor_test.go b/go/vt/vtgate/executor_test.go index 9e64e75ea0d..a8143d9c9ec 100644 --- a/go/vt/vtgate/executor_test.go +++ b/go/vt/vtgate/executor_test.go @@ -1185,6 +1185,32 @@ func TestExecutorDDL(t *testing.T) { } } +func TestExecutorDDLFk(t *testing.T) { + executor, _, _, sbc := createLegacyExecutorEnv() + + mName := "TestExecutorDDLFk" + stmts := []string{ + "create table t1(id bigint primary key, foreign key (id) references t2(id))", + "alter table t2 add foreign key (id) references t1(id) on delete cascade", + } + + for _, stmt := range stmts { + for _, fk := range []string{"allow", "disallow"} { + t.Run(stmt+fk, func(t *testing.T) { + sbc.ExecCount.Set(0) + *foreignKey = fk + _, err := executor.Execute(ctx, mName, NewSafeSession(&vtgatepb.Session{TargetString: KsTestUnsharded}), stmt, nil) + if fk == "allow" { + require.NoError(t, err) + require.EqualValues(t, 1, sbc.ExecCount.Get()) + } else { + require.EqualError(t, err, "foreign key constraint is blocked") + } + }) + } + } +} + func TestExecutorAlterVSchemaKeyspace(t *testing.T) { *vschemaacl.AuthorizedDDLUsers = "%" defer func() { From 845b04378e2e7e03576efc139aa5a0283d3a5204 Mon Sep 17 00:00:00 2001 From: Harshit Gangal Date: Thu, 1 Apr 2021 23:34:21 +0530 Subject: [PATCH 2/5] added foreign key flag and added method in ContextVschema Signed-off-by: Harshit Gangal --- go/vt/vtgate/planbuilder/builder.go | 3 +++ go/vt/vtgate/planbuilder/plan_test.go | 4 ++++ go/vt/vtgate/vcursor_impl.go | 5 +++++ go/vt/vtgate/vtgate.go | 2 ++ 4 files changed, 14 insertions(+) diff --git a/go/vt/vtgate/planbuilder/builder.go b/go/vt/vtgate/planbuilder/builder.go index af3bcc188ff..50f2e91c1b1 100644 --- a/go/vt/vtgate/planbuilder/builder.go +++ b/go/vt/vtgate/planbuilder/builder.go @@ -61,6 +61,9 @@ type ContextVSchema interface { // This will let the user know that they are using something // that could become a problem if they move to a sharded keyspace WarnUnshardedOnly(format string, params ...interface{}) + + // ForeignKey returns the foreign_key flag value + ForeignKey() string } // PlannerVersion is an alias here to make the code more readable diff --git a/go/vt/vtgate/planbuilder/plan_test.go b/go/vt/vtgate/planbuilder/plan_test.go index 1f719d5cc20..f5613f239ed 100644 --- a/go/vt/vtgate/planbuilder/plan_test.go +++ b/go/vt/vtgate/planbuilder/plan_test.go @@ -312,6 +312,10 @@ type vschemaWrapper struct { version PlannerVersion } +func (vw *vschemaWrapper) ForeignKey() string { + return "allow" +} + func (vw *vschemaWrapper) AllKeyspace() ([]*vindexes.Keyspace, error) { if vw.keyspace == nil { return nil, errors.New("keyspace not available") diff --git a/go/vt/vtgate/vcursor_impl.go b/go/vt/vtgate/vcursor_impl.go index 78a8eb40704..fd87ac89f86 100644 --- a/go/vt/vtgate/vcursor_impl.go +++ b/go/vt/vtgate/vcursor_impl.go @@ -753,6 +753,11 @@ func (vc *vcursorImpl) WarnUnshardedOnly(format string, params ...interface{}) { } } +// ForeignKey implements the VCursor interface +func (vc *vcursorImpl) ForeignKey() string { + return *foreignKey +} + // ParseDestinationTarget parses destination target string and sets default keyspace if possible. func parseDestinationTarget(targetString string, vschema *vindexes.VSchema) (string, topodatapb.TabletType, key.Destination, error) { destKeyspace, destTabletType, dest, err := topoprotopb.ParseDestination(targetString, defaultTabletType) diff --git a/go/vt/vtgate/vtgate.go b/go/vt/vtgate/vtgate.go index 37c4650cc9a..54e4ce625a6 100644 --- a/go/vt/vtgate/vtgate.go +++ b/go/vt/vtgate/vtgate.go @@ -82,6 +82,8 @@ var ( // lockHeartbeatTime is used to set the next heartbeat time. lockHeartbeatTime = flag.Duration("lock_heartbeat_time", 5*time.Second, "If there is lock function used. This will keep the lock connection active by using this heartbeat") warnShardedOnly = flag.Bool("warn_sharded_only", false, "If any features that are only available in unsharded mode are used, query execution warnings will be added to the session") + + foreignKey = flag.String("foreign_key", "allow", "This is to provide how to handle foreign key constraint in create/alter table. Valid values are: allow, disallow") ) func getTxMode() vtgatepb.TransactionMode { From cbe5f8cd86788014c6903c258c7747f0f4a38bc8 Mon Sep 17 00:00:00 2001 From: Harshit Gangal Date: Thu, 1 Apr 2021 23:41:34 +0530 Subject: [PATCH 3/5] added logic to error on fk constraint if set to disallow Signed-off-by: Harshit Gangal --- go/vt/vtgate/executor_test.go | 4 +- go/vt/vtgate/planbuilder/ddl.go | 74 ++++++++++++++++++++++++--------- 2 files changed, 57 insertions(+), 21 deletions(-) diff --git a/go/vt/vtgate/executor_test.go b/go/vt/vtgate/executor_test.go index a8143d9c9ec..0d800475675 100644 --- a/go/vt/vtgate/executor_test.go +++ b/go/vt/vtgate/executor_test.go @@ -1186,7 +1186,7 @@ func TestExecutorDDL(t *testing.T) { } func TestExecutorDDLFk(t *testing.T) { - executor, _, _, sbc := createLegacyExecutorEnv() + executor, _, _, sbc := createExecutorEnv() mName := "TestExecutorDDLFk" stmts := []string{ @@ -1204,7 +1204,7 @@ func TestExecutorDDLFk(t *testing.T) { require.NoError(t, err) require.EqualValues(t, 1, sbc.ExecCount.Get()) } else { - require.EqualError(t, err, "foreign key constraint is blocked") + require.EqualError(t, err, "foreign key constraint is not allowed") } }) } diff --git a/go/vt/vtgate/planbuilder/ddl.go b/go/vt/vtgate/planbuilder/ddl.go index be3c6bb4b74..e54db2600ee 100644 --- a/go/vt/vtgate/planbuilder/ddl.go +++ b/go/vt/vtgate/planbuilder/ddl.go @@ -1,13 +1,15 @@ package planbuilder import ( - vtrpcpb "vitess.io/vitess/go/vt/proto/vtrpc" - "vitess.io/vitess/go/vt/vterrors" - "vitess.io/vitess/go/vt/vtgate/vindexes" + "strings" "vitess.io/vitess/go/vt/key" "vitess.io/vitess/go/vt/sqlparser" + "vitess.io/vitess/go/vt/vterrors" "vitess.io/vitess/go/vt/vtgate/engine" + "vitess.io/vitess/go/vt/vtgate/vindexes" + + vtrpcpb "vitess.io/vitess/go/vt/proto/vtrpc" ) // Error messages for CreateView queries @@ -17,6 +19,33 @@ const ( DifferentDestinations string = "Tables or Views specified in the query do not belong to the same destination" ) +type fkStrategy int + +const ( + allow fkStrategy = iota + disallow +) + +var fkStrategyMap = map[string]fkStrategy{ + "allow": allow, + "disallow": disallow, +} + +type fkContraint struct { + found bool +} + +func (fk *fkContraint) FkWalk(node sqlparser.SQLNode) (kontinue bool, err error) { + switch node.(type) { + case *sqlparser.CreateTable, *sqlparser.AlterTable, + *sqlparser.TableSpec, *sqlparser.AddConstraintDefinition, *sqlparser.ConstraintDefinition: + return true, nil + case *sqlparser.ForeignKeyDefinition: + fk.found = true + } + return false, nil +} + // buildGeneralDDLPlan builds a general DDL plan, which can be either normal DDL or online DDL. // The two behave compeltely differently, and have two very different primitives. // We want to be able to dynamically choose between normal/online plans according to Session settings. @@ -55,44 +84,40 @@ func buildDDLPlans(sql string, ddlStatement sqlparser.DDLStatement, reservedVars switch ddl := ddlStatement.(type) { case *sqlparser.AlterTable, *sqlparser.TruncateTable: - // For Alter Table and other statements, the table must already exist - // We should find the target of the query from this tables location - destination, keyspace, err = findTableDestinationAndKeyspace(vschema, ddlStatement) + err = checkFKError(vschema, ddlStatement) if err != nil { return nil, nil, err } + // For Alter Table and other statements, the table must already exist + // We should find the target of the query from this tables location + destination, keyspace, err = findTableDestinationAndKeyspace(vschema, ddlStatement) case *sqlparser.CreateView: destination, keyspace, err = buildCreateView(vschema, ddl, reservedVars) - if err != nil { - return nil, nil, err - } case *sqlparser.AlterView: destination, keyspace, err = buildAlterView(vschema, ddl, reservedVars) + case *sqlparser.CreateTable: + err = checkFKError(vschema, ddlStatement) if err != nil { return nil, nil, err } - case *sqlparser.CreateTable: destination, keyspace, _, err = vschema.TargetDestination(ddlStatement.GetTable().Qualifier.String()) - // Remove the keyspace name as the database name might be different. - ddlStatement.SetTable("", ddlStatement.GetTable().Name.String()) if err != nil { return nil, nil, err } + // Remove the keyspace name as the database name might be different. + ddlStatement.SetTable("", ddlStatement.GetTable().Name.String()) case *sqlparser.DropView, *sqlparser.DropTable: destination, keyspace, err = buildDropViewOrTable(vschema, ddlStatement) - if err != nil { - return nil, nil, err - } case *sqlparser.RenameTable: destination, keyspace, err = buildRenameTable(vschema, ddl) - if err != nil { - return nil, nil, err - } - default: return nil, nil, vterrors.Errorf(vtrpcpb.Code_INTERNAL, "[BUG] unexpected ddl statement type: %T", ddlStatement) } + if err != nil { + return nil, nil, err + } + if destination == nil { destination = key.DestinationAllShards{} } @@ -116,6 +141,17 @@ func buildDDLPlans(sql string, ddlStatement sqlparser.DDLStatement, reservedVars }, nil } +func checkFKError(vschema ContextVSchema, ddlStatement sqlparser.DDLStatement) error { + if fkStrategyMap[strings.ToLower(vschema.ForeignKey())] == disallow { + fk := &fkContraint{} + _ = sqlparser.Walk(fk.FkWalk, ddlStatement) + if fk.found { + return vterrors.Errorf(vtrpcpb.Code_ABORTED, "foreign key constraint is not allowed") + } + } + return nil +} + func findTableDestinationAndKeyspace(vschema ContextVSchema, ddlStatement sqlparser.DDLStatement) (key.Destination, *vindexes.Keyspace, error) { var table *vindexes.Table var destination key.Destination From 9a88668db162e31d6c7fb48ab711c195b35972ab Mon Sep 17 00:00:00 2001 From: Harshit Gangal Date: Sun, 4 Apr 2021 16:18:29 +0530 Subject: [PATCH 4/5] addressed review comments to change flag to foreign_key_mode Signed-off-by: Harshit Gangal --- go/vt/vtgate/executor_test.go | 8 ++++---- go/vt/vtgate/planbuilder/builder.go | 4 ++-- go/vt/vtgate/planbuilder/ddl.go | 2 +- go/vt/vtgate/planbuilder/plan_test.go | 2 +- go/vt/vtgate/vcursor_impl.go | 4 ++-- go/vt/vtgate/vtgate.go | 2 +- 6 files changed, 11 insertions(+), 11 deletions(-) diff --git a/go/vt/vtgate/executor_test.go b/go/vt/vtgate/executor_test.go index 0d800475675..2f9fd2f80ac 100644 --- a/go/vt/vtgate/executor_test.go +++ b/go/vt/vtgate/executor_test.go @@ -1195,12 +1195,12 @@ func TestExecutorDDLFk(t *testing.T) { } for _, stmt := range stmts { - for _, fk := range []string{"allow", "disallow"} { - t.Run(stmt+fk, func(t *testing.T) { + for _, fkMode := range []string{"allow", "disallow"} { + t.Run(stmt+fkMode, func(t *testing.T) { sbc.ExecCount.Set(0) - *foreignKey = fk + *foreignKeyMode = fkMode _, err := executor.Execute(ctx, mName, NewSafeSession(&vtgatepb.Session{TargetString: KsTestUnsharded}), stmt, nil) - if fk == "allow" { + if fkMode == "allow" { require.NoError(t, err) require.EqualValues(t, 1, sbc.ExecCount.Get()) } else { diff --git a/go/vt/vtgate/planbuilder/builder.go b/go/vt/vtgate/planbuilder/builder.go index 50f2e91c1b1..23b6a968c02 100644 --- a/go/vt/vtgate/planbuilder/builder.go +++ b/go/vt/vtgate/planbuilder/builder.go @@ -62,8 +62,8 @@ type ContextVSchema interface { // that could become a problem if they move to a sharded keyspace WarnUnshardedOnly(format string, params ...interface{}) - // ForeignKey returns the foreign_key flag value - ForeignKey() string + // ForeignKeyMode returns the foreign_key flag value + ForeignKeyMode() string } // PlannerVersion is an alias here to make the code more readable diff --git a/go/vt/vtgate/planbuilder/ddl.go b/go/vt/vtgate/planbuilder/ddl.go index e54db2600ee..932d0e6e0f7 100644 --- a/go/vt/vtgate/planbuilder/ddl.go +++ b/go/vt/vtgate/planbuilder/ddl.go @@ -142,7 +142,7 @@ func buildDDLPlans(sql string, ddlStatement sqlparser.DDLStatement, reservedVars } func checkFKError(vschema ContextVSchema, ddlStatement sqlparser.DDLStatement) error { - if fkStrategyMap[strings.ToLower(vschema.ForeignKey())] == disallow { + if fkStrategyMap[strings.ToLower(vschema.ForeignKeyMode())] == disallow { fk := &fkContraint{} _ = sqlparser.Walk(fk.FkWalk, ddlStatement) if fk.found { diff --git a/go/vt/vtgate/planbuilder/plan_test.go b/go/vt/vtgate/planbuilder/plan_test.go index f5613f239ed..ea8c3568aa5 100644 --- a/go/vt/vtgate/planbuilder/plan_test.go +++ b/go/vt/vtgate/planbuilder/plan_test.go @@ -312,7 +312,7 @@ type vschemaWrapper struct { version PlannerVersion } -func (vw *vschemaWrapper) ForeignKey() string { +func (vw *vschemaWrapper) ForeignKeyMode() string { return "allow" } diff --git a/go/vt/vtgate/vcursor_impl.go b/go/vt/vtgate/vcursor_impl.go index fd87ac89f86..4897767923a 100644 --- a/go/vt/vtgate/vcursor_impl.go +++ b/go/vt/vtgate/vcursor_impl.go @@ -754,8 +754,8 @@ func (vc *vcursorImpl) WarnUnshardedOnly(format string, params ...interface{}) { } // ForeignKey implements the VCursor interface -func (vc *vcursorImpl) ForeignKey() string { - return *foreignKey +func (vc *vcursorImpl) ForeignKeyMode() string { + return *foreignKeyMode } // ParseDestinationTarget parses destination target string and sets default keyspace if possible. diff --git a/go/vt/vtgate/vtgate.go b/go/vt/vtgate/vtgate.go index 54e4ce625a6..33e18832d6b 100644 --- a/go/vt/vtgate/vtgate.go +++ b/go/vt/vtgate/vtgate.go @@ -83,7 +83,7 @@ var ( lockHeartbeatTime = flag.Duration("lock_heartbeat_time", 5*time.Second, "If there is lock function used. This will keep the lock connection active by using this heartbeat") warnShardedOnly = flag.Bool("warn_sharded_only", false, "If any features that are only available in unsharded mode are used, query execution warnings will be added to the session") - foreignKey = flag.String("foreign_key", "allow", "This is to provide how to handle foreign key constraint in create/alter table. Valid values are: allow, disallow") + foreignKeyMode = flag.String("foreign_key_mode", "allow", "This is to provide how to handle foreign key constraint in create/alter table. Valid values are: allow, disallow") ) func getTxMode() vtgatepb.TransactionMode { From c3c56ba3e736f552d221af92c0f64ef3eea0129f Mon Sep 17 00:00:00 2001 From: Harshit Gangal Date: Sun, 4 Apr 2021 21:19:28 +0530 Subject: [PATCH 5/5] addressed review comments Signed-off-by: Harshit Gangal --- go/vt/vtgate/planbuilder/ddl.go | 12 +++++------- go/vt/vtgate/vcursor_impl.go | 5 ++++- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/go/vt/vtgate/planbuilder/ddl.go b/go/vt/vtgate/planbuilder/ddl.go index 932d0e6e0f7..04e6ddfcd21 100644 --- a/go/vt/vtgate/planbuilder/ddl.go +++ b/go/vt/vtgate/planbuilder/ddl.go @@ -1,8 +1,6 @@ package planbuilder import ( - "strings" - "vitess.io/vitess/go/vt/key" "vitess.io/vitess/go/vt/sqlparser" "vitess.io/vitess/go/vt/vterrors" @@ -22,13 +20,13 @@ const ( type fkStrategy int const ( - allow fkStrategy = iota - disallow + fkAllow fkStrategy = iota + fkDisallow ) var fkStrategyMap = map[string]fkStrategy{ - "allow": allow, - "disallow": disallow, + "allow": fkAllow, + "disallow": fkDisallow, } type fkContraint struct { @@ -142,7 +140,7 @@ func buildDDLPlans(sql string, ddlStatement sqlparser.DDLStatement, reservedVars } func checkFKError(vschema ContextVSchema, ddlStatement sqlparser.DDLStatement) error { - if fkStrategyMap[strings.ToLower(vschema.ForeignKeyMode())] == disallow { + if fkStrategyMap[vschema.ForeignKeyMode()] == fkDisallow { fk := &fkContraint{} _ = sqlparser.Walk(fk.FkWalk, ddlStatement) if fk.found { diff --git a/go/vt/vtgate/vcursor_impl.go b/go/vt/vtgate/vcursor_impl.go index 4897767923a..f60af6dca2b 100644 --- a/go/vt/vtgate/vcursor_impl.go +++ b/go/vt/vtgate/vcursor_impl.go @@ -755,7 +755,10 @@ func (vc *vcursorImpl) WarnUnshardedOnly(format string, params ...interface{}) { // ForeignKey implements the VCursor interface func (vc *vcursorImpl) ForeignKeyMode() string { - return *foreignKeyMode + if foreignKeyMode == nil { + return "" + } + return strings.ToLower(*foreignKeyMode) } // ParseDestinationTarget parses destination target string and sets default keyspace if possible.