From 72414940cc99185305e048dfcdbcddc0b758fa38 Mon Sep 17 00:00:00 2001 From: Harshit Gangal Date: Fri, 11 Sep 2020 16:25:10 +0530 Subject: [PATCH 1/5] use lookup vindex to delete the record Signed-off-by: Harshit Gangal --- go/test/endtoend/vtgate/lookup_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go/test/endtoend/vtgate/lookup_test.go b/go/test/endtoend/vtgate/lookup_test.go index 29829410d38..47e9930d788 100644 --- a/go/test/endtoend/vtgate/lookup_test.go +++ b/go/test/endtoend/vtgate/lookup_test.go @@ -161,7 +161,7 @@ func TestConsistentLookup(t *testing.T) { if got, want := fmt.Sprintf("%v", qr.Rows), "[[INT64(5) VARBINARY(\"\\x16k@\\xb4J\\xbaK\\xd6\")]]"; got != want { t.Errorf("select:\n%v want\n%v", got, want) } - exec(t, conn, "delete from t1 where id1=1") + exec(t, conn, "delete from t1 where id2=5") } func TestDMLScatter(t *testing.T) { From be6b37fdf864a5a1c86e4c86039e8493c822bdd5 Mon Sep 17 00:00:00 2001 From: Harshit Gangal Date: Fri, 11 Sep 2020 16:25:51 +0530 Subject: [PATCH 2/5] use post shard session for vindex row lock for delete query Signed-off-by: Harshit Gangal --- go/vt/vtgate/engine/fake_vcursor_test.go | 4 ++++ go/vt/vtgate/engine/primitive.go | 2 ++ go/vt/vtgate/vcursor_impl.go | 8 ++++++++ go/vt/vtgate/vindexes/consistent_lookup.go | 2 +- go/vt/vtgate/vindexes/consistent_lookup_test.go | 4 ++++ go/vt/vtgate/vindexes/lookup_test.go | 4 ++++ go/vt/vtgate/vindexes/vindex.go | 1 + 7 files changed, 24 insertions(+), 1 deletion(-) diff --git a/go/vt/vtgate/engine/fake_vcursor_test.go b/go/vt/vtgate/engine/fake_vcursor_test.go index 04448731c4c..2f311a1f04e 100644 --- a/go/vt/vtgate/engine/fake_vcursor_test.go +++ b/go/vt/vtgate/engine/fake_vcursor_test.go @@ -53,6 +53,10 @@ type noopVCursor struct { ctx context.Context } +func (t noopVCursor) LookupRowLockShardSession() vtgatepb.CommitOrder { + panic("implement me") +} + func (t noopVCursor) InTransactionAndIsDML() bool { panic("implement me") } diff --git a/go/vt/vtgate/engine/primitive.go b/go/vt/vtgate/engine/primitive.go index 4e2e40e7ed7..19bc06c1084 100644 --- a/go/vt/vtgate/engine/primitive.go +++ b/go/vt/vtgate/engine/primitive.go @@ -88,6 +88,8 @@ type ( ExecuteLock(rs *srvtopo.ResolvedShard, query *querypb.BoundQuery) (*sqltypes.Result, error) InTransactionAndIsDML() bool + + LookupRowLockShardSession() vtgatepb.CommitOrder } //SessionActions gives primitives ability to interact with the session state diff --git a/go/vt/vtgate/vcursor_impl.go b/go/vt/vtgate/vcursor_impl.go index 09ea335ba7d..a06b82c79fb 100644 --- a/go/vt/vtgate/vcursor_impl.go +++ b/go/vt/vtgate/vcursor_impl.go @@ -332,6 +332,14 @@ func (vc *vcursorImpl) InTransactionAndIsDML() bool { return false } +func (vc *vcursorImpl) LookupRowLockShardSession() vtgatepb.CommitOrder { + switch vc.logStats.StmtType { + case "DELETE": + return vtgatepb.CommitOrder_POST + } + return vtgatepb.CommitOrder_PRE +} + func (vc *vcursorImpl) ExecuteLock(rs *srvtopo.ResolvedShard, query *querypb.BoundQuery) (*sqltypes.Result, error) { query.Sql = vc.marginComments.Leading + query.Sql + vc.marginComments.Trailing return vc.executor.ExecuteLock(vc.ctx, rs, query, vc.safeSession) diff --git a/go/vt/vtgate/vindexes/consistent_lookup.go b/go/vt/vtgate/vindexes/consistent_lookup.go index 1f58a9f3a7e..82c3889755c 100644 --- a/go/vt/vtgate/vindexes/consistent_lookup.go +++ b/go/vt/vtgate/vindexes/consistent_lookup.go @@ -164,7 +164,7 @@ func (lu *ConsistentLookupUnique) Map(vcursor VCursor, ids []sqltypes.Value) ([] return out, nil } - results, err := lu.lkp.Lookup(vcursor, ids, vtgatepb.CommitOrder_PRE) + results, err := lu.lkp.Lookup(vcursor, ids, vcursor.LookupRowLockShardSession()) if err != nil { return nil, err } diff --git a/go/vt/vtgate/vindexes/consistent_lookup_test.go b/go/vt/vtgate/vindexes/consistent_lookup_test.go index 0a77317b47f..b0ec78081b0 100644 --- a/go/vt/vtgate/vindexes/consistent_lookup_test.go +++ b/go/vt/vtgate/vindexes/consistent_lookup_test.go @@ -506,6 +506,10 @@ type loggingVCursor struct { log []string } +func (vc *loggingVCursor) LookupRowLockShardSession() vtgatepb.CommitOrder { + return vtgatepb.CommitOrder_PRE +} + func (vc *loggingVCursor) InTransactionAndIsDML() bool { return false } diff --git a/go/vt/vtgate/vindexes/lookup_test.go b/go/vt/vtgate/vindexes/lookup_test.go index ecdebe81544..febeb81f449 100644 --- a/go/vt/vtgate/vindexes/lookup_test.go +++ b/go/vt/vtgate/vindexes/lookup_test.go @@ -50,6 +50,10 @@ type vcursor struct { keys []sqltypes.Value } +func (vc *vcursor) LookupRowLockShardSession() vtgatepb.CommitOrder { + panic("implement me") +} + func (vc *vcursor) InTransactionAndIsDML() bool { return false } diff --git a/go/vt/vtgate/vindexes/vindex.go b/go/vt/vtgate/vindexes/vindex.go index b6beec73986..06141b8a52c 100644 --- a/go/vt/vtgate/vindexes/vindex.go +++ b/go/vt/vtgate/vindexes/vindex.go @@ -38,6 +38,7 @@ type VCursor interface { Execute(method string, query string, bindvars map[string]*querypb.BindVariable, rollbackOnError bool, co vtgatepb.CommitOrder) (*sqltypes.Result, error) ExecuteKeyspaceID(keyspace string, ksid []byte, query string, bindVars map[string]*querypb.BindVariable, rollbackOnError, autocommit bool) (*sqltypes.Result, error) InTransactionAndIsDML() bool + LookupRowLockShardSession() vtgatepb.CommitOrder } // Vindex defines the interface required to register a vindex. From cffe36e2884950cfe4f3bc1950d9bf7c7014d2e7 Mon Sep 17 00:00:00 2001 From: Harshit Gangal Date: Fri, 11 Sep 2020 20:28:54 +0530 Subject: [PATCH 3/5] addded unit test Signed-off-by: Harshit Gangal --- go/vt/vtgate/executor_dml_test.go | 34 +++++++++++++++++++++++ go/vt/vtgate/executor_framework_test.go | 36 +++++++++++++++++++++++-- 2 files changed, 68 insertions(+), 2 deletions(-) diff --git a/go/vt/vtgate/executor_dml_test.go b/go/vt/vtgate/executor_dml_test.go index 3c213036603..48452a1aedc 100644 --- a/go/vt/vtgate/executor_dml_test.go +++ b/go/vt/vtgate/executor_dml_test.go @@ -21,6 +21,8 @@ import ( "strings" "testing" + "vitess.io/vitess/go/vt/discovery" + "github.com/stretchr/testify/assert" "vitess.io/vitess/go/test/utils" @@ -1709,3 +1711,35 @@ func TestUpdateLastInsertID(t *testing.T) { require.Equal(t, wantQueries, sbc1.Queries) } + +func TestDeleteLookupOwnedEqual(t *testing.T) { + executor, sbc1, sbc2, _ := createLegacyExecutorEnv() + + executor.scatterConn.gateway.(*DiscoveryGateway).hc.(*discovery.FakeLegacyHealthCheck).GetAllTablets() + sbc1.SetResults([]*sqltypes.Result{ + sqltypes.MakeTestResult(sqltypes.MakeTestFields("uniq_col|keyspace_id", "int64|varbinary"), "1|N±\u0090ɢú\u0016\u009C"), + }) + _, err := executorExec(executor, "delete from t1 where unq_col = 1", nil) + require.NoError(t, err) + tupleBindVar, _ := sqltypes.BuildBindVariable([]int64{1}) + sbc1wantQueries := []*querypb.BoundQuery{{ + Sql: "select unq_col, keyspace_id from t1_lkp_idx where unq_col in ::__vals for update", + BindVariables: map[string]*querypb.BindVariable{ + "__vals": tupleBindVar, + "unq_col": tupleBindVar, + }, + }} + sbc2wantQueries := []*querypb.BoundQuery{{ + Sql: "select id, unq_col from t1 where unq_col = 1 for update", + BindVariables: map[string]*querypb.BindVariable{}, + }, { + Sql: "delete from t1 where unq_col = 1", + BindVariables: map[string]*querypb.BindVariable{}, + }} + if !reflect.DeepEqual(sbc1.Queries, sbc1wantQueries) { + t.Errorf("sbc1.Queries:\n%+v, want\n%+v\n", sbc1.Queries, sbc1wantQueries) + } + if !reflect.DeepEqual(sbc2.Queries, sbc2wantQueries) { + t.Errorf("sbc2.Queries:\n%+v, want\n%+v\n", sbc2.Queries, sbc2wantQueries) + } +} diff --git a/go/vt/vtgate/executor_framework_test.go b/go/vt/vtgate/executor_framework_test.go index 78116fdcee0..d8a9df232a9 100644 --- a/go/vt/vtgate/executor_framework_test.go +++ b/go/vt/vtgate/executor_framework_test.go @@ -95,7 +95,19 @@ var executorVSchema = ` }, "krcol_vdx": { "type": "keyrange_lookuper" - } + }, + "xxhash": { + "type": "xxhash" + }, + "t1_lkp_vdx": { + "type": "consistent_lookup_unique", + "params": { + "table": "t1_lkp_idx", + "from": "unq_col", + "to": "keyspace_id" + }, + "owner": "t1" + } }, "tables": { "user": { @@ -231,6 +243,26 @@ var executorVSchema = ` "name": "keyspace_id" } ] + }, + "t1": { + "column_vindexes": [ + { + "column": "id", + "name": "hash_index" + }, + { + "column": "unq_col", + "name": "t1_lkp_vdx" + } + ] + }, + "t1_lkp_idx": { + "column_vindexes": [ + { + "column": "unq_col", + "name": "hash_index" + } + ] } } } @@ -349,7 +381,7 @@ func createLegacyExecutorEnv() (executor *Executor, sbc1, sbc2, sbclookup *sandb sbc2 = hc.AddTestTablet(cell, "40-60", 1, "TestExecutor", "40-60", topodatapb.TabletType_MASTER, true, 1, nil) // Create these connections so scatter queries don't fail. _ = hc.AddTestTablet(cell, "20-40", 1, "TestExecutor", "20-40", topodatapb.TabletType_MASTER, true, 1, nil) - _ = hc.AddTestTablet(cell, "60-60", 1, "TestExecutor", "60-80", topodatapb.TabletType_MASTER, true, 1, nil) + _ = hc.AddTestTablet(cell, "60-80", 1, "TestExecutor", "60-80", topodatapb.TabletType_MASTER, true, 1, nil) _ = hc.AddTestTablet(cell, "80-a0", 1, "TestExecutor", "80-a0", topodatapb.TabletType_MASTER, true, 1, nil) _ = hc.AddTestTablet(cell, "a0-c0", 1, "TestExecutor", "a0-c0", topodatapb.TabletType_MASTER, true, 1, nil) _ = hc.AddTestTablet(cell, "c0-e0", 1, "TestExecutor", "c0-e0", topodatapb.TabletType_MASTER, true, 1, nil) From 591f2963533b925ea78950f85c9cf5aad038653e Mon Sep 17 00:00:00 2001 From: Harshit Gangal Date: Fri, 11 Sep 2020 20:32:58 +0530 Subject: [PATCH 4/5] fix show vindexes test Signed-off-by: Harshit Gangal --- go/vt/vtgate/executor_framework_test.go | 3 --- go/vt/vtgate/executor_test.go | 3 ++- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/go/vt/vtgate/executor_framework_test.go b/go/vt/vtgate/executor_framework_test.go index d8a9df232a9..e8bcde7c4cf 100644 --- a/go/vt/vtgate/executor_framework_test.go +++ b/go/vt/vtgate/executor_framework_test.go @@ -96,9 +96,6 @@ var executorVSchema = ` "krcol_vdx": { "type": "keyrange_lookuper" }, - "xxhash": { - "type": "xxhash" - }, "t1_lkp_vdx": { "type": "consistent_lookup_unique", "params": { diff --git a/go/vt/vtgate/executor_test.go b/go/vt/vtgate/executor_test.go index f11c0a7ce64..af00fdbd78d 100644 --- a/go/vt/vtgate/executor_test.go +++ b/go/vt/vtgate/executor_test.go @@ -809,8 +809,9 @@ func TestExecutorShow(t *testing.T) { buildVarCharRow("TestExecutor", "music_user_map", "lookup_hash_unique", "from=music_id; table=music_user_map; to=user_id", "music"), buildVarCharRow("TestExecutor", "name_lastname_keyspace_id_map", "lookup", "from=name,lastname; table=name_lastname_keyspace_id_map; to=keyspace_id", "user2"), buildVarCharRow("TestExecutor", "name_user_map", "lookup_hash", "from=name; table=name_user_map; to=user_id", "user"), + buildVarCharRow("TestExecutor", "t1_lkp_vdx", "consistent_lookup_unique", "from=unq_col; table=t1_lkp_idx; to=keyspace_id", "t1"), }, - RowsAffected: 10, + RowsAffected: 11, } if !reflect.DeepEqual(qr, wantqr) { t.Errorf("show vschema vindexes:\n%+v, want\n%+v", qr, wantqr) From 9b575ae0dd90cd17247981bb4cd8109428e14d04 Mon Sep 17 00:00:00 2001 From: Harshit Gangal Date: Fri, 11 Sep 2020 20:35:04 +0530 Subject: [PATCH 5/5] use utils library for assert Signed-off-by: Harshit Gangal --- go/vt/vtgate/executor_dml_test.go | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/go/vt/vtgate/executor_dml_test.go b/go/vt/vtgate/executor_dml_test.go index 48452a1aedc..b5ea0d29640 100644 --- a/go/vt/vtgate/executor_dml_test.go +++ b/go/vt/vtgate/executor_dml_test.go @@ -21,8 +21,6 @@ import ( "strings" "testing" - "vitess.io/vitess/go/vt/discovery" - "github.com/stretchr/testify/assert" "vitess.io/vitess/go/test/utils" @@ -1715,7 +1713,6 @@ func TestUpdateLastInsertID(t *testing.T) { func TestDeleteLookupOwnedEqual(t *testing.T) { executor, sbc1, sbc2, _ := createLegacyExecutorEnv() - executor.scatterConn.gateway.(*DiscoveryGateway).hc.(*discovery.FakeLegacyHealthCheck).GetAllTablets() sbc1.SetResults([]*sqltypes.Result{ sqltypes.MakeTestResult(sqltypes.MakeTestFields("uniq_col|keyspace_id", "int64|varbinary"), "1|N±\u0090ɢú\u0016\u009C"), }) @@ -1736,10 +1733,6 @@ func TestDeleteLookupOwnedEqual(t *testing.T) { Sql: "delete from t1 where unq_col = 1", BindVariables: map[string]*querypb.BindVariable{}, }} - if !reflect.DeepEqual(sbc1.Queries, sbc1wantQueries) { - t.Errorf("sbc1.Queries:\n%+v, want\n%+v\n", sbc1.Queries, sbc1wantQueries) - } - if !reflect.DeepEqual(sbc2.Queries, sbc2wantQueries) { - t.Errorf("sbc2.Queries:\n%+v, want\n%+v\n", sbc2.Queries, sbc2wantQueries) - } + utils.MustMatch(t, sbc1.Queries, sbc1wantQueries, "") + utils.MustMatch(t, sbc2.Queries, sbc2wantQueries, "") }