Skip to content

Commit

Permalink
[release-19.0] Fail insert when primary vindex cannot be mapped to a …
Browse files Browse the repository at this point in the history 
…shard (#15500) (#15573)

Signed-off-by: Harshit Gangal <[email protected]>
Co-authored-by: Harshit Gangal <[email protected]>
  • Loading branch information
@GrahamCampbell @harshit-gangal
GrahamCampbell and harshit-gangal authored Mar 26, 2024
1 parent 5950765 commit 3ce8f87
Show file tree
Hide file tree
Showing 4 changed files with 215 additions and 152 deletions.
6 changes: 6 additions & 0 deletions go/vt/vterrors/code.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,9 @@ var (
VT09017 = errorWithoutState("VT09017", vtrpcpb.Code_FAILED_PRECONDITION, "%s", "Invalid syntax for the statement type.")
VT09018 = errorWithoutState("VT09018", vtrpcpb.Code_FAILED_PRECONDITION, "%s", "Invalid syntax for the vindex function statement.")
VT09019 = errorWithoutState("VT09019", vtrpcpb.Code_FAILED_PRECONDITION, "keyspace '%s' has cyclic foreign keys", "Vitess doesn't support cyclic foreign keys.")
VT09022 = errorWithoutState("VT09022", vtrpcpb.Code_FAILED_PRECONDITION, "Destination does not have exactly one shard: %v", "Cannot send query to multiple shards.")
VT09023 = errorWithoutState("VT09023", vtrpcpb.Code_FAILED_PRECONDITION, "could not map %v to a keyspace id", "Unable to determine the shard for the given row.")
VT09024 = errorWithoutState("VT09024", vtrpcpb.Code_FAILED_PRECONDITION, "could not map %v to a unique keyspace id: %v", "Unable to determine the shard for the given row.")

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/.")
VT10002 = errorWithoutState("VT10002", vtrpcpb.Code_ABORTED, "'replace into' with foreign key constraints are not allowed", "Foreign key constraints sometimes are not written in binary logs and will cause issue with vreplication workflows like online-ddl.")
Expand Down Expand Up @@ -168,6 +171,9 @@ var (
VT09017,
VT09018,
VT09019,
VT09022,
VT09023,
VT09024,
VT10001,
VT10002,
VT12001,
Expand Down
11 changes: 5 additions & 6 deletions go/vt/vtgate/engine/insert_common.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ func (ins *InsertCommon) executeUnshardedTableQuery(ctx context.Context, vcursor
return nil, err
}
if len(rss) != 1 {
return nil, vterrors.Errorf(vtrpcpb.Code_FAILED_PRECONDITION, "Keyspace does not have exactly one shard: %v", rss)
return nil, vterrors.VT09022(rss)
}
err = allowOnlyPrimary(rss...)
if err != nil {
Expand Down Expand Up @@ -208,12 +208,11 @@ func (ic *InsertCommon) processPrimary(ctx context.Context, vcursor VCursor, vin
// This is a single keyspace id, we're good.
keyspaceIDs[i] = d
case key.DestinationNone:
// No valid keyspace id, we may return an error.
if !ic.Ignore {
return nil, fmt.Errorf("could not map %v to a keyspace id", vindexColumnsKeys[i])
}
// Not a valid keyspace id, so we cannot determine which shard this row belongs to.
// We have to return an error.
return nil, vterrors.VT09023(vindexColumnsKeys[i])
default:
return nil, fmt.Errorf("could not map %v to a unique keyspace id: %v", vindexColumnsKeys[i], destination)
return nil, vterrors.VT09024(vindexColumnsKeys[i], destination)
}
}

Expand Down
33 changes: 13 additions & 20 deletions go/vt/vtgate/engine/insert_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ func TestInsertUnsharded(t *testing.T) {

vc = &loggingVCursor{}
_, err = ins.TryExecute(context.Background(), vc, map[string]*querypb.BindVariable{}, false)
require.EqualError(t, err, `Keyspace does not have exactly one shard: []`)
require.EqualError(t, err, `VT09022: Destination does not have exactly one shard: []`)
}

func TestInsertUnshardedGenerate(t *testing.T) {
Expand Down Expand Up @@ -515,7 +515,7 @@ func TestInsertShardedFail(t *testing.T) {

// The lookup will fail to map to a keyspace id.
_, err := ins.TryExecute(context.Background(), vc, map[string]*querypb.BindVariable{}, false)
require.EqualError(t, err, `could not map [INT64(1)] to a keyspace id`)
require.EqualError(t, err, `VT09023: could not map [INT64(1)] to a keyspace id`)
}

func TestInsertShardedGenerate(t *testing.T) {
Expand Down Expand Up @@ -964,7 +964,6 @@ func TestInsertShardedIgnoreOwned(t *testing.T) {
// rows for id

evalengine.NewLiteralInt(1),
evalengine.NewLiteralInt(2),
evalengine.NewLiteralInt(3),
evalengine.NewLiteralInt(4),
},
Expand All @@ -973,14 +972,12 @@ func TestInsertShardedIgnoreOwned(t *testing.T) {
{
// rows for c1
evalengine.NewLiteralInt(5),
evalengine.NewLiteralInt(6),
evalengine.NewLiteralInt(7),
evalengine.NewLiteralInt(8),
},
{
// rows for c2
evalengine.NewLiteralInt(9),
evalengine.NewLiteralInt(10),
evalengine.NewLiteralInt(11),
evalengine.NewLiteralInt(12),
},
Expand All @@ -989,7 +986,6 @@ func TestInsertShardedIgnoreOwned(t *testing.T) {
{
// rows for c3
evalengine.NewLiteralInt(13),
evalengine.NewLiteralInt(14),
evalengine.NewLiteralInt(15),
evalengine.NewLiteralInt(16),
},
Expand All @@ -1000,7 +996,6 @@ func TestInsertShardedIgnoreOwned(t *testing.T) {
{&sqlparser.Argument{Name: "_id_0", Type: sqltypes.Int64}, &sqlparser.Argument{Name: "_c1_0", Type: sqltypes.Int64}, &sqlparser.Argument{Name: "_c2_0", Type: sqltypes.Int64}, &sqlparser.Argument{Name: "_c3_0", Type: sqltypes.Int64}},
{&sqlparser.Argument{Name: "_id_1", Type: sqltypes.Int64}, &sqlparser.Argument{Name: "_c1_1", Type: sqltypes.Int64}, &sqlparser.Argument{Name: "_c2_1", Type: sqltypes.Int64}, &sqlparser.Argument{Name: "_c3_1", Type: sqltypes.Int64}},
{&sqlparser.Argument{Name: "_id_2", Type: sqltypes.Int64}, &sqlparser.Argument{Name: "_c1_2", Type: sqltypes.Int64}, &sqlparser.Argument{Name: "_c2_2", Type: sqltypes.Int64}, &sqlparser.Argument{Name: "_c3_2", Type: sqltypes.Int64}},
{&sqlparser.Argument{Name: "_id_3", Type: sqltypes.Int64}, &sqlparser.Argument{Name: "_c1_3", Type: sqltypes.Int64}, &sqlparser.Argument{Name: "_c2_3", Type: sqltypes.Int64}, &sqlparser.Argument{Name: "_c3_3", Type: sqltypes.Int64}},
},
nil,
)
Expand Down Expand Up @@ -1045,31 +1040,29 @@ func TestInsertShardedIgnoreOwned(t *testing.T) {
t.Fatal(err)
}
vc.ExpectLog(t, []string{
`Execute select from1, toc from prim where from1 in ::from1 from1: type:TUPLE values:{type:INT64 value:"1"} values:{type:INT64 value:"2"} values:{type:INT64 value:"3"} values:{type:INT64 value:"4"} false`,
`Execute insert ignore into lkp2(from1, from2, toc) values(:from1_0, :from2_0, :toc_0), (:from1_1, :from2_1, :toc_1), (:from1_2, :from2_2, :toc_2) ` +
`Execute select from1, toc from prim where from1 in ::from1 ` +
`from1: type:TUPLE values:{type:INT64 value:"1"} values:{type:INT64 value:"3"} values:{type:INT64 value:"4"} false`,
`Execute insert ignore into lkp2(from1, from2, toc) values` +
`(:from1_0, :from2_0, :toc_0), (:from1_1, :from2_1, :toc_1), (:from1_2, :from2_2, :toc_2) ` +
`from1_0: type:INT64 value:"5" from1_1: type:INT64 value:"7" from1_2: type:INT64 value:"8" ` +
`from2_0: type:INT64 value:"9" from2_1: type:INT64 value:"11" from2_2: type:INT64 value:"12" ` +
`toc_0: type:VARBINARY value:"\x00" toc_1: type:VARBINARY value:"\x00" toc_2: type:VARBINARY value:"\x00" ` +
`true`,
// row 2 is out because it didn't map to a ksid.
`toc_0: type:VARBINARY value:"\x00" toc_1: type:VARBINARY value:"\x00" toc_2: type:VARBINARY value:"\x00" true`,
`Execute select from1 from lkp2 where from1 = :from1 and toc = :toc from1: type:INT64 value:"5" toc: type:VARBINARY value:"\x00" false`,
`Execute select from1 from lkp2 where from1 = :from1 and toc = :toc from1: type:INT64 value:"7" toc: type:VARBINARY value:"\x00" false`,
`Execute select from1 from lkp2 where from1 = :from1 and toc = :toc from1: type:INT64 value:"8" toc: type:VARBINARY value:"\x00" false`,
`Execute insert ignore into lkp1(from, toc) values(:from_0, :toc_0), (:from_1, :toc_1) ` +
`from_0: type:INT64 value:"13" from_1: type:INT64 value:"16" ` +
`toc_0: type:VARBINARY value:"\x00" toc_1: type:VARBINARY value:"\x00" ` +
`true`,
// row 3 is out because it failed Verify. Only two verifications from lkp1.
`toc_0: type:VARBINARY value:"\x00" toc_1: type:VARBINARY value:"\x00" true`,
// row 2 is out because it failed Verify. Only two verifications from lkp1.
`Execute select from from lkp1 where from = :from and toc = :toc from: type:INT64 value:"13" toc: type:VARBINARY value:"\x00" false`,
`Execute select from from lkp1 where from = :from and toc = :toc from: type:INT64 value:"16" toc: type:VARBINARY value:"\x00" false`,
`ResolveDestinations sharded [value:"0" value:"3"] Destinations:DestinationKeyspaceID(00),DestinationKeyspaceID(00)`,
// Bind vars for rows 2 & 3 may be missing because they were not sent.
`ResolveDestinations sharded [value:"0" value:"2"] Destinations:DestinationKeyspaceID(00),DestinationKeyspaceID(00)`,
// Bind vars for rows 2 may be missing because they were not sent.
`ExecuteMultiShard ` +
`sharded.20-: prefix(:_id_0 /* INT64 */, :_c1_0 /* INT64 */, :_c2_0 /* INT64 */, :_c3_0 /* INT64 */) ` +
`{_c1_0: type:INT64 value:"5" _c2_0: type:INT64 value:"9" _c3_0: type:INT64 value:"13" _id_0: type:INT64 value:"1"} ` +
`sharded.-20: prefix(:_id_3 /* INT64 */, :_c1_3 /* INT64 */, :_c2_3 /* INT64 */, :_c3_3 /* INT64 */) ` +
`{_c1_3: type:INT64 value:"8" _c2_3: type:INT64 value:"12" _c3_3: type:INT64 value:"16" _id_3: type:INT64 value:"4"} ` +
`true false`,
`sharded.-20: prefix(:_id_2 /* INT64 */, :_c1_2 /* INT64 */, :_c2_2 /* INT64 */, :_c3_2 /* INT64 */) ` +
`{_c1_2: type:INT64 value:"8" _c2_2: type:INT64 value:"12" _c3_2: type:INT64 value:"16" _id_2: type:INT64 value:"4"} true false`,
})
}

Expand Down
Loading

0 comments on commit 3ce8f87

Please sign in to comment.