From 03f708657b158e7657a3ffc3a339123cb90e63a1 Mon Sep 17 00:00:00 2001 From: Manan Gupta Date: Tue, 1 Aug 2023 17:02:38 +0530 Subject: [PATCH 01/12] feat: add operator planning for foreign keys in inserts Signed-off-by: Manan Gupta --- go/vt/sqlparser/ast_funcs.go | 20 +++ go/vt/sqlparser/normalizer_test.go | 28 ++++ go/vt/vtgate/planbuilder/ddl.go | 11 +- go/vt/vtgate/planbuilder/operators/ast2op.go | 16 ++ .../vtgate/planbuilder/operators/fk_verify.go | 117 +++++++++++++++ go/vt/vtgate/planbuilder/operators/insert.go | 5 + go/vt/vtgate/planbuilder/plan_test.go | 4 +- .../vtgate/planbuilder/plancontext/vschema.go | 2 +- go/vt/vtgate/vcursor_impl.go | 11 +- go/vt/vtgate/vindexes/foreign_keys.go | 138 ++++++++++++++++++ go/vt/vtgate/vindexes/vschema.go | 84 ----------- 11 files changed, 346 insertions(+), 90 deletions(-) create mode 100644 go/vt/vtgate/planbuilder/operators/fk_verify.go create mode 100644 go/vt/vtgate/vindexes/foreign_keys.go diff --git a/go/vt/sqlparser/ast_funcs.go b/go/vt/sqlparser/ast_funcs.go index 1d7ab540c12..72713723575 100644 --- a/go/vt/sqlparser/ast_funcs.go +++ b/go/vt/sqlparser/ast_funcs.go @@ -2494,3 +2494,23 @@ func (ty KillType) ToString() string { return ConnectionStr } } + +// Indexes returns if the list of columns contains all the elements in the other list provided. +// If it does, then it also returns the indexes of the columns. +func (cols Columns) Indexes(subSetCols Columns) (bool, []int) { + var indexes []int + for _, subSetCol := range subSetCols { + colFound := false + for idx, col := range cols { + if col.Equal(subSetCol) { + colFound = true + indexes = append(indexes, idx) + break + } + } + if !colFound { + return false, nil + } + } + return true, indexes +} diff --git a/go/vt/sqlparser/normalizer_test.go b/go/vt/sqlparser/normalizer_test.go index 8e40dfe9f1a..c4568ba3de6 100644 --- a/go/vt/sqlparser/normalizer_test.go +++ b/go/vt/sqlparser/normalizer_test.go @@ -436,6 +436,34 @@ func TestNormalizeValidSQL(t *testing.T) { } } +func TestNormalizeOneCasae(t *testing.T) { + testOne := struct { + input, output string + }{ + input: "select count(distinct id, name) from area where (id, name) in ((1, 'US'))", + output: "", + } + if testOne.input == "" { + return + } + tree, err := Parse(testOne.input) + require.NoError(t, err, testOne.input) + // Skip the test for the queries that do not run the normalizer + if !CanNormalize(tree) { + return + } + bv := make(map[string]*querypb.BindVariable) + known := make(BindVars) + err = Normalize(tree, NewReservedVars("vtg", known), bv) + require.NoError(t, err) + normalizerOutput := String(tree) + if normalizerOutput == "otheradmin" || normalizerOutput == "otherread" { + return + } + _, err = Parse(normalizerOutput) + require.NoError(t, err, normalizerOutput) +} + func TestGetBindVars(t *testing.T) { stmt, err := Parse("select * from t where :v1 = :v2 and :v2 = :v3 and :v4 in ::v5") if err != nil { diff --git a/go/vt/vtgate/planbuilder/ddl.go b/go/vt/vtgate/planbuilder/ddl.go index 871b8adc38a..d84e5341294 100644 --- a/go/vt/vtgate/planbuilder/ddl.go +++ b/go/vt/vtgate/planbuilder/ddl.go @@ -5,6 +5,7 @@ import ( "fmt" "vitess.io/vitess/go/vt/key" + 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" @@ -162,7 +163,15 @@ func buildDDLPlans(ctx context.Context, sql string, ddlStatement sqlparser.DDLSt } func checkFKError(vschema plancontext.VSchema, ddlStatement sqlparser.DDLStatement) error { - if fkStrategyMap[vschema.ForeignKeyMode()] == fkDisallow { + _, ks, _, err := vschema.TargetDestination(ddlStatement.GetTable().Qualifier.String()) + if err != nil { + return err + } + fkMode, err := vschema.ForeignKeyMode(ks.Name) + if err != nil { + return err + } + if fkMode == vschemapb.Keyspace_FK_DISALLOW { fk := &fkContraint{} _ = sqlparser.Walk(fk.FkWalk, ddlStatement) if fk.found { diff --git a/go/vt/vtgate/planbuilder/operators/ast2op.go b/go/vt/vtgate/planbuilder/operators/ast2op.go index 59223f0e631..a80033de23d 100644 --- a/go/vt/vtgate/planbuilder/operators/ast2op.go +++ b/go/vt/vtgate/planbuilder/operators/ast2op.go @@ -20,6 +20,7 @@ import ( "fmt" "strconv" + 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" @@ -260,6 +261,21 @@ func createOperatorFromInsert(ctx *plancontext.PlanningContext, ins *sqlparser.I Routing: routing, } + // Find the foreign key mode and store the ParentFKs that we need to verify. + ksMode, err := ctx.VSchema.ForeignKeyMode(vindexTable.Keyspace.Name) + if err != nil { + return nil, err + } + if ksMode == vschemapb.Keyspace_FK_MANAGED { + parentFKs := vindexTable.CrossShardParentFKs() + if len(parentFKs) > 0 { + insOp.FKVerify, err = NewFkVerify(ctx, parentFKs, ins.Columns) + if err != nil { + return nil, err + } + } + } + // Table column list is nil then add all the columns // If the column list is empty then add only the auto-inc column and // this happens on calling modifyForAutoinc diff --git a/go/vt/vtgate/planbuilder/operators/fk_verify.go b/go/vt/vtgate/planbuilder/operators/fk_verify.go new file mode 100644 index 00000000000..ab4dc449b56 --- /dev/null +++ b/go/vt/vtgate/planbuilder/operators/fk_verify.go @@ -0,0 +1,117 @@ +package operators + +import ( + "vitess.io/vitess/go/vt/sqlparser" + "vitess.io/vitess/go/vt/vtgate/planbuilder/operators/ops" + "vitess.io/vitess/go/vt/vtgate/planbuilder/plancontext" + "vitess.io/vitess/go/vt/vtgate/vindexes" +) + +// FKVerify represents an insert operation on a table. +type FKVerify struct { + // Sources are used to run the validation queries. + Sources []ops.Operator + InputOffsets [][]int + + // ParentFKs to verify during insert planning + ParentFKs []vindexes.ParentFKInfo + + noColumns + noPredicates +} + +func (fkVerify *FKVerify) Inputs() []ops.Operator { + return fkVerify.Sources +} + +func (fkVerify *FKVerify) SetInputs(operators []ops.Operator) { + fkVerify.Sources = operators +} + +func (fkVerify *FKVerify) ShortDescription() string { + return "FKVerify" +} + +func (fkVerify *FKVerify) GetOrdering() ([]ops.OrderBy, error) { + panic("does not expect insert operator to receive get ordering call") +} + +// Clone will return a copy of this operator, protected so changed to the original will not impact the clone +func (fkVerify *FKVerify) Clone(inputs []ops.Operator) ops.Operator { + return &FKVerify{ + Sources: inputs, + } +} + +// NewFkVerify creates a new FkVerify operator given the parent foreign key validations we want it to run. +func NewFkVerify(ctx *plancontext.PlanningContext, parentFKs []vindexes.ParentFKInfo, insertColumns sqlparser.Columns) (*FKVerify, error) { + var verifyQueryOps []ops.Operator + var inputOffsets [][]int + for _, fk := range parentFKs { + // Build the query for verification + query := &sqlparser.Select{ + SelectExprs: []sqlparser.SelectExpr{ + sqlparser.NewAliasedExpr( + &sqlparser.Count{ + Args: createColNameList(fk.ParentColumns), + }, + "", + ), + }, + From: []sqlparser.TableExpr{ + sqlparser.NewAliasedTableExpr(sqlparser.NewTableName(fk.Table.Name.String()), ""), + }, + Where: sqlparser.NewWhere( + sqlparser.WhereClause, + sqlparser.NewComparisonExpr( + sqlparser.InOp, + createLeftExpr(fk.ParentColumns), + sqlparser.NewListArg("fkInput"), // TODO: Make this a constant in the engine package. + nil, + ), + ), + } + op, err := createOperatorFromSelect(ctx, query) + if err != nil { + return nil, err + } + verifyQueryOps = append(verifyQueryOps, op) + + // Build the offsets + var offsets []int + outer: + for _, column := range fk.ParentColumns { + for idx, insertColumn := range insertColumns { + if column.Equal(insertColumn) { + offsets = append(offsets, idx) + goto outer + } + } + offsets = append(offsets, -1) + } + inputOffsets = append(inputOffsets, offsets) + } + + return &FKVerify{ + Sources: verifyQueryOps, + InputOffsets: inputOffsets, + }, nil +} + +// createColNameList creates a list of ColNames from a list of Columns +func createColNameList(columns sqlparser.Columns) []sqlparser.Expr { + var colNames []sqlparser.Expr + for _, col := range columns { + colNames = append(colNames, sqlparser.NewColName(col.String())) + } + return colNames +} + +// createLeftExpr creates a valtuple if there are more than 1 columns, otherwise it just returns a ColName. +func createLeftExpr(columns sqlparser.Columns) sqlparser.Expr { + valTuple := createColNameList(columns) + if len(valTuple) == 1 { + return valTuple[0] + } + return sqlparser.ValTuple(valTuple) +} diff --git a/go/vt/vtgate/planbuilder/operators/insert.go b/go/vt/vtgate/planbuilder/operators/insert.go index 3fc70ed8998..40fea4f22d5 100644 --- a/go/vt/vtgate/planbuilder/operators/insert.go +++ b/go/vt/vtgate/planbuilder/operators/insert.go @@ -50,6 +50,10 @@ type Insert struct { // Insert using select query will have select plan as input operator for the insert operation. Input ops.Operator + // FKVerify is required to check the validity of foreign key constraints that aren't shard scoped. + // It is nil if no verification is required. + FKVerify ops.Operator + noColumns noPredicates } @@ -111,6 +115,7 @@ func (i *Insert) Clone(inputs []ops.Operator) ops.Operator { ColVindexes: i.ColVindexes, VindexValues: i.VindexValues, VindexValueOffset: i.VindexValueOffset, + FKVerify: i.FKVerify, } } diff --git a/go/vt/vtgate/planbuilder/plan_test.go b/go/vt/vtgate/planbuilder/plan_test.go index 755a5fbf916..f85c03c1b4e 100644 --- a/go/vt/vtgate/planbuilder/plan_test.go +++ b/go/vt/vtgate/planbuilder/plan_test.go @@ -507,8 +507,8 @@ func (vw *vschemaWrapper) ConnCollation() collations.ID { func (vw *vschemaWrapper) PlannerWarning(_ string) { } -func (vw *vschemaWrapper) ForeignKeyMode() string { - return "allow" +func (vw *vschemaWrapper) ForeignKeyMode(keyspace string) (vschemapb.Keyspace_ForeignKeyMode, error) { + return vschemapb.Keyspace_FK_UNMANAGED, nil } func (vw *vschemaWrapper) AllKeyspace() ([]*vindexes.Keyspace, error) { diff --git a/go/vt/vtgate/planbuilder/plancontext/vschema.go b/go/vt/vtgate/planbuilder/plancontext/vschema.go index f288a9bd95c..fc5ee6d9207 100644 --- a/go/vt/vtgate/planbuilder/plancontext/vschema.go +++ b/go/vt/vtgate/planbuilder/plancontext/vschema.go @@ -55,7 +55,7 @@ type VSchema interface { PlannerWarning(message string) // ForeignKeyMode returns the foreign_key flag value - ForeignKeyMode() string + ForeignKeyMode(keyspace string) (vschemapb.Keyspace_ForeignKeyMode, error) // GetVSchema returns the latest cached vindexes.VSchema GetVSchema() *vindexes.VSchema diff --git a/go/vt/vtgate/vcursor_impl.go b/go/vt/vtgate/vcursor_impl.go index 3d4778d430d..f36de3d87ae 100644 --- a/go/vt/vtgate/vcursor_impl.go +++ b/go/vt/vtgate/vcursor_impl.go @@ -1011,8 +1011,15 @@ func (vc *vcursorImpl) PlannerWarning(message string) { } // ForeignKeyMode implements the VCursor interface -func (vc *vcursorImpl) ForeignKeyMode() string { - return strings.ToLower(foreignKeyMode) +func (vc *vcursorImpl) ForeignKeyMode(keyspace string) (vschemapb.Keyspace_ForeignKeyMode, error) { + if strings.ToLower(foreignKeyMode) == "disallow" { + return vschemapb.Keyspace_FK_DISALLOW, nil + } + ks := vc.vschema.Keyspaces[keyspace] + if ks == nil { + return 0, vterrors.VT14004(keyspace) + } + return ks.ForeignKeyMode, nil } // ParseDestinationTarget parses destination target string and sets default keyspace if possible. diff --git a/go/vt/vtgate/vindexes/foreign_keys.go b/go/vt/vtgate/vindexes/foreign_keys.go new file mode 100644 index 00000000000..c749cd948d6 --- /dev/null +++ b/go/vt/vtgate/vindexes/foreign_keys.go @@ -0,0 +1,138 @@ +package vindexes + +import ( + "encoding/json" + "fmt" + + "vitess.io/vitess/go/vt/sqlparser" +) + +// ParentFKInfo contains the parent foreign key info for the table. +type ParentFKInfo struct { + Table *Table + ParentColumns sqlparser.Columns + ChildColumns sqlparser.Columns +} + +// MarshalJSON returns a JSON representation of ParentFKInfo. +func (fk *ParentFKInfo) MarshalJSON() ([]byte, error) { + return json.Marshal(struct { + Name string `json:"parent_table"` + ParentColumns sqlparser.Columns `json:"parent_columns"` + ChildColumns sqlparser.Columns `json:"child_columns"` + }{ + Name: fk.Table.Name.String(), + ChildColumns: fk.ChildColumns, + ParentColumns: fk.ParentColumns, + }) +} + +// NewParentFkInfo creates a new ParentFKInfo. +func NewParentFkInfo(parentTbl *Table, fkDef *sqlparser.ForeignKeyDefinition) ParentFKInfo { + return ParentFKInfo{ + Table: parentTbl, + ChildColumns: fkDef.Source, + ParentColumns: fkDef.ReferenceDefinition.ReferencedColumns, + } +} + +// ChildFKInfo contains the child foreign key info for the table. +type ChildFKInfo struct { + Table *Table + ChildColumns sqlparser.Columns + ParentColumns sqlparser.Columns + Match sqlparser.MatchAction + OnDelete sqlparser.ReferenceAction + OnUpdate sqlparser.ReferenceAction +} + +// MarshalJSON returns a JSON representation of ChildFKInfo. +func (fk *ChildFKInfo) MarshalJSON() ([]byte, error) { + return json.Marshal(struct { + Name string `json:"child_table"` + ChildColumns sqlparser.Columns `json:"child_columns"` + ParentColumns sqlparser.Columns `json:"parent_columns"` + }{ + Name: fk.Table.Name.String(), + ChildColumns: fk.ChildColumns, + ParentColumns: fk.ParentColumns, + }) +} + +// NewChildFkInfo creates a new ChildFKInfo. +func NewChildFkInfo(childTbl *Table, fkDef *sqlparser.ForeignKeyDefinition) ChildFKInfo { + return ChildFKInfo{ + Table: childTbl, + ChildColumns: fkDef.Source, + ParentColumns: fkDef.ReferenceDefinition.ReferencedColumns, + Match: fkDef.ReferenceDefinition.Match, + OnDelete: fkDef.ReferenceDefinition.OnDelete, + OnUpdate: fkDef.ReferenceDefinition.OnUpdate, + } +} + +// addForeignKey is for testing only. +func (vschema *VSchema) addForeignKey(ksname, childTableName string, fkConstraint *sqlparser.ForeignKeyDefinition) error { + ks, ok := vschema.Keyspaces[ksname] + if !ok { + return fmt.Errorf("keyspace %s not found in vschema", ksname) + } + cTbl, ok := ks.Tables[childTableName] + if !ok { + return fmt.Errorf("child table %s not found in keyspace %s", childTableName, ksname) + } + parentTableName := fkConstraint.ReferenceDefinition.ReferencedTable.Name.String() + pTbl, ok := ks.Tables[parentTableName] + if !ok { + return fmt.Errorf("parent table %s not found in keyspace %s", parentTableName, ksname) + } + pTbl.ChildForeignKeys = append(pTbl.ChildForeignKeys, NewChildFkInfo(cTbl, fkConstraint)) + cTbl.ParentForeignKeys = append(cTbl.ParentForeignKeys, NewParentFkInfo(pTbl, fkConstraint)) + return nil +} + +// CrossShardParentFKs returns all the parent fk constraints on this table that are not shard scoped. +func (t *Table) CrossShardParentFKs() (crossShardFKs []ParentFKInfo) { + if !t.Keyspace.Sharded || len(t.ParentForeignKeys) == 0 { + return + } + primaryVindex := t.ColumnVindexes[0] + for _, fk := range t.ParentForeignKeys { + // If the primary vindexes don't match between the parent and child table, + // we cannot infer that the fk constraint in shard scoped. + if fk.Table.ColumnVindexes[0].Vindex != primaryVindex.Vindex { + crossShardFKs = append(crossShardFKs, fk) + continue + } + + childFkContatined, childFkIndexes := fk.ChildColumns.Indexes(primaryVindex.Columns) + if !childFkContatined { + // PrimaryVindex is not part of the foreign key constraint on the children side. + // So it is a cross-shard foreign key. + crossShardFKs = append(crossShardFKs, fk) + continue + } + + // We need to run the same check for the parent columns. + parentFkContatined, parentFkIndexes := fk.ParentColumns.Indexes(fk.Table.ColumnVindexes[0].Columns) + if !parentFkContatined { + crossShardFKs = append(crossShardFKs, fk) + continue + } + + // Both the child and parent table contain the foreign key and that the vindexes are the same, + // now we need to make sure, that the indexes of both match. + // For example, consider the following tables, + // t1 (primary vindex (x,y)) + // t2 (primary vindex (a,b)) + // If we have a foreign key constraint from t1(x,y) to t2(b,a), then they are not shard scoped. + // Let's say in t1, (1,3) will be in -80 and (3,1) will be in 80-, then in t2 (1,3) will end up in 80-. + for i := range parentFkIndexes { + if parentFkIndexes[i] != childFkIndexes[i] { + crossShardFKs = append(crossShardFKs, fk) + break + } + } + } + return +} diff --git a/go/vt/vtgate/vindexes/vschema.go b/go/vt/vtgate/vindexes/vschema.go index a7e1b2fe2ad..6edd97aeeb5 100644 --- a/go/vt/vtgate/vindexes/vschema.go +++ b/go/vt/vtgate/vindexes/vschema.go @@ -139,70 +139,6 @@ type ColumnVindex struct { backfill bool } -// ParentFKInfo contains the parent foreign key info for the table. -type ParentFKInfo struct { - Table *Table - ParentColumns sqlparser.Columns - ChildColumns sqlparser.Columns -} - -// MarshalJSON returns a JSON representation of ParentFKInfo. -func (fk *ParentFKInfo) MarshalJSON() ([]byte, error) { - return json.Marshal(struct { - Name string `json:"parent_table"` - ParentColumns sqlparser.Columns `json:"parent_columns"` - ChildColumns sqlparser.Columns `json:"child_columns"` - }{ - Name: fk.Table.Name.String(), - ChildColumns: fk.ChildColumns, - ParentColumns: fk.ParentColumns, - }) -} - -// NewParentFkInfo creates a new ParentFKInfo. -func NewParentFkInfo(parentTbl *Table, fkDef *sqlparser.ForeignKeyDefinition) ParentFKInfo { - return ParentFKInfo{ - Table: parentTbl, - ChildColumns: fkDef.Source, - ParentColumns: fkDef.ReferenceDefinition.ReferencedColumns, - } -} - -// ChildFKInfo contains the child foreign key info for the table. -type ChildFKInfo struct { - Table *Table - ChildColumns sqlparser.Columns - ParentColumns sqlparser.Columns - Match sqlparser.MatchAction - OnDelete sqlparser.ReferenceAction - OnUpdate sqlparser.ReferenceAction -} - -// MarshalJSON returns a JSON representation of ChildFKInfo. -func (fk *ChildFKInfo) MarshalJSON() ([]byte, error) { - return json.Marshal(struct { - Name string `json:"child_table"` - ChildColumns sqlparser.Columns `json:"child_columns"` - ParentColumns sqlparser.Columns `json:"parent_columns"` - }{ - Name: fk.Table.Name.String(), - ChildColumns: fk.ChildColumns, - ParentColumns: fk.ParentColumns, - }) -} - -// NewChildFkInfo creates a new ChildFKInfo. -func NewChildFkInfo(childTbl *Table, fkDef *sqlparser.ForeignKeyDefinition) ChildFKInfo { - return ChildFKInfo{ - Table: childTbl, - ChildColumns: fkDef.Source, - ParentColumns: fkDef.ReferenceDefinition.ReferencedColumns, - Match: fkDef.ReferenceDefinition.Match, - OnDelete: fkDef.ReferenceDefinition.OnDelete, - OnUpdate: fkDef.ReferenceDefinition.OnUpdate, - } -} - // TableInfo contains column and foreign key info for a table. type TableInfo struct { Columns []Column @@ -402,26 +338,6 @@ func replaceDefaultForeignKeyMode(fkMode vschemapb.Keyspace_ForeignKeyMode) vsch return fkMode } -// addForeignKey is for testing only. -func (vschema *VSchema) addForeignKey(ksname, childTableName string, fkConstraint *sqlparser.ForeignKeyDefinition) error { - ks, ok := vschema.Keyspaces[ksname] - if !ok { - return fmt.Errorf("keyspace %s not found in vschema", ksname) - } - cTbl, ok := ks.Tables[childTableName] - if !ok { - return fmt.Errorf("child table %s not found in keyspace %s", childTableName, ksname) - } - parentTableName := fkConstraint.ReferenceDefinition.ReferencedTable.Name.String() - pTbl, ok := ks.Tables[parentTableName] - if !ok { - return fmt.Errorf("parent table %s not found in keyspace %s", parentTableName, ksname) - } - pTbl.ChildForeignKeys = append(pTbl.ChildForeignKeys, NewChildFkInfo(cTbl, fkConstraint)) - cTbl.ParentForeignKeys = append(cTbl.ParentForeignKeys, NewParentFkInfo(pTbl, fkConstraint)) - return nil -} - func (vschema *VSchema) AddView(ksname string, viewName, query string) error { ks, ok := vschema.Keyspaces[ksname] if !ok { From 41bff3f57658119a3ef57cc24a947cbab434ae5f Mon Sep 17 00:00:00 2001 From: Manan Gupta Date: Tue, 1 Aug 2023 17:18:42 +0530 Subject: [PATCH 02/12] feat: fail logical operator conversion with error Signed-off-by: Manan Gupta --- go/vt/vterrors/code.go | 1 + go/vt/vtgate/planbuilder/insert.go | 5 +++-- go/vt/vtgate/planbuilder/operator_transformers.go | 10 ++++++++++ go/vt/vtgate/planbuilder/operators/fk_verify.go | 2 +- 4 files changed, 15 insertions(+), 3 deletions(-) diff --git a/go/vt/vterrors/code.go b/go/vt/vterrors/code.go index 2afccb561e2..a77446039db 100644 --- a/go/vt/vterrors/code.go +++ b/go/vt/vterrors/code.go @@ -80,6 +80,7 @@ var ( VT10001 = errorWithoutState("VT10001", vtrpcpb.Code_ABORTED, "foreign key constraints are not allowed", "Foreign key constraints are not allowed, see https://vitess.io/blog/2021-06-15-online-ddl-why-no-fk/.") VT12001 = errorWithoutState("VT12001", vtrpcpb.Code_UNIMPLEMENTED, "unsupported: %s", "This statement is unsupported by Vitess. Please rewrite your query to use supported syntax.") + VT12002 = errorWithoutState("VT12002", vtrpcpb.Code_UNIMPLEMENTED, "unsupported cross-shard foreign keys", "Vitess does not support cross shard foreign key management.") // VT13001 General Error VT13001 = errorWithoutState("VT13001", vtrpcpb.Code_INTERNAL, "[BUG] %s", "This error should not happen and is a bug. Please file an issue on GitHub: https://github.com/vitessio/vitess/issues/new/choose.") diff --git a/go/vt/vtgate/planbuilder/insert.go b/go/vt/vtgate/planbuilder/insert.go index 864d1056908..22de52afa6a 100644 --- a/go/vt/vtgate/planbuilder/insert.go +++ b/go/vt/vtgate/planbuilder/insert.go @@ -101,8 +101,9 @@ func insertUnshardedShortcut(stmt *sqlparser.Insert, ks *vindexes.Keyspace, tabl } type insert struct { - eInsert *engine.Insert - source logicalPlan + eInsert *engine.Insert + source logicalPlan + fkVerify logicalPlan } var _ logicalPlan = (*insert)(nil) diff --git a/go/vt/vtgate/planbuilder/operator_transformers.go b/go/vt/vtgate/planbuilder/operator_transformers.go index c0fab8a820c..120368c1e36 100644 --- a/go/vt/vtgate/planbuilder/operator_transformers.go +++ b/go/vt/vtgate/planbuilder/operator_transformers.go @@ -66,6 +66,8 @@ func transformToLogicalPlan(ctx *plancontext.PlanningContext, op ops.Operator, i return transformAggregator(ctx, op) case *operators.Distinct: return transformDistinct(ctx, op) + case *operators.FKVerify: + return nil, vterrors.VT12002() } return nil, vterrors.VT13001(fmt.Sprintf("unknown type encountered: %T (transformToLogicalPlan)", op)) @@ -426,6 +428,14 @@ func transformInsertPlan(ctx *plancontext.PlanningContext, op *operators.Route, return } } + + if ins.FKVerify != nil { + i.fkVerify, err = transformToLogicalPlan(ctx, ins.FKVerify, true) + if err != nil { + return + } + } + return } diff --git a/go/vt/vtgate/planbuilder/operators/fk_verify.go b/go/vt/vtgate/planbuilder/operators/fk_verify.go index ab4dc449b56..5db9a4f9add 100644 --- a/go/vt/vtgate/planbuilder/operators/fk_verify.go +++ b/go/vt/vtgate/planbuilder/operators/fk_verify.go @@ -71,7 +71,7 @@ func NewFkVerify(ctx *plancontext.PlanningContext, parentFKs []vindexes.ParentFK ), ), } - op, err := createOperatorFromSelect(ctx, query) + op, err := PlanQuery(ctx, query) if err != nil { return nil, err } From c788ed3b667341a45c95776ff01ac4b9ae0ff643 Mon Sep 17 00:00:00 2001 From: Manan Gupta Date: Wed, 2 Aug 2023 16:56:33 +0530 Subject: [PATCH 03/12] feat: add tests for some functions Signed-off-by: Manan Gupta --- go/vt/sqlparser/ast_funcs.go | 10 + go/vt/sqlparser/ast_funcs_test.go | 38 +++ go/vt/sqlparser/normalizer_test.go | 2 +- go/vt/vtgate/vindexes/foreign_keys_test.go | 281 +++++++++++++++++++++ 4 files changed, 330 insertions(+), 1 deletion(-) create mode 100644 go/vt/vtgate/vindexes/foreign_keys_test.go diff --git a/go/vt/sqlparser/ast_funcs.go b/go/vt/sqlparser/ast_funcs.go index 72713723575..11af88a6842 100644 --- a/go/vt/sqlparser/ast_funcs.go +++ b/go/vt/sqlparser/ast_funcs.go @@ -2514,3 +2514,13 @@ func (cols Columns) Indexes(subSetCols Columns) (bool, []int) { } return true, indexes } + +// MakeColumns is to used to make a list of columns from a list of strings. +// This function is meeant to be used in testing code. +func MakeColumns(colNames ...string) Columns { + var cols Columns + for _, name := range colNames { + cols = append(cols, NewIdentifierCI(name)) + } + return cols +} diff --git a/go/vt/sqlparser/ast_funcs_test.go b/go/vt/sqlparser/ast_funcs_test.go index b6a79da45ab..7bec47df96f 100644 --- a/go/vt/sqlparser/ast_funcs_test.go +++ b/go/vt/sqlparser/ast_funcs_test.go @@ -134,3 +134,41 @@ func TestSQLTypeToQueryType(t *testing.T) { }) } } + +// TestColumns_Indexes verifies the functionality of Indexes method on Columns. +func TestColumns_Indexes(t *testing.T) { + tests := []struct { + name string + cols Columns + subSetCols Columns + indexesWanted []int + }{ + { + name: "Not a subset", + cols: MakeColumns("col1", "col2", "col3"), + subSetCols: MakeColumns("col2", "col4"), + }, { + name: "Subset with 1 value", + cols: MakeColumns("col1", "col2", "col3"), + subSetCols: MakeColumns("col2"), + indexesWanted: []int{1}, + }, { + name: "Subset with multiple values", + cols: MakeColumns("col1", "col2", "col3", "col4", "col5"), + subSetCols: MakeColumns("col3", "col5", "col1"), + indexesWanted: []int{2, 4, 0}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + isSubset, indexes := tt.cols.Indexes(tt.subSetCols) + if tt.indexesWanted == nil { + require.False(t, isSubset) + require.Nil(t, indexes) + return + } + require.True(t, isSubset) + require.EqualValues(t, tt.indexesWanted, indexes) + }) + } +} diff --git a/go/vt/sqlparser/normalizer_test.go b/go/vt/sqlparser/normalizer_test.go index c4568ba3de6..44550d9d877 100644 --- a/go/vt/sqlparser/normalizer_test.go +++ b/go/vt/sqlparser/normalizer_test.go @@ -440,7 +440,7 @@ func TestNormalizeOneCasae(t *testing.T) { testOne := struct { input, output string }{ - input: "select count(distinct id, name) from area where (id, name) in ((1, 'US'))", + input: "", output: "", } if testOne.input == "" { diff --git a/go/vt/vtgate/vindexes/foreign_keys_test.go b/go/vt/vtgate/vindexes/foreign_keys_test.go new file mode 100644 index 00000000000..08fbbff7490 --- /dev/null +++ b/go/vt/vtgate/vindexes/foreign_keys_test.go @@ -0,0 +1,281 @@ +package vindexes + +import ( + "testing" + + "github.com/stretchr/testify/require" + + "vitess.io/vitess/go/vt/sqlparser" +) + +// TestTable_CrossShardParentFKs tests the functionality of the method CrossShardParentFKs. +func TestTable_CrossShardParentFKs(t *testing.T) { + tests := []struct { + name string + table *Table + wantCrossShardFKTables []string + }{ + { + name: "Unsharded keyspace", + table: &Table{ + ColumnVindexes: []*ColumnVindex{ + { + Name: "v1", + Vindex: binVindex, + Columns: []sqlparser.IdentifierCI{ + sqlparser.NewIdentifierCI("col1"), + }, + }, + }, + Keyspace: &Keyspace{ + Name: "ks", + Sharded: false, + }, + ParentForeignKeys: []ParentFKInfo{ + { + Table: &Table{ + Name: sqlparser.NewIdentifierCS("t1"), + ColumnVindexes: []*ColumnVindex{ + { + Name: "v2", + Vindex: binVindex, + Columns: []sqlparser.IdentifierCI{ + sqlparser.NewIdentifierCI("col4"), + }, + }, + }, + }, + ChildColumns: sqlparser.MakeColumns("col1"), + ParentColumns: sqlparser.MakeColumns("col3"), + }, + }, + }, + wantCrossShardFKTables: []string{}, + }, + { + name: "No Parent FKs", + table: &Table{ + ColumnVindexes: []*ColumnVindex{ + { + Name: "v1", + Vindex: binVindex, + Columns: []sqlparser.IdentifierCI{ + sqlparser.NewIdentifierCI("col1"), + }, + }, + }, + Keyspace: &Keyspace{ + Name: "ks", + Sharded: true, + }, + }, + wantCrossShardFKTables: []string{}, + }, { + name: "Column Vindexes don't match", + table: &Table{ + ColumnVindexes: []*ColumnVindex{ + { + Name: "v1", + Vindex: binVindex, + Columns: []sqlparser.IdentifierCI{ + sqlparser.NewIdentifierCI("col1"), + }, + }, + }, + Keyspace: &Keyspace{ + Name: "ks", + Sharded: true, + }, + ParentForeignKeys: []ParentFKInfo{ + { + Table: &Table{ + Name: sqlparser.NewIdentifierCS("t1"), + ColumnVindexes: []*ColumnVindex{ + { + Name: "v2", + Vindex: binOnlyVindex, + Columns: []sqlparser.IdentifierCI{ + sqlparser.NewIdentifierCI("col4"), + }, + }, + }, + }, + ChildColumns: sqlparser.MakeColumns("col1"), + ParentColumns: sqlparser.MakeColumns("col4"), + }, + }, + }, + wantCrossShardFKTables: []string{"t1"}, + }, { + name: "Child FK doesn't contain primary vindex", + table: &Table{ + ColumnVindexes: []*ColumnVindex{ + { + Name: "v1", + Vindex: binVindex, + Columns: []sqlparser.IdentifierCI{ + sqlparser.NewIdentifierCI("col1"), + sqlparser.NewIdentifierCI("col2"), + sqlparser.NewIdentifierCI("col3"), + }, + }, + }, + Keyspace: &Keyspace{ + Name: "ks", + Sharded: true, + }, + ParentForeignKeys: []ParentFKInfo{ + { + Table: &Table{ + Name: sqlparser.NewIdentifierCS("t1"), + ColumnVindexes: []*ColumnVindex{ + { + Name: "v2", + Vindex: binVindex, + Columns: []sqlparser.IdentifierCI{ + sqlparser.NewIdentifierCI("col4"), + sqlparser.NewIdentifierCI("col5"), + sqlparser.NewIdentifierCI("col6"), + }, + }, + }, + }, + ChildColumns: sqlparser.MakeColumns("col3", "col9", "col1"), + ParentColumns: sqlparser.MakeColumns("col4", "col5", "col6"), + }, + }, + }, + wantCrossShardFKTables: []string{"t1"}, + }, { + name: "Parent FK doesn't contain primary vindex", + table: &Table{ + ColumnVindexes: []*ColumnVindex{ + { + Name: "v1", + Vindex: binVindex, + Columns: []sqlparser.IdentifierCI{ + sqlparser.NewIdentifierCI("col1"), + sqlparser.NewIdentifierCI("col2"), + sqlparser.NewIdentifierCI("col3"), + }, + }, + }, + Keyspace: &Keyspace{ + Name: "ks", + Sharded: true, + }, + ParentForeignKeys: []ParentFKInfo{ + { + Table: &Table{ + Name: sqlparser.NewIdentifierCS("t1"), + ColumnVindexes: []*ColumnVindex{ + { + Name: "v2", + Vindex: binVindex, + Columns: []sqlparser.IdentifierCI{ + sqlparser.NewIdentifierCI("col4"), + sqlparser.NewIdentifierCI("col5"), + sqlparser.NewIdentifierCI("col6"), + }, + }, + }, + }, + ChildColumns: sqlparser.MakeColumns("col1", "col2", "col3"), + ParentColumns: sqlparser.MakeColumns("col4", "col9", "col6"), + }, + }, + }, + wantCrossShardFKTables: []string{"t1"}, + }, { + name: "Indexes of the two FKs with column vindexes don't line up", + table: &Table{ + ColumnVindexes: []*ColumnVindex{ + { + Name: "v1", + Vindex: binVindex, + Columns: []sqlparser.IdentifierCI{ + sqlparser.NewIdentifierCI("col1"), + sqlparser.NewIdentifierCI("col2"), + sqlparser.NewIdentifierCI("col3"), + }, + }, + }, + Keyspace: &Keyspace{ + Name: "ks", + Sharded: true, + }, + ParentForeignKeys: []ParentFKInfo{ + { + Table: &Table{ + Name: sqlparser.NewIdentifierCS("t1"), + ColumnVindexes: []*ColumnVindex{ + { + Name: "v2", + Vindex: binVindex, + Columns: []sqlparser.IdentifierCI{ + sqlparser.NewIdentifierCI("col4"), + sqlparser.NewIdentifierCI("col5"), + sqlparser.NewIdentifierCI("col6"), + }, + }, + }, + }, + ChildColumns: sqlparser.MakeColumns("col1", "col2", "col3", "col9"), + ParentColumns: sqlparser.MakeColumns("col4", "col9", "col5", "col6"), + }, + }, + }, + wantCrossShardFKTables: []string{"t1"}, + }, { + name: "Shard scoped foreign key constraint", + table: &Table{ + ColumnVindexes: []*ColumnVindex{ + { + Name: "v1", + Vindex: binVindex, + Columns: []sqlparser.IdentifierCI{ + sqlparser.NewIdentifierCI("col1"), + sqlparser.NewIdentifierCI("col2"), + sqlparser.NewIdentifierCI("col3"), + }, + }, + }, + Keyspace: &Keyspace{ + Name: "ks", + Sharded: true, + }, + ParentForeignKeys: []ParentFKInfo{ + { + Table: &Table{ + Name: sqlparser.NewIdentifierCS("t1"), + ColumnVindexes: []*ColumnVindex{ + { + Name: "v2", + Vindex: binVindex, + Columns: []sqlparser.IdentifierCI{ + sqlparser.NewIdentifierCI("col4"), + sqlparser.NewIdentifierCI("col5"), + sqlparser.NewIdentifierCI("col6"), + }, + }, + }, + }, + ChildColumns: sqlparser.MakeColumns("col1", "cola", "col2", "col3", "colb"), + ParentColumns: sqlparser.MakeColumns("col4", "col9", "col5", "col6", "colc"), + }, + }, + }, + wantCrossShardFKTables: []string{}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + crossShardFks := tt.table.CrossShardParentFKs() + var crossShardFkTables []string + for _, fk := range crossShardFks { + crossShardFkTables = append(crossShardFkTables, fk.Table.Name.String()) + } + require.ElementsMatch(t, tt.wantCrossShardFKTables, crossShardFkTables) + }) + } +} From 24ee35120b6124ad89f25a65ea539b4867a21383 Mon Sep 17 00:00:00 2001 From: Manan Gupta Date: Thu, 3 Aug 2023 11:41:27 +0530 Subject: [PATCH 04/12] feat: fix checkFkError to use the keyspace found Signed-off-by: Manan Gupta --- go/vt/vtgate/planbuilder/ddl.go | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/go/vt/vtgate/planbuilder/ddl.go b/go/vt/vtgate/planbuilder/ddl.go index d84e5341294..1557a1d1d8f 100644 --- a/go/vt/vtgate/planbuilder/ddl.go +++ b/go/vt/vtgate/planbuilder/ddl.go @@ -111,10 +111,6 @@ func buildDDLPlans(ctx context.Context, sql string, ddlStatement sqlparser.DDLSt switch ddl := ddlStatement.(type) { case *sqlparser.AlterTable, *sqlparser.CreateTable, *sqlparser.TruncateTable: - err = checkFKError(vschema, ddlStatement) - if err != nil { - return nil, nil, err - } // For ALTER TABLE and TRUNCATE TABLE, the table must already exist // // For CREATE TABLE, the table may (in the case of --declarative) @@ -122,6 +118,10 @@ func buildDDLPlans(ctx context.Context, sql string, ddlStatement sqlparser.DDLSt // // We should find the target of the query from this tables location. destination, keyspace, err = findTableDestinationAndKeyspace(vschema, ddlStatement) + if err != nil { + return nil, nil, err + } + err = checkFKError(vschema, ddlStatement, keyspace) case *sqlparser.CreateView: destination, keyspace, err = buildCreateView(ctx, vschema, ddl, reservedVars, enableOnlineDDL, enableDirectDDL) case *sqlparser.AlterView: @@ -162,12 +162,8 @@ func buildDDLPlans(ctx context.Context, sql string, ddlStatement sqlparser.DDLSt }, nil } -func checkFKError(vschema plancontext.VSchema, ddlStatement sqlparser.DDLStatement) error { - _, ks, _, err := vschema.TargetDestination(ddlStatement.GetTable().Qualifier.String()) - if err != nil { - return err - } - fkMode, err := vschema.ForeignKeyMode(ks.Name) +func checkFKError(vschema plancontext.VSchema, ddlStatement sqlparser.DDLStatement, keyspace *vindexes.Keyspace) error { + fkMode, err := vschema.ForeignKeyMode(keyspace.Name) if err != nil { return err } From 914117f45a24a98254feacec134b1d05326221bb Mon Sep 17 00:00:00 2001 From: Manan Gupta Date: Thu, 3 Aug 2023 12:32:20 +0530 Subject: [PATCH 05/12] feat: add unit tests for foreign_keys_planning Signed-off-by: Manan Gupta --- .../vtgate/planbuilder/operators/fk_verify.go | 9 ++- go/vt/vtgate/planbuilder/plan_test.go | 43 ++++++++++++- .../vtgate/planbuilder/testdata/fk_cases.json | 57 +++++++++++++++++ .../planbuilder/testdata/vschemas/schema.json | 63 +++++++++++++++++++ go/vt/vtgate/vindexes/foreign_keys.go | 4 +- go/vt/vtgate/vindexes/vschema_test.go | 2 +- 6 files changed, 173 insertions(+), 5 deletions(-) create mode 100644 go/vt/vtgate/planbuilder/testdata/fk_cases.json diff --git a/go/vt/vtgate/planbuilder/operators/fk_verify.go b/go/vt/vtgate/planbuilder/operators/fk_verify.go index 5db9a4f9add..32e483dfa46 100644 --- a/go/vt/vtgate/planbuilder/operators/fk_verify.go +++ b/go/vt/vtgate/planbuilder/operators/fk_verify.go @@ -4,6 +4,7 @@ import ( "vitess.io/vitess/go/vt/sqlparser" "vitess.io/vitess/go/vt/vtgate/planbuilder/operators/ops" "vitess.io/vitess/go/vt/vtgate/planbuilder/plancontext" + "vitess.io/vitess/go/vt/vtgate/semantics" "vitess.io/vitess/go/vt/vtgate/vindexes" ) @@ -71,7 +72,13 @@ func NewFkVerify(ctx *plancontext.PlanningContext, parentFKs []vindexes.ParentFK ), ), } - op, err := PlanQuery(ctx, query) + + newSemTable, err := semantics.Analyze(query, fk.Table.Keyspace.Name, ctx.VSchema) + if err != nil { + return nil, err + } + newCtx := plancontext.NewPlanningContext(nil, newSemTable, ctx.VSchema, ctx.PlannerVersion) + op, err := PlanQuery(newCtx, query) if err != nil { return nil, err } diff --git a/go/vt/vtgate/planbuilder/plan_test.go b/go/vt/vtgate/planbuilder/plan_test.go index f85c03c1b4e..c15e7074190 100644 --- a/go/vt/vtgate/planbuilder/plan_test.go +++ b/go/vt/vtgate/planbuilder/plan_test.go @@ -100,6 +100,21 @@ func TestPlan(t *testing.T) { testFile(t, "misc_cases.json", testOutputTempDir, vschemaWrapper, false) } +// 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, + }, + } + testOutputTempDir := makeTestOutput(t) + + testFile(t, "fk_cases.json", testOutputTempDir, vschemaWrapper, false) +} + func TestSystemTables57(t *testing.T) { // first we move everything to use 5.7 logic servenv.SetMySQLServerVersionForTest("5.7") @@ -418,9 +433,31 @@ 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"})) + 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"})) + 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"})) + require.NoError(t, err) + } return vschema } +// createFkDefinition is a helper function to create a Foreign key definition struct from the columns used in it provided as list of strings. +func createFkDefinition(childCols []string, parentTableName string, parentCols []string) *sqlparser.ForeignKeyDefinition { + return &sqlparser.ForeignKeyDefinition{ + Source: sqlparser.MakeColumns(childCols...), + ReferenceDefinition: &sqlparser.ReferenceDefinition{ + ReferencedTable: sqlparser.NewTableName(parentTableName), + ReferencedColumns: sqlparser.MakeColumns(parentCols...), + }, + } +} + var _ plancontext.VSchema = (*vschemaWrapper)(nil) type vschemaWrapper struct { @@ -508,7 +545,11 @@ func (vw *vschemaWrapper) PlannerWarning(_ string) { } func (vw *vschemaWrapper) ForeignKeyMode(keyspace string) (vschemapb.Keyspace_ForeignKeyMode, error) { - return vschemapb.Keyspace_FK_UNMANAGED, nil + defaultFkMode := vschemapb.Keyspace_FK_UNMANAGED + if vw.v.Keyspaces[keyspace] != nil && vw.v.Keyspaces[keyspace].ForeignKeyMode != vschemapb.Keyspace_FK_DEFAULT { + return vw.v.Keyspaces[keyspace].ForeignKeyMode, nil + } + return defaultFkMode, nil } func (vw *vschemaWrapper) AllKeyspace() ([]*vindexes.Keyspace, error) { diff --git a/go/vt/vtgate/planbuilder/testdata/fk_cases.json b/go/vt/vtgate/planbuilder/testdata/fk_cases.json new file mode 100644 index 00000000000..500c58b8972 --- /dev/null +++ b/go/vt/vtgate/planbuilder/testdata/fk_cases.json @@ -0,0 +1,57 @@ +[ + { + "comment": "Insertion in a table with cross-shard foreign keys disallowed", + "query": "insert into tbl3 (col3, coly) values (1, 3)", + "plan": "VT12002: unsupported cross-shard foreign keys" + }, + { + "comment": "Insertion in a table with shard-scoped foreign keys is allowed", + "query": "insert into tbl2 (col2, coly) values (1, 3)", + "plan": { + "QueryType": "INSERT", + "Original": "insert into tbl2 (col2, coly) values (1, 3)", + "Instructions": { + "OperatorType": "Insert", + "Variant": "Sharded", + "Keyspace": { + "Name": "user_fk_allow", + "Sharded": true + }, + "TargetTabletType": "PRIMARY", + "Query": "insert into tbl2(col2, coly) values (:_col2_0, 3)", + "TableName": "tbl2", + "VindexValues": { + "hash_vin": "INT64(1)" + } + }, + "TablesUsed": [ + "user_fk_allow.tbl2" + ] + } + }, + { + "comment": "Insertion in a table with shard-scoped multiple column foreign key is allowed", + "query": "insert into multicol_tbl2 (cola, colb, colc) values (1, 2, 3)", + "plan": { + "QueryType": "INSERT", + "Original": "insert into multicol_tbl2 (cola, colb, colc) values (1, 2, 3)", + "Instructions": { + "OperatorType": "Insert", + "Variant": "Sharded", + "Keyspace": { + "Name": "user_fk_allow", + "Sharded": true + }, + "TargetTabletType": "PRIMARY", + "Query": "insert into multicol_tbl2(cola, colb, colc) values (:_cola_0, :_colb_0, :_colc_0)", + "TableName": "multicol_tbl2", + "VindexValues": { + "multicolIdx": "INT64(1), INT64(2), INT64(3)" + } + }, + "TablesUsed": [ + "user_fk_allow.multicol_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 9f9ea97200f..1d23f669795 100644 --- a/go/vt/vtgate/planbuilder/testdata/vschemas/schema.json +++ b/go/vt/vtgate/planbuilder/testdata/vschemas/schema.json @@ -602,6 +602,69 @@ ] } } + }, + "user_fk_allow": { + "sharded": true, + "foreignKeyMode": "FK_MANAGED", + "vindexes": { + "hash_vin": { + "type": "hash_test", + "owner": "user" + }, + "multicolIdx": { + "type": "multiCol_test" + } + }, + "tables": { + "multicol_tbl1": { + "column_vindexes": [ + { + "columns": [ + "cola", + "colb", + "colc" + ], + "name": "multicolIdx" + } + ] + }, + "multicol_tbl2": { + "column_vindexes": [ + { + "columns": [ + "cola", + "colb", + "colc" + ], + "name": "multicolIdx" + } + ] + }, + "tbl1": { + "column_vindexes": [ + { + "column": "col1", + "name": "hash_vin" + } + ] + }, + "tbl2": { + "column_vindexes": [ + { + "column": "col2", + "name": "hash_vin" + } + ] + }, + "tbl3": { + "column_vindexes": [ + { + "column": "col3", + "name": "hash_vin" + } + ] + } + } } } } diff --git a/go/vt/vtgate/vindexes/foreign_keys.go b/go/vt/vtgate/vindexes/foreign_keys.go index c749cd948d6..1aaabc0cd6f 100644 --- a/go/vt/vtgate/vindexes/foreign_keys.go +++ b/go/vt/vtgate/vindexes/foreign_keys.go @@ -71,8 +71,8 @@ func NewChildFkInfo(childTbl *Table, fkDef *sqlparser.ForeignKeyDefinition) Chil } } -// addForeignKey is for testing only. -func (vschema *VSchema) addForeignKey(ksname, childTableName string, fkConstraint *sqlparser.ForeignKeyDefinition) error { +// AddForeignKey is for testing only. +func (vschema *VSchema) AddForeignKey(ksname, childTableName string, fkConstraint *sqlparser.ForeignKeyDefinition) error { ks, ok := vschema.Keyspaces[ksname] if !ok { return fmt.Errorf("keyspace %s not found in vschema", ksname) diff --git a/go/vt/vtgate/vindexes/vschema_test.go b/go/vt/vtgate/vindexes/vschema_test.go index 1e21235dc0e..58874967d49 100644 --- a/go/vt/vtgate/vindexes/vschema_test.go +++ b/go/vt/vtgate/vindexes/vschema_test.go @@ -411,7 +411,7 @@ func TestVSchemaForeignKeys(t *testing.T) { require.NoError(t, vschema.Keyspaces["main"].Error) // add fk containst a keyspace. - vschema.addForeignKey("main", "t1", &sqlparser.ForeignKeyDefinition{ + vschema.AddForeignKey("main", "t1", &sqlparser.ForeignKeyDefinition{ Source: sqlparser.Columns{sqlparser.NewIdentifierCI("c2")}, ReferenceDefinition: &sqlparser.ReferenceDefinition{ ReferencedTable: sqlparser.NewTableName("t1"), From aee57e6b9945fbc15f2f33c07585ebab17e7c56c Mon Sep 17 00:00:00 2001 From: Manan Gupta Date: Thu, 3 Aug 2023 12:49:52 +0530 Subject: [PATCH 06/12] feat: also handle the case of cross-keyspace foreign keys Signed-off-by: Manan Gupta --- go/vt/vtgate/vindexes/foreign_keys.go | 13 ++++- go/vt/vtgate/vindexes/foreign_keys_test.go | 64 ++++++++++++++++++++++ 2 files changed, 76 insertions(+), 1 deletion(-) diff --git a/go/vt/vtgate/vindexes/foreign_keys.go b/go/vt/vtgate/vindexes/foreign_keys.go index 1aaabc0cd6f..f8afe297075 100644 --- a/go/vt/vtgate/vindexes/foreign_keys.go +++ b/go/vt/vtgate/vindexes/foreign_keys.go @@ -93,11 +93,22 @@ 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 !t.Keyspace.Sharded || len(t.ParentForeignKeys) == 0 { + if len(t.ParentForeignKeys) == 0 { return } primaryVindex := t.ColumnVindexes[0] 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) + continue + } + // If the keyspaces match and they are unsharded, then the fk defintion + // is shard-scoped. + if !t.Keyspace.Sharded { + continue + } // If the primary vindexes don't match between the parent and child table, // we cannot infer that the fk constraint in shard scoped. if fk.Table.ColumnVindexes[0].Vindex != primaryVindex.Vindex { diff --git a/go/vt/vtgate/vindexes/foreign_keys_test.go b/go/vt/vtgate/vindexes/foreign_keys_test.go index 08fbbff7490..706c191b7fb 100644 --- a/go/vt/vtgate/vindexes/foreign_keys_test.go +++ b/go/vt/vtgate/vindexes/foreign_keys_test.go @@ -35,6 +35,10 @@ func TestTable_CrossShardParentFKs(t *testing.T) { { Table: &Table{ Name: sqlparser.NewIdentifierCS("t1"), + Keyspace: &Keyspace{ + Name: "ks", + Sharded: false, + }, ColumnVindexes: []*ColumnVindex{ { Name: "v2", @@ -70,6 +74,46 @@ func TestTable_CrossShardParentFKs(t *testing.T) { }, }, wantCrossShardFKTables: []string{}, + }, { + name: "Keyspaces don't match", + table: &Table{ + ColumnVindexes: []*ColumnVindex{ + { + Name: "v1", + Vindex: binVindex, + Columns: []sqlparser.IdentifierCI{ + sqlparser.NewIdentifierCI("col1"), + }, + }, + }, + Keyspace: &Keyspace{ + Name: "ks", + Sharded: false, + }, + ParentForeignKeys: []ParentFKInfo{ + { + Table: &Table{ + Name: sqlparser.NewIdentifierCS("t1"), + Keyspace: &Keyspace{ + Name: "ks2", + Sharded: false, + }, + ColumnVindexes: []*ColumnVindex{ + { + Name: "v2", + Vindex: binVindex, + Columns: []sqlparser.IdentifierCI{ + sqlparser.NewIdentifierCI("col4"), + }, + }, + }, + }, + ChildColumns: sqlparser.MakeColumns("col1"), + ParentColumns: sqlparser.MakeColumns("col4"), + }, + }, + }, + wantCrossShardFKTables: []string{"t1"}, }, { name: "Column Vindexes don't match", table: &Table{ @@ -90,6 +134,10 @@ func TestTable_CrossShardParentFKs(t *testing.T) { { Table: &Table{ Name: sqlparser.NewIdentifierCS("t1"), + Keyspace: &Keyspace{ + Name: "ks", + Sharded: true, + }, ColumnVindexes: []*ColumnVindex{ { Name: "v2", @@ -128,6 +176,10 @@ func TestTable_CrossShardParentFKs(t *testing.T) { { Table: &Table{ Name: sqlparser.NewIdentifierCS("t1"), + Keyspace: &Keyspace{ + Name: "ks", + Sharded: true, + }, ColumnVindexes: []*ColumnVindex{ { Name: "v2", @@ -168,6 +220,10 @@ func TestTable_CrossShardParentFKs(t *testing.T) { { Table: &Table{ Name: sqlparser.NewIdentifierCS("t1"), + Keyspace: &Keyspace{ + Name: "ks", + Sharded: true, + }, ColumnVindexes: []*ColumnVindex{ { Name: "v2", @@ -208,6 +264,10 @@ func TestTable_CrossShardParentFKs(t *testing.T) { { Table: &Table{ Name: sqlparser.NewIdentifierCS("t1"), + Keyspace: &Keyspace{ + Name: "ks", + Sharded: true, + }, ColumnVindexes: []*ColumnVindex{ { Name: "v2", @@ -248,6 +308,10 @@ func TestTable_CrossShardParentFKs(t *testing.T) { { Table: &Table{ Name: sqlparser.NewIdentifierCS("t1"), + Keyspace: &Keyspace{ + Name: "ks", + Sharded: true, + }, ColumnVindexes: []*ColumnVindex{ { Name: "v2", From 4688028a6440459818bfdf86fb4f521c02e7a63c Mon Sep 17 00:00:00 2001 From: Manan Gupta Date: Thu, 3 Aug 2023 13:40:53 +0530 Subject: [PATCH 07/12] feat: add e2e tests and fix infinite loop in vtgate :3 Signed-off-by: Manan Gupta --- .../endtoend/vtgate/foreign_keys/fk_test.go | 51 +++++++ .../endtoend/vtgate/foreign_keys/main_test.go | 125 ++++++++++++++++++ .../vtgate/foreign_keys/sharded_schema.sql | 41 ++++++ .../vtgate/foreign_keys/sharded_vschema.json | 67 ++++++++++ .../vtgate/planbuilder/operators/fk_verify.go | 4 +- test/config.json | 9 ++ 6 files changed, 295 insertions(+), 2 deletions(-) create mode 100644 go/test/endtoend/vtgate/foreign_keys/fk_test.go create mode 100644 go/test/endtoend/vtgate/foreign_keys/main_test.go create mode 100644 go/test/endtoend/vtgate/foreign_keys/sharded_schema.sql create mode 100644 go/test/endtoend/vtgate/foreign_keys/sharded_vschema.json diff --git a/go/test/endtoend/vtgate/foreign_keys/fk_test.go b/go/test/endtoend/vtgate/foreign_keys/fk_test.go new file mode 100644 index 00000000000..310f81674df --- /dev/null +++ b/go/test/endtoend/vtgate/foreign_keys/fk_test.go @@ -0,0 +1,51 @@ +/* +Copyright 2023 The Vitess Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package foreign_keys + +import ( + "testing" + + "github.com/stretchr/testify/require" + + "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) { + mcmp, closer := start(t) + defer closer() + + // insert some data. + utils.Exec(t, mcmp.VtConn, `insert into t1(id, col) values (100, 123),(10, 12),(1, 13),(1000, 1234)`) + + // Verify that inserting data into a table that has shard scoped foreign keys works. + utils.Exec(t, mcmp.VtConn, `insert into t2(id, col) values (100, 125), (1, 132)`) + // Verify that insertion fails if the data doesn't follow the fk constraint. + _, err := utils.ExecAllowError(t, mcmp.VtConn, `insert into t2(id, col) values (1310, 125)`) + require.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, mcmp.VtConn, `insert into t3(id, col) values (100, 100)`) + require.ErrorContains(t, err, "VT12002: unsupported cross-shard foreign keys") + + // insert some data in a table with multicol vindex. + utils.Exec(t, mcmp.VtConn, `insert into multicol_tbl1(cola, colb, colc, msg) values (100, 'a', 'b', 'msg'), (101, 'c', 'd', 'msg2')`) + // Verify that inserting data into a table that has shard scoped multi-column foreign keys works. + utils.Exec(t, mcmp.VtConn, `insert into multicol_tbl2(cola, colb, colc, msg) values (100, 'a', 'b', 'msg3')`) + // Verify that insertion fails if the data doesn't follow the fk constraint. + _, err = utils.ExecAllowError(t, mcmp.VtConn, `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") +} diff --git a/go/test/endtoend/vtgate/foreign_keys/main_test.go b/go/test/endtoend/vtgate/foreign_keys/main_test.go new file mode 100644 index 00000000000..3ac1875e99f --- /dev/null +++ b/go/test/endtoend/vtgate/foreign_keys/main_test.go @@ -0,0 +1,125 @@ +/* +Copyright 2023 The Vitess Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package foreign_keys + +import ( + _ "embed" + "flag" + "fmt" + "os" + "testing" + + "github.com/stretchr/testify/require" + + "vitess.io/vitess/go/test/endtoend/utils" + + "vitess.io/vitess/go/vt/vtgate/planbuilder" + + "vitess.io/vitess/go/mysql" + "vitess.io/vitess/go/test/endtoend/cluster" +) + +var ( + clusterInstance *cluster.LocalProcessCluster + vtParams mysql.ConnParams + mysqlParams mysql.ConnParams + shardedKs = "ks" + shardedKsShards = []string{"-19a0", "19a0-20", "20-20c0", "20c0-"} + Cell = "test" + //go:embed sharded_schema.sql + shardedSchemaSQL string + + //go:embed sharded_vschema.json + shardedVSchema string +) + +func TestMain(m *testing.M) { + defer cluster.PanicHandler(nil) + flag.Parse() + + exitCode := func() int { + clusterInstance = cluster.NewCluster(Cell, "localhost") + defer clusterInstance.Teardown() + + // Start topo server + err := clusterInstance.StartTopo() + if err != nil { + return 1 + } + + // Start keyspace + sKs := &cluster.Keyspace{ + Name: shardedKs, + SchemaSQL: shardedSchemaSQL, + VSchema: shardedVSchema, + } + + clusterInstance.VtGateExtraArgs = []string{"--schema_change_signal"} + clusterInstance.VtTabletExtraArgs = []string{"--queryserver-config-schema-change-signal", "--queryserver-config-schema-change-signal-interval", "0.1"} + err = clusterInstance.StartKeyspace(*sKs, shardedKsShards, 0, false) + if err != nil { + return 1 + } + + err = clusterInstance.VtctlclientProcess.ExecuteCommand("RebuildVSchemaGraph") + if err != nil { + return 1 + } + + // Start vtgate + clusterInstance.VtGatePlannerVersion = planbuilder.Gen4 // enable Gen4 planner. + err = clusterInstance.StartVtgate() + if err != nil { + return 1 + } + vtParams = mysql.ConnParams{ + Host: clusterInstance.Hostname, + Port: clusterInstance.VtgateMySQLPort, + } + + conn, closer, err := utils.NewMySQL(clusterInstance, shardedKs, shardedSchemaSQL) + if err != nil { + fmt.Println(err) + return 1 + } + defer closer() + mysqlParams = conn + return m.Run() + }() + os.Exit(exitCode) +} + +func start(t *testing.T) (utils.MySQLCompare, func()) { + mcmp, err := utils.NewMySQLCompare(t, vtParams, mysqlParams) + require.NoError(t, err) + deleteAll := func() { + _, _ = utils.ExecAllowError(t, mcmp.VtConn, "set workload = oltp") + + tables := []string{"t3", "t2", "t1", "multicol_tbl2", "multicol_tbl1"} + for _, table := range tables { + _, _ = mcmp.ExecAndIgnore("delete from " + table) + } + } + + deleteAll() + + return mcmp, func() { + deleteAll() + mcmp.Close() + cluster.PanicHandler(t) + } +} diff --git a/go/test/endtoend/vtgate/foreign_keys/sharded_schema.sql b/go/test/endtoend/vtgate/foreign_keys/sharded_schema.sql new file mode 100644 index 00000000000..47432e0998b --- /dev/null +++ b/go/test/endtoend/vtgate/foreign_keys/sharded_schema.sql @@ -0,0 +1,41 @@ +create table t1 +( + id bigint, + col bigint, + primary key (id) +) Engine = InnoDB; + +create table t2 +( + id bigint, + col bigint, + primary key (id), + foreign key (id) references t1 (id) +) Engine = InnoDB; + +create table t3 +( + id bigint, + col bigint, + primary key (id), + foreign key (col) references t1 (id) +) Engine = InnoDB; + +create table multicol_tbl1 +( + cola bigint, + colb varbinary(50), + colc varchar(50), + msg varchar(50), + primary key (cola, colb, colc) +) Engine = InnoDB; + +create table multicol_tbl2 +( + cola bigint, + colb varbinary(50), + colc varchar(50), + msg varchar(50), + primary key (cola, colb, colc), + foreign key (cola, colb, colc) references multicol_tbl1 (cola, colb, colc) +) Engine = InnoDB; diff --git a/go/test/endtoend/vtgate/foreign_keys/sharded_vschema.json b/go/test/endtoend/vtgate/foreign_keys/sharded_vschema.json new file mode 100644 index 00000000000..e55185f25b0 --- /dev/null +++ b/go/test/endtoend/vtgate/foreign_keys/sharded_vschema.json @@ -0,0 +1,67 @@ +{ + "sharded": true, + "foreignKeyMode": "FK_MANAGED", + "vindexes": { + "xxhash": { + "type": "xxhash" + }, + "multicol_vdx": { + "type": "multicol", + "params": { + "column_count": "3", + "column_bytes": "1,3,4", + "column_vindex": "hash,binary,unicode_loose_xxhash" + } + } + }, + "tables": { + "t1": { + "column_vindexes": [ + { + "column": "id", + "name": "xxhash" + } + ] + }, + "t2": { + "column_vindexes": [ + { + "column": "id", + "name": "xxhash" + } + ] + }, + "t3": { + "column_vindexes": [ + { + "column": "id", + "name": "xxhash" + } + ] + }, + "multicol_tbl1": { + "column_vindexes": [ + { + "columns": [ + "cola", + "colb", + "colc" + ], + "name": "multicol_vdx" + } + ] + }, + "multicol_tbl2": { + "column_vindexes": [ + { + "columns": [ + "cola", + "colb", + "colc" + ], + "name": "multicol_vdx" + } + ] + } + } +} \ No newline at end of file diff --git a/go/vt/vtgate/planbuilder/operators/fk_verify.go b/go/vt/vtgate/planbuilder/operators/fk_verify.go index 32e483dfa46..1e403639a0a 100644 --- a/go/vt/vtgate/planbuilder/operators/fk_verify.go +++ b/go/vt/vtgate/planbuilder/operators/fk_verify.go @@ -87,11 +87,11 @@ func NewFkVerify(ctx *plancontext.PlanningContext, parentFKs []vindexes.ParentFK // Build the offsets var offsets []int outer: - for _, column := range fk.ParentColumns { + for _, column := range fk.ChildColumns { for idx, insertColumn := range insertColumns { if column.Equal(insertColumn) { offsets = append(offsets, idx) - goto outer + continue outer } } offsets = append(offsets, -1) diff --git a/test/config.json b/test/config.json index c0f0b0825f8..903331f2600 100644 --- a/test/config.json +++ b/test/config.json @@ -842,6 +842,15 @@ "RetryMax": 1, "Tags": [] }, + "vtgate_foreign_keys": { + "File": "unused.go", + "Args": ["vitess.io/vitess/go/test/endtoend/vtgate/foreign_keys"], + "Command": [], + "Manual": false, + "Shard": "vtgate_gen4", + "RetryMax": 2, + "Tags": [] + }, "vtgate_gen4": { "File": "unused.go", "Args": ["vitess.io/vitess/go/test/endtoend/vtgate/gen4"], From 031cefe307004c6f669c6f8552b6c160b58bcb5c Mon Sep 17 00:00:00 2001 From: Harshit Gangal Date: Mon, 7 Aug 2023 12:43:29 +0530 Subject: [PATCH 08/12] addressed review comments Signed-off-by: Harshit Gangal --- .../{foreign_keys => foreignkey}/fk_test.go | 18 +++++------ .../{foreign_keys => foreignkey}/main_test.go | 30 +++++-------------- .../sharded_schema.sql | 0 .../sharded_vschema.json | 0 go/vt/sqlparser/ast_funcs.go | 8 ++--- go/vt/sqlparser/normalizer_test.go | 2 +- go/vt/vterrors/code.go | 3 +- .../vtgate/planbuilder/operators/fk_verify.go | 16 ++++++++++ go/vt/vtgate/vindexes/foreign_keys.go | 16 ++++++++++ go/vt/vtgate/vindexes/foreign_keys_test.go | 16 ++++++++++ 10 files changed, 72 insertions(+), 37 deletions(-) rename go/test/endtoend/vtgate/{foreign_keys => foreignkey}/fk_test.go (67%) rename go/test/endtoend/vtgate/{foreign_keys => foreignkey}/main_test.go (71%) rename go/test/endtoend/vtgate/{foreign_keys => foreignkey}/sharded_schema.sql (100%) rename go/test/endtoend/vtgate/{foreign_keys => foreignkey}/sharded_vschema.json (100%) diff --git a/go/test/endtoend/vtgate/foreign_keys/fk_test.go b/go/test/endtoend/vtgate/foreignkey/fk_test.go similarity index 67% rename from go/test/endtoend/vtgate/foreign_keys/fk_test.go rename to go/test/endtoend/vtgate/foreignkey/fk_test.go index 310f81674df..4095cd9f770 100644 --- a/go/test/endtoend/vtgate/foreign_keys/fk_test.go +++ b/go/test/endtoend/vtgate/foreignkey/fk_test.go @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -package foreign_keys +package foreignkey import ( "testing" @@ -26,26 +26,26 @@ import ( // TestInsertions tests that insertions work as expected when foreign key management is enabled in Vitess. func TestInsertions(t *testing.T) { - mcmp, closer := start(t) + conn, closer := start(t) defer closer() // insert some data. - utils.Exec(t, mcmp.VtConn, `insert into t1(id, col) values (100, 123),(10, 12),(1, 13),(1000, 1234)`) + utils.Exec(t, conn, `insert into t1(id, col) values (100, 123),(10, 12),(1, 13),(1000, 1234)`) // Verify that inserting data into a table that has shard scoped foreign keys works. - utils.Exec(t, mcmp.VtConn, `insert into t2(id, col) values (100, 125), (1, 132)`) + utils.Exec(t, conn, `insert into t2(id, col) values (100, 125), (1, 132)`) // Verify that insertion fails if the data doesn't follow the fk constraint. - _, err := utils.ExecAllowError(t, mcmp.VtConn, `insert into t2(id, col) values (1310, 125)`) + _, 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") // Verify that insertion fails if the table has cross-shard foreign keys (even if the data follows the constraints). - _, err = utils.ExecAllowError(t, mcmp.VtConn, `insert into t3(id, col) values (100, 100)`) + _, err = utils.ExecAllowError(t, conn, `insert into t3(id, col) values (100, 100)`) require.ErrorContains(t, err, "VT12002: unsupported cross-shard foreign keys") // insert some data in a table with multicol vindex. - utils.Exec(t, mcmp.VtConn, `insert into multicol_tbl1(cola, colb, colc, msg) values (100, 'a', 'b', 'msg'), (101, 'c', 'd', 'msg2')`) + utils.Exec(t, conn, `insert into multicol_tbl1(cola, colb, colc, msg) values (100, 'a', 'b', 'msg'), (101, 'c', 'd', 'msg2')`) // Verify that inserting data into a table that has shard scoped multi-column foreign keys works. - utils.Exec(t, mcmp.VtConn, `insert into multicol_tbl2(cola, colb, colc, msg) values (100, 'a', 'b', 'msg3')`) + utils.Exec(t, conn, `insert into multicol_tbl2(cola, colb, colc, msg) values (100, 'a', 'b', 'msg3')`) // Verify that insertion fails if the data doesn't follow the fk constraint. - _, err = utils.ExecAllowError(t, mcmp.VtConn, `insert into multicol_tbl2(cola, colb, colc, msg) values (103, 'c', 'd', 'msg2')`) + _, 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") } diff --git a/go/test/endtoend/vtgate/foreign_keys/main_test.go b/go/test/endtoend/vtgate/foreignkey/main_test.go similarity index 71% rename from go/test/endtoend/vtgate/foreign_keys/main_test.go rename to go/test/endtoend/vtgate/foreignkey/main_test.go index 3ac1875e99f..cf01ca37de0 100644 --- a/go/test/endtoend/vtgate/foreign_keys/main_test.go +++ b/go/test/endtoend/vtgate/foreignkey/main_test.go @@ -14,12 +14,12 @@ See the License for the specific language governing permissions and limitations under the License. */ -package foreign_keys +package foreignkey import ( + "context" _ "embed" "flag" - "fmt" "os" "testing" @@ -27,8 +27,6 @@ import ( "vitess.io/vitess/go/test/endtoend/utils" - "vitess.io/vitess/go/vt/vtgate/planbuilder" - "vitess.io/vitess/go/mysql" "vitess.io/vitess/go/test/endtoend/cluster" ) @@ -36,7 +34,6 @@ import ( var ( clusterInstance *cluster.LocalProcessCluster vtParams mysql.ConnParams - mysqlParams mysql.ConnParams shardedKs = "ks" shardedKsShards = []string{"-19a0", "19a0-20", "20-20c0", "20c0-"} Cell = "test" @@ -68,8 +65,6 @@ func TestMain(m *testing.M) { VSchema: shardedVSchema, } - clusterInstance.VtGateExtraArgs = []string{"--schema_change_signal"} - clusterInstance.VtTabletExtraArgs = []string{"--queryserver-config-schema-change-signal", "--queryserver-config-schema-change-signal-interval", "0.1"} err = clusterInstance.StartKeyspace(*sKs, shardedKsShards, 0, false) if err != nil { return 1 @@ -81,7 +76,6 @@ func TestMain(m *testing.M) { } // Start vtgate - clusterInstance.VtGatePlannerVersion = planbuilder.Gen4 // enable Gen4 planner. err = clusterInstance.StartVtgate() if err != nil { return 1 @@ -91,35 +85,27 @@ func TestMain(m *testing.M) { Port: clusterInstance.VtgateMySQLPort, } - conn, closer, err := utils.NewMySQL(clusterInstance, shardedKs, shardedSchemaSQL) - if err != nil { - fmt.Println(err) - return 1 - } - defer closer() - mysqlParams = conn return m.Run() }() os.Exit(exitCode) } -func start(t *testing.T) (utils.MySQLCompare, func()) { - mcmp, err := utils.NewMySQLCompare(t, vtParams, mysqlParams) +func start(t *testing.T) (*mysql.Conn, func()) { + conn, err := mysql.Connect(context.Background(), &vtParams) require.NoError(t, err) - deleteAll := func() { - _, _ = utils.ExecAllowError(t, mcmp.VtConn, "set workload = oltp") + deleteAll := func() { tables := []string{"t3", "t2", "t1", "multicol_tbl2", "multicol_tbl1"} for _, table := range tables { - _, _ = mcmp.ExecAndIgnore("delete from " + table) + _ = utils.Exec(t, conn, "delete from "+table) } } deleteAll() - return mcmp, func() { + return conn, func() { deleteAll() - mcmp.Close() + conn.Close() cluster.PanicHandler(t) } } diff --git a/go/test/endtoend/vtgate/foreign_keys/sharded_schema.sql b/go/test/endtoend/vtgate/foreignkey/sharded_schema.sql similarity index 100% rename from go/test/endtoend/vtgate/foreign_keys/sharded_schema.sql rename to go/test/endtoend/vtgate/foreignkey/sharded_schema.sql diff --git a/go/test/endtoend/vtgate/foreign_keys/sharded_vschema.json b/go/test/endtoend/vtgate/foreignkey/sharded_vschema.json similarity index 100% rename from go/test/endtoend/vtgate/foreign_keys/sharded_vschema.json rename to go/test/endtoend/vtgate/foreignkey/sharded_vschema.json diff --git a/go/vt/sqlparser/ast_funcs.go b/go/vt/sqlparser/ast_funcs.go index 11af88a6842..a88883339f4 100644 --- a/go/vt/sqlparser/ast_funcs.go +++ b/go/vt/sqlparser/ast_funcs.go @@ -2495,8 +2495,8 @@ func (ty KillType) ToString() string { } } -// Indexes returns if the list of columns contains all the elements in the other list provided. -// If it does, then it also returns the indexes of the columns. +// Indexes returns true, +// if the list of columns contains all the elements in the other list provided along with their indexes. func (cols Columns) Indexes(subSetCols Columns) (bool, []int) { var indexes []int for _, subSetCol := range subSetCols { @@ -2515,8 +2515,8 @@ func (cols Columns) Indexes(subSetCols Columns) (bool, []int) { return true, indexes } -// MakeColumns is to used to make a list of columns from a list of strings. -// This function is meeant to be used in testing code. +// MakeColumns is used to make a list of columns from a list of strings. +// This function is meant to be used in testing code. func MakeColumns(colNames ...string) Columns { var cols Columns for _, name := range colNames { diff --git a/go/vt/sqlparser/normalizer_test.go b/go/vt/sqlparser/normalizer_test.go index 44550d9d877..2b0a4b52122 100644 --- a/go/vt/sqlparser/normalizer_test.go +++ b/go/vt/sqlparser/normalizer_test.go @@ -444,7 +444,7 @@ func TestNormalizeOneCasae(t *testing.T) { output: "", } if testOne.input == "" { - return + t.Skip("empty test case") } tree, err := Parse(testOne.input) require.NoError(t, err, testOne.input) diff --git a/go/vt/vterrors/code.go b/go/vt/vterrors/code.go index a77446039db..9f178cc9a13 100644 --- a/go/vt/vterrors/code.go +++ b/go/vt/vterrors/code.go @@ -80,7 +80,7 @@ var ( VT10001 = errorWithoutState("VT10001", vtrpcpb.Code_ABORTED, "foreign key constraints are not allowed", "Foreign key constraints are not allowed, see https://vitess.io/blog/2021-06-15-online-ddl-why-no-fk/.") VT12001 = errorWithoutState("VT12001", vtrpcpb.Code_UNIMPLEMENTED, "unsupported: %s", "This statement is unsupported by Vitess. Please rewrite your query to use supported syntax.") - VT12002 = errorWithoutState("VT12002", vtrpcpb.Code_UNIMPLEMENTED, "unsupported cross-shard foreign keys", "Vitess does not support cross shard foreign key management.") + VT12002 = errorWithoutState("VT12002", vtrpcpb.Code_UNIMPLEMENTED, "unsupported: cross-shard foreign keys", "Vitess does not support cross shard foreign keys.") // VT13001 General Error VT13001 = errorWithoutState("VT13001", vtrpcpb.Code_INTERNAL, "[BUG] %s", "This error should not happen and is a bug. Please file an issue on GitHub: https://github.com/vitessio/vitess/issues/new/choose.") @@ -144,6 +144,7 @@ var ( VT09015, VT10001, VT12001, + VT12002, VT13001, VT13002, VT14001, diff --git a/go/vt/vtgate/planbuilder/operators/fk_verify.go b/go/vt/vtgate/planbuilder/operators/fk_verify.go index 1e403639a0a..8e3acd337c4 100644 --- a/go/vt/vtgate/planbuilder/operators/fk_verify.go +++ b/go/vt/vtgate/planbuilder/operators/fk_verify.go @@ -1,3 +1,19 @@ +/* +Copyright 2023 The Vitess Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + package operators import ( diff --git a/go/vt/vtgate/vindexes/foreign_keys.go b/go/vt/vtgate/vindexes/foreign_keys.go index f8afe297075..9be86b075da 100644 --- a/go/vt/vtgate/vindexes/foreign_keys.go +++ b/go/vt/vtgate/vindexes/foreign_keys.go @@ -1,3 +1,19 @@ +/* +Copyright 2023 The Vitess Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + package vindexes import ( diff --git a/go/vt/vtgate/vindexes/foreign_keys_test.go b/go/vt/vtgate/vindexes/foreign_keys_test.go index 706c191b7fb..e2218c1d525 100644 --- a/go/vt/vtgate/vindexes/foreign_keys_test.go +++ b/go/vt/vtgate/vindexes/foreign_keys_test.go @@ -1,3 +1,19 @@ +/* +Copyright 2023 The Vitess Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + package vindexes import ( From 6302e15d12821508877a8af7ad79accc9bb17e0c Mon Sep 17 00:00:00 2001 From: Harshit Gangal Date: Mon, 7 Aug 2023 13:47:10 +0530 Subject: [PATCH 09/12] fix test expectation Signed-off-by: Harshit Gangal --- go/test/endtoend/vtgate/foreignkey/fk_test.go | 2 +- go/vt/vtgate/planbuilder/testdata/fk_cases.json | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/go/test/endtoend/vtgate/foreignkey/fk_test.go b/go/test/endtoend/vtgate/foreignkey/fk_test.go index 4095cd9f770..d61aea0aed9 100644 --- a/go/test/endtoend/vtgate/foreignkey/fk_test.go +++ b/go/test/endtoend/vtgate/foreignkey/fk_test.go @@ -39,7 +39,7 @@ func TestInsertions(t *testing.T) { require.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") + require.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')`) diff --git a/go/vt/vtgate/planbuilder/testdata/fk_cases.json b/go/vt/vtgate/planbuilder/testdata/fk_cases.json index 500c58b8972..3d244086667 100644 --- a/go/vt/vtgate/planbuilder/testdata/fk_cases.json +++ b/go/vt/vtgate/planbuilder/testdata/fk_cases.json @@ -2,7 +2,7 @@ { "comment": "Insertion in a table with cross-shard foreign keys disallowed", "query": "insert into tbl3 (col3, coly) values (1, 3)", - "plan": "VT12002: unsupported cross-shard foreign keys" + "plan": "VT12002: unsupported: cross-shard foreign keys" }, { "comment": "Insertion in a table with shard-scoped foreign keys is allowed", From d5341a079b21e0ec087f540c87fde4b7a00176d2 Mon Sep 17 00:00:00 2001 From: Harshit Gangal Date: Mon, 7 Aug 2023 15:00:26 +0530 Subject: [PATCH 10/12] fix test config Signed-off-by: Harshit Gangal --- test/config.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/config.json b/test/config.json index 903331f2600..b46152462c4 100644 --- a/test/config.json +++ b/test/config.json @@ -842,9 +842,9 @@ "RetryMax": 1, "Tags": [] }, - "vtgate_foreign_keys": { + "vtgate_foreignkey": { "File": "unused.go", - "Args": ["vitess.io/vitess/go/test/endtoend/vtgate/foreign_keys"], + "Args": ["vitess.io/vitess/go/test/endtoend/vtgate/foreignkey"], "Command": [], "Manual": false, "Shard": "vtgate_gen4", From e927865053d9341b2ce1bc715abe7c1ebef44fb6 Mon Sep 17 00:00:00 2001 From: Harshit Gangal Date: Mon, 7 Aug 2023 15:38:03 +0530 Subject: [PATCH 11/12] minor refactor Signed-off-by: Harshit Gangal --- go/test/endtoend/vtgate/foreignkey/main_test.go | 9 ++------- go/vt/sqlparser/ast_funcs.go | 4 ++-- go/vt/vtgate/planbuilder/plan_test.go | 2 +- .../testdata/{fk_cases.json => foreignkey_cases.json} | 0 4 files changed, 5 insertions(+), 10 deletions(-) rename go/vt/vtgate/planbuilder/testdata/{fk_cases.json => foreignkey_cases.json} (100%) diff --git a/go/test/endtoend/vtgate/foreignkey/main_test.go b/go/test/endtoend/vtgate/foreignkey/main_test.go index cf01ca37de0..eaeae4eeb9b 100644 --- a/go/test/endtoend/vtgate/foreignkey/main_test.go +++ b/go/test/endtoend/vtgate/foreignkey/main_test.go @@ -35,8 +35,8 @@ var ( clusterInstance *cluster.LocalProcessCluster vtParams mysql.ConnParams shardedKs = "ks" - shardedKsShards = []string{"-19a0", "19a0-20", "20-20c0", "20c0-"} Cell = "test" + //go:embed sharded_schema.sql shardedSchemaSQL string @@ -65,12 +65,7 @@ func TestMain(m *testing.M) { VSchema: shardedVSchema, } - err = clusterInstance.StartKeyspace(*sKs, shardedKsShards, 0, false) - if err != nil { - return 1 - } - - err = clusterInstance.VtctlclientProcess.ExecuteCommand("RebuildVSchemaGraph") + err = clusterInstance.StartKeyspace(*sKs, []string{"-80", "80-"}, 0, false) if err != nil { return 1 } diff --git a/go/vt/sqlparser/ast_funcs.go b/go/vt/sqlparser/ast_funcs.go index a88883339f4..3c8e917bb5d 100644 --- a/go/vt/sqlparser/ast_funcs.go +++ b/go/vt/sqlparser/ast_funcs.go @@ -2495,8 +2495,8 @@ func (ty KillType) ToString() string { } } -// Indexes returns true, -// if the list of columns contains all the elements in the other list provided along with their indexes. +// Indexes returns true, if the list of columns contains all the elements in the other list. +// It also returns the indexes of the columns in the list. func (cols Columns) Indexes(subSetCols Columns) (bool, []int) { var indexes []int for _, subSetCol := range subSetCols { diff --git a/go/vt/vtgate/planbuilder/plan_test.go b/go/vt/vtgate/planbuilder/plan_test.go index c15e7074190..c8cdfbf7a0a 100644 --- a/go/vt/vtgate/planbuilder/plan_test.go +++ b/go/vt/vtgate/planbuilder/plan_test.go @@ -112,7 +112,7 @@ func TestForeignKeyPlanning(t *testing.T) { } testOutputTempDir := makeTestOutput(t) - testFile(t, "fk_cases.json", testOutputTempDir, vschemaWrapper, false) + testFile(t, "foreignkey_cases.json", testOutputTempDir, vschemaWrapper, false) } func TestSystemTables57(t *testing.T) { diff --git a/go/vt/vtgate/planbuilder/testdata/fk_cases.json b/go/vt/vtgate/planbuilder/testdata/foreignkey_cases.json similarity index 100% rename from go/vt/vtgate/planbuilder/testdata/fk_cases.json rename to go/vt/vtgate/planbuilder/testdata/foreignkey_cases.json From 71c09a9f74a3b140b48b9707c600ee62650a137c Mon Sep 17 00:00:00 2001 From: Harshit Gangal Date: Mon, 7 Aug 2023 23:14:17 +0530 Subject: [PATCH 12/12] remove fk_verify operator, refactors the fk test Signed-off-by: Harshit Gangal --- go/vt/vtgate/planbuilder/insert.go | 5 +- .../planbuilder/operator_transformers.go | 9 - go/vt/vtgate/planbuilder/operators/ast2op.go | 5 +- .../vtgate/planbuilder/operators/fk_verify.go | 140 ------ go/vt/vtgate/planbuilder/operators/insert.go | 5 - go/vt/vtgate/vindexes/foreign_keys.go | 3 +- go/vt/vtgate/vindexes/foreign_keys_test.go | 427 +++++------------- 7 files changed, 116 insertions(+), 478 deletions(-) delete mode 100644 go/vt/vtgate/planbuilder/operators/fk_verify.go diff --git a/go/vt/vtgate/planbuilder/insert.go b/go/vt/vtgate/planbuilder/insert.go index 22de52afa6a..864d1056908 100644 --- a/go/vt/vtgate/planbuilder/insert.go +++ b/go/vt/vtgate/planbuilder/insert.go @@ -101,9 +101,8 @@ func insertUnshardedShortcut(stmt *sqlparser.Insert, ks *vindexes.Keyspace, tabl } type insert struct { - eInsert *engine.Insert - source logicalPlan - fkVerify logicalPlan + eInsert *engine.Insert + source logicalPlan } var _ logicalPlan = (*insert)(nil) diff --git a/go/vt/vtgate/planbuilder/operator_transformers.go b/go/vt/vtgate/planbuilder/operator_transformers.go index 120368c1e36..057f2f5713d 100644 --- a/go/vt/vtgate/planbuilder/operator_transformers.go +++ b/go/vt/vtgate/planbuilder/operator_transformers.go @@ -66,8 +66,6 @@ func transformToLogicalPlan(ctx *plancontext.PlanningContext, op ops.Operator, i return transformAggregator(ctx, op) case *operators.Distinct: return transformDistinct(ctx, op) - case *operators.FKVerify: - return nil, vterrors.VT12002() } return nil, vterrors.VT13001(fmt.Sprintf("unknown type encountered: %T (transformToLogicalPlan)", op)) @@ -429,13 +427,6 @@ func transformInsertPlan(ctx *plancontext.PlanningContext, op *operators.Route, } } - if ins.FKVerify != nil { - i.fkVerify, err = transformToLogicalPlan(ctx, ins.FKVerify, true) - if err != nil { - return - } - } - return } diff --git a/go/vt/vtgate/planbuilder/operators/ast2op.go b/go/vt/vtgate/planbuilder/operators/ast2op.go index a80033de23d..d81670a4921 100644 --- a/go/vt/vtgate/planbuilder/operators/ast2op.go +++ b/go/vt/vtgate/planbuilder/operators/ast2op.go @@ -269,10 +269,7 @@ func createOperatorFromInsert(ctx *plancontext.PlanningContext, ins *sqlparser.I if ksMode == vschemapb.Keyspace_FK_MANAGED { parentFKs := vindexTable.CrossShardParentFKs() if len(parentFKs) > 0 { - insOp.FKVerify, err = NewFkVerify(ctx, parentFKs, ins.Columns) - if err != nil { - return nil, err - } + return nil, vterrors.VT12002() } } diff --git a/go/vt/vtgate/planbuilder/operators/fk_verify.go b/go/vt/vtgate/planbuilder/operators/fk_verify.go deleted file mode 100644 index 8e3acd337c4..00000000000 --- a/go/vt/vtgate/planbuilder/operators/fk_verify.go +++ /dev/null @@ -1,140 +0,0 @@ -/* -Copyright 2023 The Vitess Authors. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package operators - -import ( - "vitess.io/vitess/go/vt/sqlparser" - "vitess.io/vitess/go/vt/vtgate/planbuilder/operators/ops" - "vitess.io/vitess/go/vt/vtgate/planbuilder/plancontext" - "vitess.io/vitess/go/vt/vtgate/semantics" - "vitess.io/vitess/go/vt/vtgate/vindexes" -) - -// FKVerify represents an insert operation on a table. -type FKVerify struct { - // Sources are used to run the validation queries. - Sources []ops.Operator - InputOffsets [][]int - - // ParentFKs to verify during insert planning - ParentFKs []vindexes.ParentFKInfo - - noColumns - noPredicates -} - -func (fkVerify *FKVerify) Inputs() []ops.Operator { - return fkVerify.Sources -} - -func (fkVerify *FKVerify) SetInputs(operators []ops.Operator) { - fkVerify.Sources = operators -} - -func (fkVerify *FKVerify) ShortDescription() string { - return "FKVerify" -} - -func (fkVerify *FKVerify) GetOrdering() ([]ops.OrderBy, error) { - panic("does not expect insert operator to receive get ordering call") -} - -// Clone will return a copy of this operator, protected so changed to the original will not impact the clone -func (fkVerify *FKVerify) Clone(inputs []ops.Operator) ops.Operator { - return &FKVerify{ - Sources: inputs, - } -} - -// NewFkVerify creates a new FkVerify operator given the parent foreign key validations we want it to run. -func NewFkVerify(ctx *plancontext.PlanningContext, parentFKs []vindexes.ParentFKInfo, insertColumns sqlparser.Columns) (*FKVerify, error) { - var verifyQueryOps []ops.Operator - var inputOffsets [][]int - for _, fk := range parentFKs { - // Build the query for verification - query := &sqlparser.Select{ - SelectExprs: []sqlparser.SelectExpr{ - sqlparser.NewAliasedExpr( - &sqlparser.Count{ - Args: createColNameList(fk.ParentColumns), - }, - "", - ), - }, - From: []sqlparser.TableExpr{ - sqlparser.NewAliasedTableExpr(sqlparser.NewTableName(fk.Table.Name.String()), ""), - }, - Where: sqlparser.NewWhere( - sqlparser.WhereClause, - sqlparser.NewComparisonExpr( - sqlparser.InOp, - createLeftExpr(fk.ParentColumns), - sqlparser.NewListArg("fkInput"), // TODO: Make this a constant in the engine package. - nil, - ), - ), - } - - newSemTable, err := semantics.Analyze(query, fk.Table.Keyspace.Name, ctx.VSchema) - if err != nil { - return nil, err - } - newCtx := plancontext.NewPlanningContext(nil, newSemTable, ctx.VSchema, ctx.PlannerVersion) - op, err := PlanQuery(newCtx, query) - if err != nil { - return nil, err - } - verifyQueryOps = append(verifyQueryOps, op) - - // Build the offsets - var offsets []int - outer: - for _, column := range fk.ChildColumns { - for idx, insertColumn := range insertColumns { - if column.Equal(insertColumn) { - offsets = append(offsets, idx) - continue outer - } - } - offsets = append(offsets, -1) - } - inputOffsets = append(inputOffsets, offsets) - } - - return &FKVerify{ - Sources: verifyQueryOps, - InputOffsets: inputOffsets, - }, nil -} - -// createColNameList creates a list of ColNames from a list of Columns -func createColNameList(columns sqlparser.Columns) []sqlparser.Expr { - var colNames []sqlparser.Expr - for _, col := range columns { - colNames = append(colNames, sqlparser.NewColName(col.String())) - } - return colNames -} - -// createLeftExpr creates a valtuple if there are more than 1 columns, otherwise it just returns a ColName. -func createLeftExpr(columns sqlparser.Columns) sqlparser.Expr { - valTuple := createColNameList(columns) - if len(valTuple) == 1 { - return valTuple[0] - } - return sqlparser.ValTuple(valTuple) -} diff --git a/go/vt/vtgate/planbuilder/operators/insert.go b/go/vt/vtgate/planbuilder/operators/insert.go index 40fea4f22d5..3fc70ed8998 100644 --- a/go/vt/vtgate/planbuilder/operators/insert.go +++ b/go/vt/vtgate/planbuilder/operators/insert.go @@ -50,10 +50,6 @@ type Insert struct { // Insert using select query will have select plan as input operator for the insert operation. Input ops.Operator - // FKVerify is required to check the validity of foreign key constraints that aren't shard scoped. - // It is nil if no verification is required. - FKVerify ops.Operator - noColumns noPredicates } @@ -115,7 +111,6 @@ func (i *Insert) Clone(inputs []ops.Operator) ops.Operator { ColVindexes: i.ColVindexes, VindexValues: i.VindexValues, VindexValueOffset: i.VindexValueOffset, - FKVerify: i.FKVerify, } } diff --git a/go/vt/vtgate/vindexes/foreign_keys.go b/go/vt/vtgate/vindexes/foreign_keys.go index 9be86b075da..dca74d163d7 100644 --- a/go/vt/vtgate/vindexes/foreign_keys.go +++ b/go/vt/vtgate/vindexes/foreign_keys.go @@ -112,7 +112,6 @@ func (t *Table) CrossShardParentFKs() (crossShardFKs []ParentFKInfo) { if len(t.ParentForeignKeys) == 0 { return } - primaryVindex := t.ColumnVindexes[0] for _, fk := range t.ParentForeignKeys { // If the keyspaces are different, then the fk definition // is going to go across shards. @@ -125,8 +124,10 @@ func (t *Table) CrossShardParentFKs() (crossShardFKs []ParentFKInfo) { if !t.Keyspace.Sharded { continue } + // If the primary vindexes don't match between the parent and child table, // we cannot infer that the fk constraint in shard scoped. + primaryVindex := t.ColumnVindexes[0] if fk.Table.ColumnVindexes[0].Vindex != primaryVindex.Vindex { crossShardFKs = append(crossShardFKs, fk) continue diff --git a/go/vt/vtgate/vindexes/foreign_keys_test.go b/go/vt/vtgate/vindexes/foreign_keys_test.go index e2218c1d525..b56bdf2f062 100644 --- a/go/vt/vtgate/vindexes/foreign_keys_test.go +++ b/go/vt/vtgate/vindexes/foreign_keys_test.go @@ -24,330 +24,117 @@ import ( "vitess.io/vitess/go/vt/sqlparser" ) +var ( + uks = &Keyspace{Name: "uks"} + uks2 = &Keyspace{Name: "uks2"} + sks = &Keyspace{Name: "sks", Sharded: true} +) + // TestTable_CrossShardParentFKs tests the functionality of the method CrossShardParentFKs. func TestTable_CrossShardParentFKs(t *testing.T) { + col1Vindex := &ColumnVindex{ + Name: "v1", + Vindex: binVindex, + Columns: sqlparser.MakeColumns("col1"), + } + col4DiffVindex := &ColumnVindex{ + Name: "v2", + Vindex: binOnlyVindex, + Columns: sqlparser.MakeColumns("col4"), + } + col123Vindex := &ColumnVindex{ + Name: "v2", + Vindex: binVindex, + Columns: sqlparser.MakeColumns("col1", "col2", "col3"), + } + col456Vindex := &ColumnVindex{ + Name: "v2", + Vindex: binVindex, + Columns: sqlparser.MakeColumns("col4", "col5", "col6"), + } + + unshardedTbl := &Table{ + Name: sqlparser.NewIdentifierCS("t1"), + Keyspace: uks2, + } + shardedSingleColTblWithDiffVindex := &Table{ + Name: sqlparser.NewIdentifierCS("t1"), + Keyspace: sks, + ColumnVindexes: []*ColumnVindex{col4DiffVindex}, + } + shardedMultiColTbl := &Table{ + Name: sqlparser.NewIdentifierCS("t1"), + Keyspace: sks, + ColumnVindexes: []*ColumnVindex{col456Vindex}, + } + tests := []struct { name string table *Table wantCrossShardFKTables []string - }{ - { - name: "Unsharded keyspace", - table: &Table{ - ColumnVindexes: []*ColumnVindex{ - { - Name: "v1", - Vindex: binVindex, - Columns: []sqlparser.IdentifierCI{ - sqlparser.NewIdentifierCI("col1"), - }, - }, - }, - Keyspace: &Keyspace{ - Name: "ks", - Sharded: false, - }, - ParentForeignKeys: []ParentFKInfo{ - { - Table: &Table{ - Name: sqlparser.NewIdentifierCS("t1"), - Keyspace: &Keyspace{ - Name: "ks", - Sharded: false, - }, - ColumnVindexes: []*ColumnVindex{ - { - Name: "v2", - Vindex: binVindex, - Columns: []sqlparser.IdentifierCI{ - sqlparser.NewIdentifierCI("col4"), - }, - }, - }, - }, - ChildColumns: sqlparser.MakeColumns("col1"), - ParentColumns: sqlparser.MakeColumns("col3"), - }, - }, - }, - wantCrossShardFKTables: []string{}, + }{{ + name: "No Parent FKs", + table: &Table{ + ColumnVindexes: []*ColumnVindex{col1Vindex}, + Keyspace: sks, }, - { - name: "No Parent FKs", - table: &Table{ - ColumnVindexes: []*ColumnVindex{ - { - Name: "v1", - Vindex: binVindex, - Columns: []sqlparser.IdentifierCI{ - sqlparser.NewIdentifierCI("col1"), - }, - }, - }, - Keyspace: &Keyspace{ - Name: "ks", - Sharded: true, - }, - }, - wantCrossShardFKTables: []string{}, - }, { - name: "Keyspaces don't match", - table: &Table{ - ColumnVindexes: []*ColumnVindex{ - { - Name: "v1", - Vindex: binVindex, - Columns: []sqlparser.IdentifierCI{ - sqlparser.NewIdentifierCI("col1"), - }, - }, - }, - Keyspace: &Keyspace{ - Name: "ks", - Sharded: false, - }, - ParentForeignKeys: []ParentFKInfo{ - { - Table: &Table{ - Name: sqlparser.NewIdentifierCS("t1"), - Keyspace: &Keyspace{ - Name: "ks2", - Sharded: false, - }, - ColumnVindexes: []*ColumnVindex{ - { - Name: "v2", - Vindex: binVindex, - Columns: []sqlparser.IdentifierCI{ - sqlparser.NewIdentifierCI("col4"), - }, - }, - }, - }, - ChildColumns: sqlparser.MakeColumns("col1"), - ParentColumns: sqlparser.MakeColumns("col4"), - }, - }, - }, - wantCrossShardFKTables: []string{"t1"}, - }, { - name: "Column Vindexes don't match", - table: &Table{ - ColumnVindexes: []*ColumnVindex{ - { - Name: "v1", - Vindex: binVindex, - Columns: []sqlparser.IdentifierCI{ - sqlparser.NewIdentifierCI("col1"), - }, - }, - }, - Keyspace: &Keyspace{ - Name: "ks", - Sharded: true, - }, - ParentForeignKeys: []ParentFKInfo{ - { - Table: &Table{ - Name: sqlparser.NewIdentifierCS("t1"), - Keyspace: &Keyspace{ - Name: "ks", - Sharded: true, - }, - ColumnVindexes: []*ColumnVindex{ - { - Name: "v2", - Vindex: binOnlyVindex, - Columns: []sqlparser.IdentifierCI{ - sqlparser.NewIdentifierCI("col4"), - }, - }, - }, - }, - ChildColumns: sqlparser.MakeColumns("col1"), - ParentColumns: sqlparser.MakeColumns("col4"), - }, - }, - }, - wantCrossShardFKTables: []string{"t1"}, - }, { - name: "Child FK doesn't contain primary vindex", - table: &Table{ - ColumnVindexes: []*ColumnVindex{ - { - Name: "v1", - Vindex: binVindex, - Columns: []sqlparser.IdentifierCI{ - sqlparser.NewIdentifierCI("col1"), - sqlparser.NewIdentifierCI("col2"), - sqlparser.NewIdentifierCI("col3"), - }, - }, - }, - Keyspace: &Keyspace{ - Name: "ks", - Sharded: true, - }, - ParentForeignKeys: []ParentFKInfo{ - { - Table: &Table{ - Name: sqlparser.NewIdentifierCS("t1"), - Keyspace: &Keyspace{ - Name: "ks", - Sharded: true, - }, - ColumnVindexes: []*ColumnVindex{ - { - Name: "v2", - Vindex: binVindex, - Columns: []sqlparser.IdentifierCI{ - sqlparser.NewIdentifierCI("col4"), - sqlparser.NewIdentifierCI("col5"), - sqlparser.NewIdentifierCI("col6"), - }, - }, - }, - }, - ChildColumns: sqlparser.MakeColumns("col3", "col9", "col1"), - ParentColumns: sqlparser.MakeColumns("col4", "col5", "col6"), - }, - }, - }, - wantCrossShardFKTables: []string{"t1"}, - }, { - name: "Parent FK doesn't contain primary vindex", - table: &Table{ - ColumnVindexes: []*ColumnVindex{ - { - Name: "v1", - Vindex: binVindex, - Columns: []sqlparser.IdentifierCI{ - sqlparser.NewIdentifierCI("col1"), - sqlparser.NewIdentifierCI("col2"), - sqlparser.NewIdentifierCI("col3"), - }, - }, - }, - Keyspace: &Keyspace{ - Name: "ks", - Sharded: true, - }, - ParentForeignKeys: []ParentFKInfo{ - { - Table: &Table{ - Name: sqlparser.NewIdentifierCS("t1"), - Keyspace: &Keyspace{ - Name: "ks", - Sharded: true, - }, - ColumnVindexes: []*ColumnVindex{ - { - Name: "v2", - Vindex: binVindex, - Columns: []sqlparser.IdentifierCI{ - sqlparser.NewIdentifierCI("col4"), - sqlparser.NewIdentifierCI("col5"), - sqlparser.NewIdentifierCI("col6"), - }, - }, - }, - }, - ChildColumns: sqlparser.MakeColumns("col1", "col2", "col3"), - ParentColumns: sqlparser.MakeColumns("col4", "col9", "col6"), - }, - }, - }, - wantCrossShardFKTables: []string{"t1"}, - }, { - name: "Indexes of the two FKs with column vindexes don't line up", - table: &Table{ - ColumnVindexes: []*ColumnVindex{ - { - Name: "v1", - Vindex: binVindex, - Columns: []sqlparser.IdentifierCI{ - sqlparser.NewIdentifierCI("col1"), - sqlparser.NewIdentifierCI("col2"), - sqlparser.NewIdentifierCI("col3"), - }, - }, - }, - Keyspace: &Keyspace{ - Name: "ks", - Sharded: true, - }, - ParentForeignKeys: []ParentFKInfo{ - { - Table: &Table{ - Name: sqlparser.NewIdentifierCS("t1"), - Keyspace: &Keyspace{ - Name: "ks", - Sharded: true, - }, - ColumnVindexes: []*ColumnVindex{ - { - Name: "v2", - Vindex: binVindex, - Columns: []sqlparser.IdentifierCI{ - sqlparser.NewIdentifierCI("col4"), - sqlparser.NewIdentifierCI("col5"), - sqlparser.NewIdentifierCI("col6"), - }, - }, - }, - }, - ChildColumns: sqlparser.MakeColumns("col1", "col2", "col3", "col9"), - ParentColumns: sqlparser.MakeColumns("col4", "col9", "col5", "col6"), - }, - }, - }, - wantCrossShardFKTables: []string{"t1"}, - }, { - name: "Shard scoped foreign key constraint", - table: &Table{ - ColumnVindexes: []*ColumnVindex{ - { - Name: "v1", - Vindex: binVindex, - Columns: []sqlparser.IdentifierCI{ - sqlparser.NewIdentifierCI("col1"), - sqlparser.NewIdentifierCI("col2"), - sqlparser.NewIdentifierCI("col3"), - }, - }, - }, - Keyspace: &Keyspace{ - Name: "ks", - Sharded: true, - }, - ParentForeignKeys: []ParentFKInfo{ - { - Table: &Table{ - Name: sqlparser.NewIdentifierCS("t1"), - Keyspace: &Keyspace{ - Name: "ks", - Sharded: true, - }, - ColumnVindexes: []*ColumnVindex{ - { - Name: "v2", - Vindex: binVindex, - Columns: []sqlparser.IdentifierCI{ - sqlparser.NewIdentifierCI("col4"), - sqlparser.NewIdentifierCI("col5"), - sqlparser.NewIdentifierCI("col6"), - }, - }, - }, - }, - ChildColumns: sqlparser.MakeColumns("col1", "cola", "col2", "col3", "colb"), - ParentColumns: sqlparser.MakeColumns("col4", "col9", "col5", "col6", "colc"), - }, - }, - }, - wantCrossShardFKTables: []string{}, + wantCrossShardFKTables: []string{}, + }, { + name: "Unsharded keyspace", + table: &Table{ + ColumnVindexes: []*ColumnVindex{col1Vindex}, + Keyspace: uks2, + ParentForeignKeys: []ParentFKInfo{pkInfo(unshardedTbl, []string{"col4"}, []string{"col1"})}, }, - } + wantCrossShardFKTables: []string{}, + }, { + name: "Keyspaces don't match", // parent table is on uks2 + table: &Table{ + Keyspace: uks, + ParentForeignKeys: []ParentFKInfo{pkInfo(unshardedTbl, []string{"col4"}, []string{"col1"})}, + }, + wantCrossShardFKTables: []string{"t1"}, + }, { + name: "Column Vindexes don't match", // primary vindexes on different vindex type + table: &Table{ + Keyspace: sks, + ColumnVindexes: []*ColumnVindex{col1Vindex}, + ParentForeignKeys: []ParentFKInfo{pkInfo(shardedSingleColTblWithDiffVindex, []string{"col4"}, []string{"col1"})}, + }, + wantCrossShardFKTables: []string{"t1"}, + }, { + name: "child table foreign key does not contain primary vindex columns", + table: &Table{ + Keyspace: sks, + ColumnVindexes: []*ColumnVindex{col123Vindex}, + ParentForeignKeys: []ParentFKInfo{pkInfo(shardedMultiColTbl, []string{"col4", "col5", "col6"}, []string{"col3", "col9", "col1"})}, + }, + wantCrossShardFKTables: []string{"t1"}, + }, { + name: "Parent FK doesn't contain primary vindex", + table: &Table{ + Keyspace: sks, + ColumnVindexes: []*ColumnVindex{col123Vindex}, + ParentForeignKeys: []ParentFKInfo{pkInfo(shardedMultiColTbl, []string{"col4", "col9", "col6"}, []string{"col1", "col2", "col3"})}, + }, + wantCrossShardFKTables: []string{"t1"}, + }, { + name: "Indexes of the two FKs with column vindexes don't line up", + table: &Table{ + Keyspace: sks, + ColumnVindexes: []*ColumnVindex{col123Vindex}, + ParentForeignKeys: []ParentFKInfo{pkInfo(shardedMultiColTbl, []string{"col4", "col9", "col5", "col6"}, []string{"col1", "col2", "col3", "col9"})}, + }, + wantCrossShardFKTables: []string{"t1"}, + }, { + name: "Shard scoped foreign key constraint", + table: &Table{ + Keyspace: sks, + ColumnVindexes: []*ColumnVindex{col123Vindex}, + ParentForeignKeys: []ParentFKInfo{pkInfo(shardedMultiColTbl, []string{"col4", "col9", "col5", "col6", "colc"}, []string{"col1", "cola", "col2", "col3", "colb"})}, + }, + wantCrossShardFKTables: []string{}, + }} for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { crossShardFks := tt.table.CrossShardParentFKs() @@ -359,3 +146,11 @@ func TestTable_CrossShardParentFKs(t *testing.T) { }) } } + +func pkInfo(parentTable *Table, pCols []string, cCols []string) ParentFKInfo { + return ParentFKInfo{ + Table: parentTable, + ParentColumns: sqlparser.MakeColumns(pCols...), + ChildColumns: sqlparser.MakeColumns(cCols...), + } +}