From c4d3409027a49488e855f8476bdff602092e47e2 Mon Sep 17 00:00:00 2001 From: Harshit Gangal Date: Thu, 10 Aug 2023 17:11:04 +0530 Subject: [PATCH 1/5] feat: verify fk management requirement for update columns Signed-off-by: Harshit Gangal --- go/vt/vtgate/planbuilder/delete.go | 2 +- go/vt/vtgate/planbuilder/operators/ast2op.go | 20 +++++- go/vt/vtgate/planbuilder/plan_test.go | 57 +++++++++-------- .../testdata/foreignkey_cases.json | 61 +++++++++++++++++-- .../planbuilder/testdata/vschemas/schema.json | 13 +++- go/vt/vtgate/planbuilder/update.go | 47 +++++++++++++- go/vt/vtgate/vindexes/foreign_keys.go | 7 ++- go/vt/vtgate/vindexes/foreign_keys_test.go | 3 +- 8 files changed, 170 insertions(+), 40 deletions(-) diff --git a/go/vt/vtgate/planbuilder/delete.go b/go/vt/vtgate/planbuilder/delete.go index cef2108824e..4fae6ae97fa 100644 --- a/go/vt/vtgate/planbuilder/delete.go +++ b/go/vt/vtgate/planbuilder/delete.go @@ -111,7 +111,7 @@ func fkManagementNotRequired(vschema plancontext.VSchema, vTables []*vindexes.Ta if ksMode != vschemapb.Keyspace_FK_MANAGED { continue } - childFks := vTable.ChildFKsNeedsHandling() + childFks := vTable.ChildFKsNeedsHandling(vindexes.DeleteAction) if len(childFks) > 0 { return false } diff --git a/go/vt/vtgate/planbuilder/operators/ast2op.go b/go/vt/vtgate/planbuilder/operators/ast2op.go index 24e0de66a0a..6be8feea537 100644 --- a/go/vt/vtgate/planbuilder/operators/ast2op.go +++ b/go/vt/vtgate/planbuilder/operators/ast2op.go @@ -160,6 +160,24 @@ func createOperatorFromUpdate(ctx *plancontext.PlanningContext, updStmt *sqlpars Routing: routing, } + ksMode, err := ctx.VSchema.ForeignKeyMode(vindexTable.Keyspace.Name) + if err != nil { + return nil, err + } + if ksMode == vschemapb.Keyspace_FK_MANAGED { + childFks := vindexTable.ChildFKsNeedsHandling(vindexes.UpdateAction) + if len(childFks) > 0 { + // Check if any column in the parent table is being updated which has a child foreign key. + for _, updateExpr := range updStmt.Exprs { + for _, childFk := range childFks { + if childFk.ParentColumns.FindColumn(updateExpr.Name.Name) >= 0 { + return nil, vterrors.VT12003() + } + } + } + } + } + subq, err := createSubqueryFromStatement(ctx, updStmt) if err != nil { return nil, err @@ -197,7 +215,7 @@ func createOperatorFromDelete(ctx *plancontext.PlanningContext, deleteStmt *sqlp return nil, err } if ksMode == vschemapb.Keyspace_FK_MANAGED { - childFks := vindexTable.ChildFKsNeedsHandling() + childFks := vindexTable.ChildFKsNeedsHandling(vindexes.DeleteAction) if len(childFks) > 0 { return nil, vterrors.VT12003() } diff --git a/go/vt/vtgate/planbuilder/plan_test.go b/go/vt/vtgate/planbuilder/plan_test.go index fe8bb11b72c..f192f1e14a5 100644 --- a/go/vt/vtgate/planbuilder/plan_test.go +++ b/go/vt/vtgate/planbuilder/plan_test.go @@ -102,19 +102,43 @@ func TestPlan(t *testing.T) { // TestForeignKeyPlanning tests the planning of foreign keys in a managed mode by Vitess. func TestForeignKeyPlanning(t *testing.T) { - vschemaWrapper := &vschemaWrapper{ - v: loadSchema(t, "vschemas/schema.json", true), - // Set the keyspace with foreign keys enabled as the default. - keyspace: &vindexes.Keyspace{ - Name: "user_fk_allow", - Sharded: true, - }, - } + vschema := loadSchema(t, "vschemas/schema.json", true) + setFks(t, vschema) + + vschemaWrapper := &vschemaWrapper{v: vschema} testOutputTempDir := makeTestOutput(t) testFile(t, "foreignkey_cases.json", testOutputTempDir, vschemaWrapper, false) } +func setFks(t *testing.T, vschema *vindexes.VSchema) { + if vschema.Keyspaces["sharded_fk_allow"] != nil { + // FK from multicol_tbl2 referencing multicol_tbl1 that is shard scoped. + _ = vschema.AddForeignKey("sharded_fk_allow", "multicol_tbl2", createFkDefinition([]string{"colb", "cola", "x", "colc", "y"}, "multicol_tbl1", []string{"colb", "cola", "y", "colc", "x"}, sqlparser.Cascade, sqlparser.Cascade)) + // FK from tbl2 referencing tbl1 that is shard scoped. + _ = vschema.AddForeignKey("sharded_fk_allow", "tbl2", createFkDefinition([]string{"col2"}, "tbl1", []string{"col1"}, sqlparser.Restrict, sqlparser.Restrict)) + // FK from tbl3 referencing tbl1 that is not shard scoped. + _ = vschema.AddForeignKey("sharded_fk_allow", "tbl3", createFkDefinition([]string{"coly"}, "tbl1", []string{"col1"}, sqlparser.DefaultAction, sqlparser.DefaultAction)) + // FK from tbl4 referencing tbl5 that is shard scoped. + _ = vschema.AddForeignKey("sharded_fk_allow", "tbl4", createFkDefinition([]string{"col4"}, "tbl5", []string{"col5"}, sqlparser.SetNull, sqlparser.SetNull)) + // FK from tbl6 referencing tbl7 that is shard scoped. + _ = vschema.AddForeignKey("sharded_fk_allow", "tbl6", createFkDefinition([]string{"col6"}, "tbl7", []string{"col7"}, sqlparser.NoAction, sqlparser.NoAction)) + } + if vschema.Keyspaces["unsharded_fk_allow"] != nil { + // u_tbl2(col2) -> u_tbl1(col1) Cascade. + // u_tbl3(col2) -> u_tbl2(col2) Cascade Null. + // u_tbl4(col41) -> u_tbl1(col14) Restrict. + // u_tbl4(col4) -> u_tbl3(col3) Restrict. + // u_tbl6(col6) -> u_tbl5(col5) Restrict. + + _ = vschema.AddForeignKey("unsharded_fk_allow", "u_tbl2", createFkDefinition([]string{"col2"}, "u_tbl1", []string{"col1"}, sqlparser.Cascade, sqlparser.Cascade)) + _ = vschema.AddForeignKey("unsharded_fk_allow", "u_tbl3", createFkDefinition([]string{"col3"}, "u_tbl2", []string{"col2"}, sqlparser.SetNull, sqlparser.SetNull)) + _ = vschema.AddForeignKey("unsharded_fk_allow", "u_tbl4", createFkDefinition([]string{"col41"}, "u_tbl1", []string{"col14"}, sqlparser.NoAction, sqlparser.NoAction)) + _ = vschema.AddForeignKey("unsharded_fk_allow", "u_tbl4", createFkDefinition([]string{"col4"}, "u_tbl3", []string{"col3"}, sqlparser.Restrict, sqlparser.Restrict)) + _ = vschema.AddForeignKey("unsharded_fk_allow", "u_tbl6", createFkDefinition([]string{"col6"}, "u_tbl5", []string{"col5"}, sqlparser.DefaultAction, sqlparser.DefaultAction)) + } +} + func TestSystemTables57(t *testing.T) { // first we move everything to use 5.7 logic oldVer := servenv.MySQLServerVersion() @@ -439,23 +463,6 @@ func loadSchema(t testing.TB, filename string, setCollation bool) *vindexes.VSch } } } - if vschema.Keyspaces["user_fk_allow"] != nil { - // FK from multicol_tbl2 referencing multicol_tbl1 that is shard scoped. - err = vschema.AddForeignKey("user_fk_allow", "multicol_tbl2", createFkDefinition([]string{"colb", "cola", "x", "colc", "y"}, "multicol_tbl1", []string{"colb", "cola", "y", "colc", "x"}, sqlparser.Cascade, sqlparser.Cascade)) - require.NoError(t, err) - // FK from tbl2 referencing tbl1 that is shard scoped. - err = vschema.AddForeignKey("user_fk_allow", "tbl2", createFkDefinition([]string{"col2"}, "tbl1", []string{"col1"}, sqlparser.Restrict, sqlparser.Restrict)) - require.NoError(t, err) - // FK from tbl3 referencing tbl1 that is not shard scoped. - err = vschema.AddForeignKey("user_fk_allow", "tbl3", createFkDefinition([]string{"coly"}, "tbl1", []string{"col1"}, sqlparser.DefaultAction, sqlparser.DefaultAction)) - require.NoError(t, err) - // FK from tbl4 referencing tbl5 that is shard scoped. - err = vschema.AddForeignKey("user_fk_allow", "tbl4", createFkDefinition([]string{"col4"}, "tbl5", []string{"col5"}, sqlparser.SetNull, sqlparser.SetNull)) - require.NoError(t, err) - // FK from tbl6 referencing tbl7 that is shard scoped. - err = vschema.AddForeignKey("user_fk_allow", "tbl6", createFkDefinition([]string{"col6"}, "tbl7", []string{"col7"}, sqlparser.NoAction, sqlparser.NoAction)) - require.NoError(t, err) - } return vschema } diff --git a/go/vt/vtgate/planbuilder/testdata/foreignkey_cases.json b/go/vt/vtgate/planbuilder/testdata/foreignkey_cases.json index aefdd6dfb50..de5797f296f 100644 --- a/go/vt/vtgate/planbuilder/testdata/foreignkey_cases.json +++ b/go/vt/vtgate/planbuilder/testdata/foreignkey_cases.json @@ -14,7 +14,7 @@ "OperatorType": "Insert", "Variant": "Sharded", "Keyspace": { - "Name": "user_fk_allow", + "Name": "sharded_fk_allow", "Sharded": true }, "TargetTabletType": "PRIMARY", @@ -25,7 +25,7 @@ } }, "TablesUsed": [ - "user_fk_allow.tbl2" + "sharded_fk_allow.tbl2" ] } }, @@ -39,7 +39,7 @@ "OperatorType": "Insert", "Variant": "Sharded", "Keyspace": { - "Name": "user_fk_allow", + "Name": "sharded_fk_allow", "Sharded": true }, "TargetTabletType": "PRIMARY", @@ -50,7 +50,7 @@ } }, "TablesUsed": [ - "user_fk_allow.multicol_tbl2" + "sharded_fk_allow.multicol_tbl2" ] } }, @@ -69,7 +69,7 @@ "OperatorType": "Delete", "Variant": "Scatter", "Keyspace": { - "Name": "user_fk_allow", + "Name": "sharded_fk_allow", "Sharded": true }, "TargetTabletType": "PRIMARY", @@ -77,7 +77,7 @@ "Table": "tbl7" }, "TablesUsed": [ - "user_fk_allow.tbl7" + "sharded_fk_allow.tbl7" ] } }, @@ -90,5 +90,54 @@ "comment": "Delete in a table with shard-scoped foreign keys with cascade disallowed", "query": "delete from tbl5", "plan": "VT12002: unsupported: foreign keys management at vitess" + }, + { + "comment": "update in unsharded table with restrict", + "query": "update u_tbl5 set col5 = 'foo' where id = 1", + "plan": { + "QueryType": "UPDATE", + "Original": "update u_tbl5 set col5 = 'foo' where id = 1", + "Instructions": { + "OperatorType": "Update", + "Variant": "Unsharded", + "Keyspace": { + "Name": "unsharded_fk_allow", + "Sharded": false + }, + "TargetTabletType": "PRIMARY", + "Query": "update u_tbl5 set col5 = 'foo' where id = 1", + "Table": "u_tbl5" + }, + "TablesUsed": [ + "unsharded_fk_allow.u_tbl5" + ] + } + }, + { + "comment": "update in unsharded table with cascade", + "query": "update u_tbl2 set col2 = 'bar' where id = 1", + "plan": "VT12002: unsupported: foreign keys management at vitess" + }, + { + "comment": "update in unsharded table with cascade - on non-referenced column", + "query": "update u_tbl2 set col_no_ref = 'baz' where id = 1", + "plan": { + "QueryType": "UPDATE", + "Original": "update u_tbl2 set col_no_ref = 'baz' where id = 1", + "Instructions": { + "OperatorType": "Update", + "Variant": "Unsharded", + "Keyspace": { + "Name": "unsharded_fk_allow", + "Sharded": false + }, + "TargetTabletType": "PRIMARY", + "Query": "update u_tbl2 set col_no_ref = 'baz' where id = 1", + "Table": "u_tbl2" + }, + "TablesUsed": [ + "unsharded_fk_allow.u_tbl2" + ] + } } ] \ No newline at end of file diff --git a/go/vt/vtgate/planbuilder/testdata/vschemas/schema.json b/go/vt/vtgate/planbuilder/testdata/vschemas/schema.json index 52f1c8da7a8..d625001cabf 100644 --- a/go/vt/vtgate/planbuilder/testdata/vschemas/schema.json +++ b/go/vt/vtgate/planbuilder/testdata/vschemas/schema.json @@ -603,7 +603,7 @@ } } }, - "user_fk_allow": { + "sharded_fk_allow": { "sharded": true, "foreignKeyMode": "FK_MANAGED", "vindexes": { @@ -696,6 +696,17 @@ ] } } + }, + "unsharded_fk_allow": { + "foreignKeyMode": "FK_MANAGED", + "tables": { + "u_tbl1": {}, + "u_tbl2": {}, + "u_tbl3": {}, + "u_tbl4": {}, + "u_tbl5": {}, + "u_tbl6": {} + } } } } diff --git a/go/vt/vtgate/planbuilder/update.go b/go/vt/vtgate/planbuilder/update.go index 7813acb660f..ffa5c505886 100644 --- a/go/vt/vtgate/planbuilder/update.go +++ b/go/vt/vtgate/planbuilder/update.go @@ -18,6 +18,7 @@ package planbuilder import ( querypb "vitess.io/vitess/go/vt/proto/query" + vschemapb "vitess.io/vitess/go/vt/proto/vschema" "vitess.io/vitess/go/vt/sqlparser" "vitess.io/vitess/go/vt/vterrors" "vitess.io/vitess/go/vt/vtgate/engine" @@ -54,9 +55,11 @@ func gen4UpdateStmtPlanner( } if ks, tables := semTable.SingleUnshardedKeyspace(); ks != nil { - plan := updateUnshardedShortcut(updStmt, ks, tables) - plan = pushCommentDirectivesOnPlan(plan, updStmt) - return newPlanResult(plan.Primitive(), operators.QualifiedTables(ks, tables)...), nil + if fkManagementNotRequiredForUpdate(semTable, vschema, tables, updStmt.Exprs) { + plan := updateUnshardedShortcut(updStmt, ks, tables) + plan = pushCommentDirectivesOnPlan(plan, updStmt) + return newPlanResult(plan.Primitive(), operators.QualifiedTables(ks, tables)...), nil + } } if semTable.NotUnshardedErr != nil { @@ -91,6 +94,44 @@ func gen4UpdateStmtPlanner( return newPlanResult(plan.Primitive(), operators.TablesUsed(op)...), nil } +func fkManagementNotRequiredForUpdate(semTable *semantics.SemTable, vschema plancontext.VSchema, vTables []*vindexes.Table, updateExprs sqlparser.UpdateExprs) bool { + childFkMap := make(map[string][]vindexes.ChildFKInfo) + + // Find the foreign key mode and check for any managed child foreign keys. + for _, vTable := range vTables { + ksMode, err := vschema.ForeignKeyMode(vTable.Keyspace.Name) + if err != nil { + return false + } + if ksMode != vschemapb.Keyspace_FK_MANAGED { + continue + } + childFks := vTable.ChildFKsNeedsHandling(vindexes.UpdateAction) + if len(childFks) > 0 { + childFkMap[vTable.String()] = childFks + } + } + + // Check if any column in the parent table is being updated which has a child foreign key. + for _, updateExpr := range updateExprs { + tblInfo, err := semTable.TableInfoForExpr(updateExpr.Name) + if err != nil { + continue + } + vTable := tblInfo.GetVindexTable() + childFks, exists := childFkMap[vTable.String()] + if !exists { + continue + } + for _, childFk := range childFks { + if childFk.ParentColumns.FindColumn(updateExpr.Name.Name) >= 0 { + return false + } + } + } + return true +} + func updateUnshardedShortcut(stmt *sqlparser.Update, ks *vindexes.Keyspace, tables []*vindexes.Table) logicalPlan { edml := engine.NewDML() edml.Keyspace = ks diff --git a/go/vt/vtgate/vindexes/foreign_keys.go b/go/vt/vtgate/vindexes/foreign_keys.go index e18ed27ff25..6f13c56e7e9 100644 --- a/go/vt/vtgate/vindexes/foreign_keys.go +++ b/go/vt/vtgate/vindexes/foreign_keys.go @@ -134,7 +134,7 @@ func (t *Table) CrossShardParentFKs() (crossShardFKs []ParentFKInfo) { // ChildFKsNeedsHandling retuns the child foreign keys that needs to be handled by the vtgate. // This can be either the foreign key is not shard scoped or the child tables needs cascading. -func (t *Table) ChildFKsNeedsHandling() (fks []ChildFKInfo) { +func (t *Table) ChildFKsNeedsHandling(getAction func(fk ChildFKInfo) sqlparser.ReferenceAction) (fks []ChildFKInfo) { for _, fk := range t.ChildForeignKeys { // If the keyspaces are different, then the fk definition // is going to go across shards. @@ -143,7 +143,7 @@ func (t *Table) ChildFKsNeedsHandling() (fks []ChildFKInfo) { continue } // If the action is not Restrict, then it needs a cascade. - switch fk.OnDelete { + switch getAction(fk) { case sqlparser.Cascade, sqlparser.SetNull, sqlparser.SetDefault: fks = append(fks, fk) continue @@ -159,6 +159,9 @@ func (t *Table) ChildFKsNeedsHandling() (fks []ChildFKInfo) { return } +func UpdateAction(fk ChildFKInfo) sqlparser.ReferenceAction { return fk.OnUpdate } +func DeleteAction(fk ChildFKInfo) sqlparser.ReferenceAction { return fk.OnDelete } + func isShardScoped(pTable *Table, cTable *Table, pCols sqlparser.Columns, cCols sqlparser.Columns) bool { if !pTable.Keyspace.Sharded { return true diff --git a/go/vt/vtgate/vindexes/foreign_keys_test.go b/go/vt/vtgate/vindexes/foreign_keys_test.go index 8a21e6909d0..e1edbd34e30 100644 --- a/go/vt/vtgate/vindexes/foreign_keys_test.go +++ b/go/vt/vtgate/vindexes/foreign_keys_test.go @@ -243,9 +243,10 @@ func TestChildFKs(t *testing.T) { }, expChildTbls: []string{"t1"}, }} + deleteAction := func(fk ChildFKInfo) sqlparser.ReferenceAction { return fk.OnDelete } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - childFks := tt.table.ChildFKsNeedsHandling() + childFks := tt.table.ChildFKsNeedsHandling(deleteAction) var actualChildTbls []string for _, fk := range childFks { actualChildTbls = append(actualChildTbls, fk.Table.Name.String()) From aa4e9d6eb37be92e1a38cafdb64f568c30a64140 Mon Sep 17 00:00:00 2001 From: Harshit Gangal Date: Thu, 10 Aug 2023 23:36:55 +0530 Subject: [PATCH 2/5] if table vindex are shard scoped, all the foreign keys become shard scoped, hence can be ignored based on the statement type Signed-off-by: Harshit Gangal --- go/vt/vtgate/planbuilder/plan_test.go | 4 +- .../testdata/foreignkey_cases.json | 79 +++++++++++++++++++ go/vt/vtgate/vindexes/foreign_keys.go | 22 +++++- 3 files changed, 101 insertions(+), 4 deletions(-) diff --git a/go/vt/vtgate/planbuilder/plan_test.go b/go/vt/vtgate/planbuilder/plan_test.go index f192f1e14a5..c0af7850e82 100644 --- a/go/vt/vtgate/planbuilder/plan_test.go +++ b/go/vt/vtgate/planbuilder/plan_test.go @@ -118,11 +118,13 @@ func setFks(t *testing.T, vschema *vindexes.VSchema) { // FK from tbl2 referencing tbl1 that is shard scoped. _ = vschema.AddForeignKey("sharded_fk_allow", "tbl2", createFkDefinition([]string{"col2"}, "tbl1", []string{"col1"}, sqlparser.Restrict, sqlparser.Restrict)) // FK from tbl3 referencing tbl1 that is not shard scoped. - _ = vschema.AddForeignKey("sharded_fk_allow", "tbl3", createFkDefinition([]string{"coly"}, "tbl1", []string{"col1"}, sqlparser.DefaultAction, sqlparser.DefaultAction)) + _ = vschema.AddForeignKey("sharded_fk_allow", "tbl3", createFkDefinition([]string{"coly"}, "tbl1", []string{"t1col1"}, sqlparser.DefaultAction, sqlparser.DefaultAction)) // FK from tbl4 referencing tbl5 that is shard scoped. _ = vschema.AddForeignKey("sharded_fk_allow", "tbl4", createFkDefinition([]string{"col4"}, "tbl5", []string{"col5"}, sqlparser.SetNull, sqlparser.SetNull)) + _ = vschema.AddForeignKey("sharded_fk_allow", "tbl4", createFkDefinition([]string{"t4col4"}, "tbl5", []string{"t5col5"}, sqlparser.SetNull, sqlparser.SetNull)) // FK from tbl6 referencing tbl7 that is shard scoped. _ = vschema.AddForeignKey("sharded_fk_allow", "tbl6", createFkDefinition([]string{"col6"}, "tbl7", []string{"col7"}, sqlparser.NoAction, sqlparser.NoAction)) + _ = vschema.AddForeignKey("sharded_fk_allow", "tbl6", createFkDefinition([]string{"t6col6"}, "tbl7", []string{"t7col7"}, sqlparser.NoAction, sqlparser.NoAction)) } if vschema.Keyspaces["unsharded_fk_allow"] != nil { // u_tbl2(col2) -> u_tbl1(col1) Cascade. diff --git a/go/vt/vtgate/planbuilder/testdata/foreignkey_cases.json b/go/vt/vtgate/planbuilder/testdata/foreignkey_cases.json index de5797f296f..19336dc0fea 100644 --- a/go/vt/vtgate/planbuilder/testdata/foreignkey_cases.json +++ b/go/vt/vtgate/planbuilder/testdata/foreignkey_cases.json @@ -139,5 +139,84 @@ "unsharded_fk_allow.u_tbl2" ] } + }, + { + "comment": "Update in a table with cross-shard foreign keys disallowed", + "query": "update tbl1 set t1col1 = 'foo' where col1 = 1", + "plan": "VT12002: unsupported: foreign keys management at vitess" + }, + { + "comment": "Update in a table with cross-shard foreign keys, column not in update expression - allowed", + "query": "update tbl1 set not_ref_col = 'foo' where id = 1", + "plan": { + "QueryType": "UPDATE", + "Original": "update tbl1 set not_ref_col = 'foo' where id = 1", + "Instructions": { + "OperatorType": "Update", + "Variant": "Scatter", + "Keyspace": { + "Name": "sharded_fk_allow", + "Sharded": true + }, + "TargetTabletType": "PRIMARY", + "Query": "update tbl1 set not_ref_col = 'foo' where id = 1", + "Table": "tbl1" + }, + "TablesUsed": [ + "sharded_fk_allow.tbl1" + ] + } + }, + { + "comment": "Update in a table with shard-scoped foreign keys is allowed", + "query": "update tbl7 set t7col7 = 'foo'", + "plan": { + "QueryType": "UPDATE", + "Original": "update tbl7 set t7col7 = 'foo'", + "Instructions": { + "OperatorType": "Update", + "Variant": "Scatter", + "Keyspace": { + "Name": "sharded_fk_allow", + "Sharded": true + }, + "TargetTabletType": "PRIMARY", + "Query": "update tbl7 set t7col7 = 'foo'", + "Table": "tbl7" + }, + "TablesUsed": [ + "sharded_fk_allow.tbl7" + ] + } + }, + { + "comment": "Update in a table with shard-scoped foreign keys with cascade disallowed", + "query": "update tbl5 set t5col5 = 'foo'", + "plan": "VT12002: unsupported: foreign keys management at vitess" + }, + { + "comment": "Insertion in a table with 2 foreign keys constraint with same table on different columns", + "query": "insert into tbl6 (col6, t6col6) values (100, 'foo')", + "plan": { + "QueryType": "INSERT", + "Original": "insert into tbl6 (col6, t6col6) values (100, 'foo')", + "Instructions": { + "OperatorType": "Insert", + "Variant": "Sharded", + "Keyspace": { + "Name": "sharded_fk_allow", + "Sharded": true + }, + "TargetTabletType": "PRIMARY", + "Query": "insert into tbl6(col6, t6col6) values (:_col6_0, 'foo')", + "TableName": "tbl6", + "VindexValues": { + "hash_vin": "INT64(100)" + } + }, + "TablesUsed": [ + "sharded_fk_allow.tbl6" + ] + } } ] \ No newline at end of file diff --git a/go/vt/vtgate/vindexes/foreign_keys.go b/go/vt/vtgate/vindexes/foreign_keys.go index 6f13c56e7e9..697d6fa3d04 100644 --- a/go/vt/vtgate/vindexes/foreign_keys.go +++ b/go/vt/vtgate/vindexes/foreign_keys.go @@ -109,9 +109,8 @@ func (vschema *VSchema) AddForeignKey(ksname, childTableName string, fkConstrain // CrossShardParentFKs returns all the parent fk constraints on this table that are not shard scoped. func (t *Table) CrossShardParentFKs() (crossShardFKs []ParentFKInfo) { - if len(t.ParentForeignKeys) == 0 { - return - } + pTblShardScoped := make(map[string]bool) + var maybeShardScopedFks []ParentFKInfo for _, fk := range t.ParentForeignKeys { // If the keyspaces are different, then the fk definition // is going to go across shards. @@ -126,15 +125,25 @@ func (t *Table) CrossShardParentFKs() (crossShardFKs []ParentFKInfo) { } if !isShardScoped(fk.Table, t, fk.ParentColumns, fk.ChildColumns) { + maybeShardScopedFks = append(maybeShardScopedFks, fk) + continue + } + pTblShardScoped[fk.Table.Name.String()] = true + } + for _, fk := range maybeShardScopedFks { + if _, shardScoped := pTblShardScoped[fk.Table.Name.String()]; !shardScoped { crossShardFKs = append(crossShardFKs, fk) } } + return } // ChildFKsNeedsHandling retuns the child foreign keys that needs to be handled by the vtgate. // This can be either the foreign key is not shard scoped or the child tables needs cascading. func (t *Table) ChildFKsNeedsHandling(getAction func(fk ChildFKInfo) sqlparser.ReferenceAction) (fks []ChildFKInfo) { + cTblShardScoped := make(map[string]bool) + var restrictFks []ChildFKInfo for _, fk := range t.ChildForeignKeys { // If the keyspaces are different, then the fk definition // is going to go across shards. @@ -153,6 +162,13 @@ func (t *Table) ChildFKsNeedsHandling(getAction func(fk ChildFKInfo) sqlparser.R // do not allow modification if there is a child row. // Check if the restrict is shard scoped. if !isShardScoped(t, fk.Table, fk.ParentColumns, fk.ChildColumns) { + restrictFks = append(restrictFks, fk) + continue + } + cTblShardScoped[fk.Table.Name.String()] = true + } + for _, fk := range restrictFks { + if _, shardScoped := cTblShardScoped[fk.Table.Name.String()]; !shardScoped { fks = append(fks, fk) } } From 759abb2d31a6a862ba798ecada0dbdbc759758ff Mon Sep 17 00:00:00 2001 From: Harshit Gangal Date: Sat, 12 Aug 2023 00:09:42 +0530 Subject: [PATCH 3/5] fix logic for checking fk involvement, added update fk e2e test Signed-off-by: Harshit Gangal --- go/test/endtoend/vtgate/foreignkey/fk_test.go | 60 ++++++++++++---- .../vtgate/foreignkey/sharded_schema.sql | 35 ++++++++-- .../vtgate/foreignkey/sharded_vschema.json | 16 +++++ go/vt/vtgate/planbuilder/plan_test.go | 1 + .../testdata/foreignkey_cases.json | 68 ++----------------- go/vt/vtgate/vindexes/foreign_keys.go | 19 ------ 6 files changed, 102 insertions(+), 97 deletions(-) diff --git a/go/test/endtoend/vtgate/foreignkey/fk_test.go b/go/test/endtoend/vtgate/foreignkey/fk_test.go index 2478b525508..531aed2642a 100644 --- a/go/test/endtoend/vtgate/foreignkey/fk_test.go +++ b/go/test/endtoend/vtgate/foreignkey/fk_test.go @@ -19,13 +19,13 @@ package foreignkey import ( "testing" - "github.com/stretchr/testify/require" + "github.com/stretchr/testify/assert" "vitess.io/vitess/go/test/endtoend/utils" ) -// TestInsertions tests that insertions work as expected when foreign key management is enabled in Vitess. -func TestInsertions(t *testing.T) { +// TestInsertWithFK tests that insertions work as expected when foreign key management is enabled in Vitess. +func TestInsertWithFK(t *testing.T) { conn, closer := start(t) defer closer() @@ -37,11 +37,11 @@ func TestInsertions(t *testing.T) { // Verify that insertion fails if the data doesn't follow the fk constraint. _, err := utils.ExecAllowError(t, conn, `insert into t2(id, col) values (1310, 125)`) - require.ErrorContains(t, err, "Cannot add or update a child row: a foreign key constraint fails") + assert.ErrorContains(t, err, "Cannot add or update a child row: a foreign key constraint fails") // Verify that insertion fails if the table has cross-shard foreign keys (even if the data follows the constraints). _, err = utils.ExecAllowError(t, conn, `insert into t3(id, col) values (100, 100)`) - require.ErrorContains(t, err, "VT12002: unsupported: cross-shard foreign keys") + assert.ErrorContains(t, err, "VT12002: unsupported: cross-shard foreign keys") // insert some data in a table with multicol vindex. utils.Exec(t, conn, `insert into multicol_tbl1(cola, colb, colc, msg) values (100, 'a', 'b', 'msg'), (101, 'c', 'd', 'msg2')`) @@ -51,11 +51,11 @@ func TestInsertions(t *testing.T) { // Verify that insertion fails if the data doesn't follow the fk constraint. _, err = utils.ExecAllowError(t, conn, `insert into multicol_tbl2(cola, colb, colc, msg) values (103, 'c', 'd', 'msg2')`) - require.ErrorContains(t, err, "Cannot add or update a child row: a foreign key constraint fails") + assert.ErrorContains(t, err, "Cannot add or update a child row: a foreign key constraint fails") } -// TestDeletions tests that deletions work as expected when foreign key management is enabled in Vitess. -func TestDeletions(t *testing.T) { +// TestDeleteWithFK tests that deletions work as expected when foreign key management is enabled in Vitess. +func TestDeleteWithFK(t *testing.T) { conn, closer := start(t) defer closer() @@ -68,17 +68,53 @@ func TestDeletions(t *testing.T) { // child foreign key is shard scoped. Query will fail at mysql due to On Delete Restrict. _, err := utils.ExecAllowError(t, conn, `delete from t2 where col = 132`) - require.ErrorContains(t, err, "Cannot delete or update a parent row: a foreign key constraint fails") + assert.ErrorContains(t, err, "Cannot delete or update a parent row: a foreign key constraint fails") // child row does not exist so query will succeed. qr := utils.Exec(t, conn, `delete from t2 where col = 125`) - require.EqualValues(t, 1, qr.RowsAffected) + assert.EqualValues(t, 1, qr.RowsAffected) // table's child foreign key has cross shard fk, so query will fail at vtgate. _, err = utils.ExecAllowError(t, conn, `delete from t1 where id = 42`) - require.ErrorContains(t, err, "VT12002: unsupported: foreign keys management at vitess (errno 1235) (sqlstate 42000)") + assert.ErrorContains(t, err, "VT12002: unsupported: foreign keys management at vitess (errno 1235) (sqlstate 42000)") // child foreign key is cascade, so query will fail at vtgate. _, err = utils.ExecAllowError(t, conn, `delete from multicol_tbl1 where cola = 100`) - require.ErrorContains(t, err, "VT12002: unsupported: foreign keys management at vitess (errno 1235) (sqlstate 42000)") + assert.ErrorContains(t, err, "VT12002: unsupported: foreign keys management at vitess (errno 1235) (sqlstate 42000)") +} + +// TestUpdations tests that update work as expected when foreign key management is enabled in Vitess. +func TestUpdateWithFK(t *testing.T) { + conn, closer := start(t) + defer closer() + + // insert some data. + utils.Exec(t, conn, `insert into t1(id, col) values (100, 123),(10, 12),(1, 13),(1000, 1234)`) + utils.Exec(t, conn, `insert into t2(id, col, mycol) values (100, 125, 'foo'), (1, 132, 'bar')`) + utils.Exec(t, conn, `insert into t4(id, col, t2_mycol) values (1, 321, 'bar')`) + utils.Exec(t, conn, `insert into t5(pk, sk, col1) values (1, 1, 1),(2, 1, 1),(3, 1, 10),(4, 1, 20),(5, 1, 30),(6, 1, 40)`) + utils.Exec(t, conn, `insert into t6(pk, sk, col1) values (10, 1, 1), (20, 1, 20)`) + + // parent foreign key is shard scoped and value is not updated. Query will succeed. + _ = utils.Exec(t, conn, `update t4 set t2_mycol = 'bar' where id = 1`) + + // parent foreign key is shard scoped and value does not exists in parent table. Query will fail at mysql due to On Update Restrict. + _, err := utils.ExecAllowError(t, conn, `update t4 set t2_mycol = 'foo' where id = 1`) + assert.ErrorContains(t, err, "Cannot add or update a child row: a foreign key constraint fails") + + // updating column which does not have foreign key constraint, so query will succeed. + qr := utils.Exec(t, conn, `update t4 set col = 20 where id = 1`) + assert.EqualValues(t, 1, qr.RowsAffected) + + // child table have cascade which is cross shard. Query will fail at vtgate. + _, err = utils.ExecAllowError(t, conn, `update t2 set col = 125 where id = 100`) + assert.ErrorContains(t, err, "VT12002: unsupported: foreign keys management at vitess (errno 1235) (sqlstate 42000)") + + // updating column which does not have foreign key constraint, so query will succeed. + _ = utils.Exec(t, conn, `update t2 set mycol = 'baz' where id = 100`) + assert.EqualValues(t, 1, qr.RowsAffected) + + // child table have restrict in shard scoped and value exists in parent table. + _ = utils.Exec(t, conn, `update t6 set col1 = 40 where pk = 20`) + assert.EqualValues(t, 1, qr.RowsAffected) } diff --git a/go/test/endtoend/vtgate/foreignkey/sharded_schema.sql b/go/test/endtoend/vtgate/foreignkey/sharded_schema.sql index 37b9e5c15c6..8475104fecc 100644 --- a/go/test/endtoend/vtgate/foreignkey/sharded_schema.sql +++ b/go/test/endtoend/vtgate/foreignkey/sharded_schema.sql @@ -9,14 +9,17 @@ create table t2 ( id bigint, col bigint, + mycol varchar(50), primary key (id), + index(id, mycol), + index(id, col), foreign key (id) references t1 (id) on delete restrict ) Engine = InnoDB; create table t3 ( - id bigint, - col bigint, + id bigint, + col bigint, primary key (id), foreign key (col) references t1 (id) on delete restrict ) Engine = InnoDB; @@ -42,8 +45,30 @@ create table multicol_tbl2 create table t4 ( - id bigint, - col bigint, + id bigint, + col bigint, + t2_mycol varchar(50), + t2_col bigint, primary key (id), - foreign key (id) references t2 (id) on delete restrict + foreign key (id) references t2 (id) on delete restrict, + foreign key (id, t2_mycol) references t2 (id, mycol) on update restrict, + foreign key (id, t2_col) references t2 (id, col) on update cascade +) Engine = InnoDB; + +create table t5 +( + pk bigint, + sk bigint, + col1 varchar(50), + primary key (pk), + index(sk, col1) +) Engine = InnoDB; + +create table t6 +( + pk bigint, + sk bigint, + col1 varchar(50), + primary key (pk), + foreign key (sk, col1) references t5 (sk, col1) on delete restrict on update restrict ) Engine = InnoDB; diff --git a/go/test/endtoend/vtgate/foreignkey/sharded_vschema.json b/go/test/endtoend/vtgate/foreignkey/sharded_vschema.json index 04127774fd1..c80662b6852 100644 --- a/go/test/endtoend/vtgate/foreignkey/sharded_vschema.json +++ b/go/test/endtoend/vtgate/foreignkey/sharded_vschema.json @@ -47,6 +47,22 @@ } ] }, + "t5": { + "column_vindexes": [ + { + "column": "sk", + "name": "xxhash" + } + ] + }, + "t6": { + "column_vindexes": [ + { + "column": "sk", + "name": "xxhash" + } + ] + }, "multicol_tbl1": { "column_vindexes": [ { diff --git a/go/vt/vtgate/planbuilder/plan_test.go b/go/vt/vtgate/planbuilder/plan_test.go index c0af7850e82..10e8ce025a3 100644 --- a/go/vt/vtgate/planbuilder/plan_test.go +++ b/go/vt/vtgate/planbuilder/plan_test.go @@ -125,6 +125,7 @@ func setFks(t *testing.T, vschema *vindexes.VSchema) { // FK from tbl6 referencing tbl7 that is shard scoped. _ = vschema.AddForeignKey("sharded_fk_allow", "tbl6", createFkDefinition([]string{"col6"}, "tbl7", []string{"col7"}, sqlparser.NoAction, sqlparser.NoAction)) _ = vschema.AddForeignKey("sharded_fk_allow", "tbl6", createFkDefinition([]string{"t6col6"}, "tbl7", []string{"t7col7"}, sqlparser.NoAction, sqlparser.NoAction)) + _ = vschema.AddForeignKey("sharded_fk_allow", "tbl6", createFkDefinition([]string{"t6col62"}, "tbl7", []string{"t7col72"}, sqlparser.NoAction, sqlparser.NoAction)) } if vschema.Keyspaces["unsharded_fk_allow"] != nil { // u_tbl2(col2) -> u_tbl1(col1) Cascade. diff --git a/go/vt/vtgate/planbuilder/testdata/foreignkey_cases.json b/go/vt/vtgate/planbuilder/testdata/foreignkey_cases.json index 19336dc0fea..7f305dfb95a 100644 --- a/go/vt/vtgate/planbuilder/testdata/foreignkey_cases.json +++ b/go/vt/vtgate/planbuilder/testdata/foreignkey_cases.json @@ -60,26 +60,9 @@ "plan": "VT12002: unsupported: foreign keys management at vitess" }, { - "comment": "Delete in a table with shard-scoped foreign keys is allowed", + "comment": "Delete in a table with not all column shard-scoped foreign keys - disallowed", "query": "delete from tbl7", - "plan": { - "QueryType": "DELETE", - "Original": "delete from tbl7", - "Instructions": { - "OperatorType": "Delete", - "Variant": "Scatter", - "Keyspace": { - "Name": "sharded_fk_allow", - "Sharded": true - }, - "TargetTabletType": "PRIMARY", - "Query": "delete from tbl7", - "Table": "tbl7" - }, - "TablesUsed": [ - "sharded_fk_allow.tbl7" - ] - } + "plan": "VT12002: unsupported: foreign keys management at vitess" }, { "comment": "Delete in a table with shard-scoped multiple column foreign key with cascade not allowed", @@ -168,26 +151,9 @@ } }, { - "comment": "Update in a table with shard-scoped foreign keys is allowed", - "query": "update tbl7 set t7col7 = 'foo'", - "plan": { - "QueryType": "UPDATE", - "Original": "update tbl7 set t7col7 = 'foo'", - "Instructions": { - "OperatorType": "Update", - "Variant": "Scatter", - "Keyspace": { - "Name": "sharded_fk_allow", - "Sharded": true - }, - "TargetTabletType": "PRIMARY", - "Query": "update tbl7 set t7col7 = 'foo'", - "Table": "tbl7" - }, - "TablesUsed": [ - "sharded_fk_allow.tbl7" - ] - } + "comment": "Update in a table with column modified not shard-scoped foreign key whereas other column referencing same table is - disallowed", + "query": "update tbl7 set t7col7 = 'foo', t7col72 = 42", + "plan": "VT12002: unsupported: foreign keys management at vitess" }, { "comment": "Update in a table with shard-scoped foreign keys with cascade disallowed", @@ -195,28 +161,8 @@ "plan": "VT12002: unsupported: foreign keys management at vitess" }, { - "comment": "Insertion in a table with 2 foreign keys constraint with same table on different columns", + "comment": "Insertion in a table with 2 foreign keys constraint with same table on different columns - both are not shard scoped - disallowed", "query": "insert into tbl6 (col6, t6col6) values (100, 'foo')", - "plan": { - "QueryType": "INSERT", - "Original": "insert into tbl6 (col6, t6col6) values (100, 'foo')", - "Instructions": { - "OperatorType": "Insert", - "Variant": "Sharded", - "Keyspace": { - "Name": "sharded_fk_allow", - "Sharded": true - }, - "TargetTabletType": "PRIMARY", - "Query": "insert into tbl6(col6, t6col6) values (:_col6_0, 'foo')", - "TableName": "tbl6", - "VindexValues": { - "hash_vin": "INT64(100)" - } - }, - "TablesUsed": [ - "sharded_fk_allow.tbl6" - ] - } + "plan": "VT12002: unsupported: cross-shard foreign keys" } ] \ No newline at end of file diff --git a/go/vt/vtgate/vindexes/foreign_keys.go b/go/vt/vtgate/vindexes/foreign_keys.go index 697d6fa3d04..5abb0785669 100644 --- a/go/vt/vtgate/vindexes/foreign_keys.go +++ b/go/vt/vtgate/vindexes/foreign_keys.go @@ -109,8 +109,6 @@ func (vschema *VSchema) AddForeignKey(ksname, childTableName string, fkConstrain // CrossShardParentFKs returns all the parent fk constraints on this table that are not shard scoped. func (t *Table) CrossShardParentFKs() (crossShardFKs []ParentFKInfo) { - pTblShardScoped := make(map[string]bool) - var maybeShardScopedFks []ParentFKInfo for _, fk := range t.ParentForeignKeys { // If the keyspaces are different, then the fk definition // is going to go across shards. @@ -125,25 +123,15 @@ func (t *Table) CrossShardParentFKs() (crossShardFKs []ParentFKInfo) { } if !isShardScoped(fk.Table, t, fk.ParentColumns, fk.ChildColumns) { - maybeShardScopedFks = append(maybeShardScopedFks, fk) - continue - } - pTblShardScoped[fk.Table.Name.String()] = true - } - for _, fk := range maybeShardScopedFks { - if _, shardScoped := pTblShardScoped[fk.Table.Name.String()]; !shardScoped { crossShardFKs = append(crossShardFKs, fk) } } - return } // ChildFKsNeedsHandling retuns the child foreign keys that needs to be handled by the vtgate. // This can be either the foreign key is not shard scoped or the child tables needs cascading. func (t *Table) ChildFKsNeedsHandling(getAction func(fk ChildFKInfo) sqlparser.ReferenceAction) (fks []ChildFKInfo) { - cTblShardScoped := make(map[string]bool) - var restrictFks []ChildFKInfo for _, fk := range t.ChildForeignKeys { // If the keyspaces are different, then the fk definition // is going to go across shards. @@ -162,13 +150,6 @@ func (t *Table) ChildFKsNeedsHandling(getAction func(fk ChildFKInfo) sqlparser.R // do not allow modification if there is a child row. // Check if the restrict is shard scoped. if !isShardScoped(t, fk.Table, fk.ParentColumns, fk.ChildColumns) { - restrictFks = append(restrictFks, fk) - continue - } - cTblShardScoped[fk.Table.Name.String()] = true - } - for _, fk := range restrictFks { - if _, shardScoped := cTblShardScoped[fk.Table.Name.String()]; !shardScoped { fks = append(fks, fk) } } From 81b31ac062d14b89685daeb589f2ae2a9211e823 Mon Sep 17 00:00:00 2001 From: Harshit Gangal Date: Sat, 12 Aug 2023 00:40:07 +0530 Subject: [PATCH 4/5] refactor: merge update column fk detection logic Signed-off-by: Harshit Gangal --- go/vt/vtgate/planbuilder/operators/ast2op.go | 27 +++++++++++++------- go/vt/vtgate/planbuilder/update.go | 22 ++++++---------- 2 files changed, 26 insertions(+), 23 deletions(-) diff --git a/go/vt/vtgate/planbuilder/operators/ast2op.go b/go/vt/vtgate/planbuilder/operators/ast2op.go index 6be8feea537..b6a1b8da2ff 100644 --- a/go/vt/vtgate/planbuilder/operators/ast2op.go +++ b/go/vt/vtgate/planbuilder/operators/ast2op.go @@ -166,15 +166,11 @@ func createOperatorFromUpdate(ctx *plancontext.PlanningContext, updStmt *sqlpars } if ksMode == vschemapb.Keyspace_FK_MANAGED { childFks := vindexTable.ChildFKsNeedsHandling(vindexes.UpdateAction) - if len(childFks) > 0 { - // Check if any column in the parent table is being updated which has a child foreign key. - for _, updateExpr := range updStmt.Exprs { - for _, childFk := range childFks { - if childFk.ParentColumns.FindColumn(updateExpr.Name.Name) >= 0 { - return nil, vterrors.VT12003() - } - } - } + if len(childFks) > 0 && + ColumnModified(updStmt.Exprs, func(expr *sqlparser.UpdateExpr) []vindexes.ChildFKInfo { + return childFks + }) { + return nil, vterrors.VT12003() } } @@ -189,6 +185,19 @@ func createOperatorFromUpdate(ctx *plancontext.PlanningContext, updStmt *sqlpars return subq, nil } +// ColumnModified checks if any column in the parent table is being updated which has a child foreign key. +func ColumnModified(exprs sqlparser.UpdateExprs, getChildFks func(expr *sqlparser.UpdateExpr) []vindexes.ChildFKInfo) bool { + for _, updateExpr := range exprs { + childFks := getChildFks(updateExpr) + for _, childFk := range childFks { + if childFk.ParentColumns.FindColumn(updateExpr.Name.Name) >= 0 { + return true + } + } + } + return false +} + func createOperatorFromDelete(ctx *plancontext.PlanningContext, deleteStmt *sqlparser.Delete) (ops.Operator, error) { tableInfo, qt, err := createQueryTableForDML(ctx, deleteStmt.TableExprs[0], deleteStmt.Where) if err != nil { diff --git a/go/vt/vtgate/planbuilder/update.go b/go/vt/vtgate/planbuilder/update.go index ffa5c505886..73da2bcd167 100644 --- a/go/vt/vtgate/planbuilder/update.go +++ b/go/vt/vtgate/planbuilder/update.go @@ -94,6 +94,7 @@ func gen4UpdateStmtPlanner( return newPlanResult(plan.Primitive(), operators.TablesUsed(op)...), nil } +// TODO: Handle all this in semantic analysis. func fkManagementNotRequiredForUpdate(semTable *semantics.SemTable, vschema plancontext.VSchema, vTables []*vindexes.Table, updateExprs sqlparser.UpdateExprs) bool { childFkMap := make(map[string][]vindexes.ChildFKInfo) @@ -112,24 +113,17 @@ func fkManagementNotRequiredForUpdate(semTable *semantics.SemTable, vschema plan } } - // Check if any column in the parent table is being updated which has a child foreign key. - for _, updateExpr := range updateExprs { - tblInfo, err := semTable.TableInfoForExpr(updateExpr.Name) + getChildFKInfo := func(expr *sqlparser.UpdateExpr) []vindexes.ChildFKInfo { + tblInfo, err := semTable.TableInfoForExpr(expr.Name) if err != nil { - continue + return nil } vTable := tblInfo.GetVindexTable() - childFks, exists := childFkMap[vTable.String()] - if !exists { - continue - } - for _, childFk := range childFks { - if childFk.ParentColumns.FindColumn(updateExpr.Name.Name) >= 0 { - return false - } - } + return childFkMap[vTable.String()] } - return true + + // Check if any column in the parent table is being updated which has a child foreign key. + return !operators.ColumnModified(updateExprs, getChildFKInfo) } func updateUnshardedShortcut(stmt *sqlparser.Update, ks *vindexes.Keyspace, tables []*vindexes.Table) logicalPlan { From f552d4c8a55eb23ec15455f80b51b89d3806a082 Mon Sep 17 00:00:00 2001 From: Harshit Gangal Date: Mon, 14 Aug 2023 22:58:34 +0530 Subject: [PATCH 5/5] check parent foreign key for update column validity, added tests Signed-off-by: Harshit Gangal --- go/vt/vtgate/planbuilder/operators/ast2op.go | 18 ++++++++----- go/vt/vtgate/planbuilder/plan_test.go | 8 ++++++ .../testdata/foreignkey_cases.json | 27 +++++++++++++++++++ .../planbuilder/testdata/vschemas/schema.json | 8 ++++++ go/vt/vtgate/planbuilder/update.go | 8 +++--- go/vt/vtgate/vindexes/foreign_keys.go | 8 +++--- go/vt/vtgate/vindexes/foreign_keys_test.go | 2 +- 7 files changed, 64 insertions(+), 15 deletions(-) diff --git a/go/vt/vtgate/planbuilder/operators/ast2op.go b/go/vt/vtgate/planbuilder/operators/ast2op.go index b6a1b8da2ff..a4463035d67 100644 --- a/go/vt/vtgate/planbuilder/operators/ast2op.go +++ b/go/vt/vtgate/planbuilder/operators/ast2op.go @@ -165,10 +165,11 @@ func createOperatorFromUpdate(ctx *plancontext.PlanningContext, updStmt *sqlpars return nil, err } if ksMode == vschemapb.Keyspace_FK_MANAGED { + parentFKs := vindexTable.ParentFKsNeedsHandling() childFks := vindexTable.ChildFKsNeedsHandling(vindexes.UpdateAction) - if len(childFks) > 0 && - ColumnModified(updStmt.Exprs, func(expr *sqlparser.UpdateExpr) []vindexes.ChildFKInfo { - return childFks + if (len(childFks) > 0 || len(parentFKs) > 0) && + ColumnModified(updStmt.Exprs, func(expr *sqlparser.UpdateExpr) ([]vindexes.ParentFKInfo, []vindexes.ChildFKInfo) { + return parentFKs, childFks }) { return nil, vterrors.VT12003() } @@ -186,14 +187,19 @@ func createOperatorFromUpdate(ctx *plancontext.PlanningContext, updStmt *sqlpars } // ColumnModified checks if any column in the parent table is being updated which has a child foreign key. -func ColumnModified(exprs sqlparser.UpdateExprs, getChildFks func(expr *sqlparser.UpdateExpr) []vindexes.ChildFKInfo) bool { +func ColumnModified(exprs sqlparser.UpdateExprs, getFks func(expr *sqlparser.UpdateExpr) ([]vindexes.ParentFKInfo, []vindexes.ChildFKInfo)) bool { for _, updateExpr := range exprs { - childFks := getChildFks(updateExpr) + parentFKs, childFks := getFks(updateExpr) for _, childFk := range childFks { if childFk.ParentColumns.FindColumn(updateExpr.Name.Name) >= 0 { return true } } + for _, parentFk := range parentFKs { + if parentFk.ChildColumns.FindColumn(updateExpr.Name.Name) >= 0 { + return true + } + } } return false } @@ -306,7 +312,7 @@ func createOperatorFromInsert(ctx *plancontext.PlanningContext, ins *sqlparser.I return nil, err } if ksMode == vschemapb.Keyspace_FK_MANAGED { - parentFKs := vindexTable.CrossShardParentFKs() + parentFKs := vindexTable.ParentFKsNeedsHandling() if len(parentFKs) > 0 { return nil, vterrors.VT12002() } diff --git a/go/vt/vtgate/planbuilder/plan_test.go b/go/vt/vtgate/planbuilder/plan_test.go index 10e8ce025a3..dc91881352e 100644 --- a/go/vt/vtgate/planbuilder/plan_test.go +++ b/go/vt/vtgate/planbuilder/plan_test.go @@ -115,13 +115,21 @@ func setFks(t *testing.T, vschema *vindexes.VSchema) { if vschema.Keyspaces["sharded_fk_allow"] != nil { // FK from multicol_tbl2 referencing multicol_tbl1 that is shard scoped. _ = vschema.AddForeignKey("sharded_fk_allow", "multicol_tbl2", createFkDefinition([]string{"colb", "cola", "x", "colc", "y"}, "multicol_tbl1", []string{"colb", "cola", "y", "colc", "x"}, sqlparser.Cascade, sqlparser.Cascade)) + // FK from tbl2 referencing tbl1 that is shard scoped. _ = vschema.AddForeignKey("sharded_fk_allow", "tbl2", createFkDefinition([]string{"col2"}, "tbl1", []string{"col1"}, sqlparser.Restrict, sqlparser.Restrict)) + _ = vschema.AddForeignKey("sharded_fk_allow", "tbl2", createFkDefinition([]string{"col2", "col"}, "tbl1", []string{"col1", "col"}, sqlparser.Restrict, sqlparser.Restrict)) // FK from tbl3 referencing tbl1 that is not shard scoped. _ = vschema.AddForeignKey("sharded_fk_allow", "tbl3", createFkDefinition([]string{"coly"}, "tbl1", []string{"t1col1"}, sqlparser.DefaultAction, sqlparser.DefaultAction)) + // FK from tbl10 referencing tbl2 that is shard scoped. + _ = vschema.AddForeignKey("sharded_fk_allow", "tbl10", createFkDefinition([]string{"sk", "col"}, "tbl2", []string{"col2", "col"}, sqlparser.Restrict, sqlparser.Restrict)) + // FK from tbl10 referencing tbl3 that is not shard scoped. + _ = vschema.AddForeignKey("sharded_fk_allow", "tbl10", createFkDefinition([]string{"col"}, "tbl3", []string{"col"}, sqlparser.Restrict, sqlparser.Restrict)) + // FK from tbl4 referencing tbl5 that is shard scoped. _ = vschema.AddForeignKey("sharded_fk_allow", "tbl4", createFkDefinition([]string{"col4"}, "tbl5", []string{"col5"}, sqlparser.SetNull, sqlparser.SetNull)) _ = vschema.AddForeignKey("sharded_fk_allow", "tbl4", createFkDefinition([]string{"t4col4"}, "tbl5", []string{"t5col5"}, sqlparser.SetNull, sqlparser.SetNull)) + // FK from tbl6 referencing tbl7 that is shard scoped. _ = vschema.AddForeignKey("sharded_fk_allow", "tbl6", createFkDefinition([]string{"col6"}, "tbl7", []string{"col7"}, sqlparser.NoAction, sqlparser.NoAction)) _ = vschema.AddForeignKey("sharded_fk_allow", "tbl6", createFkDefinition([]string{"t6col6"}, "tbl7", []string{"t7col7"}, sqlparser.NoAction, sqlparser.NoAction)) diff --git a/go/vt/vtgate/planbuilder/testdata/foreignkey_cases.json b/go/vt/vtgate/planbuilder/testdata/foreignkey_cases.json index 7f305dfb95a..f316dd68e92 100644 --- a/go/vt/vtgate/planbuilder/testdata/foreignkey_cases.json +++ b/go/vt/vtgate/planbuilder/testdata/foreignkey_cases.json @@ -164,5 +164,32 @@ "comment": "Insertion in a table with 2 foreign keys constraint with same table on different columns - both are not shard scoped - disallowed", "query": "insert into tbl6 (col6, t6col6) values (100, 'foo')", "plan": "VT12002: unsupported: cross-shard foreign keys" + }, + { + "comment": "Update a table with parent and child foreign keys - shard scoped", + "query": "update tbl2 set col = 'foo'", + "plan": { + "QueryType": "UPDATE", + "Original": "update tbl2 set col = 'foo'", + "Instructions": { + "OperatorType": "Update", + "Variant": "Scatter", + "Keyspace": { + "Name": "sharded_fk_allow", + "Sharded": true + }, + "TargetTabletType": "PRIMARY", + "Query": "update tbl2 set col = 'foo'", + "Table": "tbl2" + }, + "TablesUsed": [ + "sharded_fk_allow.tbl2" + ] + } + }, + { + "comment": "update table with column's parent foreign key cross shard - disallowed", + "query": "update tbl10 set col = 'foo'", + "plan": "VT12002: unsupported: foreign keys management at vitess" } ] \ No newline at end of file diff --git a/go/vt/vtgate/planbuilder/testdata/vschemas/schema.json b/go/vt/vtgate/planbuilder/testdata/vschemas/schema.json index d625001cabf..0b474163ce5 100644 --- a/go/vt/vtgate/planbuilder/testdata/vschemas/schema.json +++ b/go/vt/vtgate/planbuilder/testdata/vschemas/schema.json @@ -694,6 +694,14 @@ "name": "hash_vin" } ] + }, + "tbl10": { + "column_vindexes": [ + { + "column": "sk", + "name": "hash_vin" + } + ] } } }, diff --git a/go/vt/vtgate/planbuilder/update.go b/go/vt/vtgate/planbuilder/update.go index 73da2bcd167..052a204cc1b 100644 --- a/go/vt/vtgate/planbuilder/update.go +++ b/go/vt/vtgate/planbuilder/update.go @@ -113,17 +113,17 @@ func fkManagementNotRequiredForUpdate(semTable *semantics.SemTable, vschema plan } } - getChildFKInfo := func(expr *sqlparser.UpdateExpr) []vindexes.ChildFKInfo { + getFKInfo := func(expr *sqlparser.UpdateExpr) ([]vindexes.ParentFKInfo, []vindexes.ChildFKInfo) { tblInfo, err := semTable.TableInfoForExpr(expr.Name) if err != nil { - return nil + return nil, nil } vTable := tblInfo.GetVindexTable() - return childFkMap[vTable.String()] + return vTable.ParentForeignKeys, childFkMap[vTable.String()] } // Check if any column in the parent table is being updated which has a child foreign key. - return !operators.ColumnModified(updateExprs, getChildFKInfo) + return !operators.ColumnModified(updateExprs, getFKInfo) } func updateUnshardedShortcut(stmt *sqlparser.Update, ks *vindexes.Keyspace, tables []*vindexes.Table) logicalPlan { diff --git a/go/vt/vtgate/vindexes/foreign_keys.go b/go/vt/vtgate/vindexes/foreign_keys.go index 5abb0785669..5029c1f7fe1 100644 --- a/go/vt/vtgate/vindexes/foreign_keys.go +++ b/go/vt/vtgate/vindexes/foreign_keys.go @@ -107,13 +107,13 @@ func (vschema *VSchema) AddForeignKey(ksname, childTableName string, fkConstrain return nil } -// CrossShardParentFKs returns all the parent fk constraints on this table that are not shard scoped. -func (t *Table) CrossShardParentFKs() (crossShardFKs []ParentFKInfo) { +// ParentFKsNeedsHandling returns all the parent fk constraints on this table that are not shard scoped. +func (t *Table) ParentFKsNeedsHandling() (fks []ParentFKInfo) { for _, fk := range t.ParentForeignKeys { // If the keyspaces are different, then the fk definition // is going to go across shards. if fk.Table.Keyspace.Name != t.Keyspace.Name { - crossShardFKs = append(crossShardFKs, fk) + fks = append(fks, fk) continue } // If the keyspaces match and they are unsharded, then the fk defintion @@ -123,7 +123,7 @@ func (t *Table) CrossShardParentFKs() (crossShardFKs []ParentFKInfo) { } if !isShardScoped(fk.Table, t, fk.ParentColumns, fk.ChildColumns) { - crossShardFKs = append(crossShardFKs, fk) + fks = append(fks, fk) } } return diff --git a/go/vt/vtgate/vindexes/foreign_keys_test.go b/go/vt/vtgate/vindexes/foreign_keys_test.go index e1edbd34e30..b5dfa57fbce 100644 --- a/go/vt/vtgate/vindexes/foreign_keys_test.go +++ b/go/vt/vtgate/vindexes/foreign_keys_test.go @@ -137,7 +137,7 @@ func TestTable_CrossShardParentFKs(t *testing.T) { }} for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - crossShardFks := tt.table.CrossShardParentFKs() + crossShardFks := tt.table.ParentFKsNeedsHandling() var crossShardFkTables []string for _, fk := range crossShardFks { crossShardFkTables = append(crossShardFkTables, fk.Table.Name.String())