From 027d616710799230e67901f8c3716080b8587f83 Mon Sep 17 00:00:00 2001 From: Florent Poinsard Date: Mon, 30 Aug 2021 11:14:31 +0200 Subject: [PATCH 1/6] Addition of an end-to-end test to reproduce the panic of #8694 Signed-off-by: Florent Poinsard --- go/vt/vtgate/endtoend/misc_test.go | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/go/vt/vtgate/endtoend/misc_test.go b/go/vt/vtgate/endtoend/misc_test.go index 9cc28986327..1c8ecf89a0e 100644 --- a/go/vt/vtgate/endtoend/misc_test.go +++ b/go/vt/vtgate/endtoend/misc_test.go @@ -69,3 +69,14 @@ func TestCreateAndDropDatabase(t *testing.T) { }) } } + +func TestRenameFieldsOnOLAP(t *testing.T) { + // note that this is testing vttest and not vtgate + conn, err := mysql.Connect(ctx, &vtParams) + require.NoError(t, err) + defer conn.Close() + + _ = exec(t, conn, "set workload = olap") + qr := exec(t, conn, "show tables") + assert.Equal(t, `[[VARCHAR("aggr_test")] [VARCHAR("t1")] [VARCHAR("t1_id2_idx")] [VARCHAR("t1_last_insert_id")] [VARCHAR("t1_row_count")] [VARCHAR("t1_sharded")] [VARCHAR("t2")] [VARCHAR("t2_id4_idx")] [VARCHAR("vstream_test")]]`, fmt.Sprintf("%v", qr.Rows)) +} From 6e97c4503f17b0c4361fdf28a8e4a127f118ec71 Mon Sep 17 00:00:00 2001 From: Florent Poinsard Date: Mon, 30 Aug 2021 11:15:30 +0200 Subject: [PATCH 2/6] Avoiding panic in RenameFields.renameFields method Signed-off-by: Florent Poinsard --- go/vt/vtgate/engine/rename_fields.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/go/vt/vtgate/engine/rename_fields.go b/go/vt/vtgate/engine/rename_fields.go index 9983c5036e1..b45ea04071c 100644 --- a/go/vt/vtgate/engine/rename_fields.go +++ b/go/vt/vtgate/engine/rename_fields.go @@ -73,6 +73,9 @@ func (r *RenameFields) Execute(vcursor VCursor, bindVars map[string]*querypb.Bin } func (r *RenameFields) renameFields(qr *sqltypes.Result) { + if len(r.Indices) != len(qr.Fields) { + return + } for ind, index := range r.Indices { colName := r.Cols[ind] qr.Fields[index].Name = colName From 93a5e933428f3bc49f7603193b10c82d87d80789 Mon Sep 17 00:00:00 2001 From: Florent Poinsard Date: Tue, 31 Aug 2021 11:14:07 +0200 Subject: [PATCH 3/6] Improved 8694 E2E test and for loop in renameFields Signed-off-by: Florent Poinsard --- go/test/endtoend/vtgate/misc_test.go | 19 +++++++++++++++++++ go/vt/vtgate/endtoend/misc_test.go | 11 ----------- go/vt/vtgate/engine/rename_fields.go | 9 +++------ 3 files changed, 22 insertions(+), 17 deletions(-) diff --git a/go/test/endtoend/vtgate/misc_test.go b/go/test/endtoend/vtgate/misc_test.go index 7bd41b5c9ac..7acf2aa8f3c 100644 --- a/go/test/endtoend/vtgate/misc_test.go +++ b/go/test/endtoend/vtgate/misc_test.go @@ -704,6 +704,25 @@ ts12 TIMESTAMP DEFAULT LOCALTIME() exec(t, conn, "drop table function_default") } +func TestRenameFieldsOnOLAP(t *testing.T) { + defer cluster.PanicHandler(t) + ctx := context.Background() + conn, err := mysql.Connect(ctx, &vtParams) + require.NoError(t, err) + defer conn.Close() + + _ = exec(t, conn, "set workload = olap") + defer func() { + exec(t, conn, "set workload = oltp") + }() + + qr := exec(t, conn, "show tables") + assert.Equal(t, `[[VARCHAR("aggr_test")] [VARCHAR("t1")] [VARCHAR("t1_id2_idx")] [VARCHAR("t2")] [VARCHAR("t2_id4_idx")] [VARCHAR("t3")] [VARCHAR("t3_id7_idx")] [VARCHAR("t4")] [VARCHAR("t4_id2_idx")] [VARCHAR("t5_null_vindex")] [VARCHAR("t6")] [VARCHAR("t6_id2_idx")] [VARCHAR("t7_fk")] [VARCHAR("t7_xxhash")] [VARCHAR("t7_xxhash_idx")] [VARCHAR("t8")] [VARCHAR("vstream_test")]]`, fmt.Sprintf("%v", qr.Rows)) + _ = exec(t, conn, "use mysql") + qr = exec(t, conn, "select @@workload") + assert.Equal(t, `[[VARBINARY("OLAP")]]`, fmt.Sprintf("%v", qr.Rows)) +} + func assertMatches(t *testing.T, conn *mysql.Conn, query, expected string) { t.Helper() qr := exec(t, conn, query) diff --git a/go/vt/vtgate/endtoend/misc_test.go b/go/vt/vtgate/endtoend/misc_test.go index 1c8ecf89a0e..9cc28986327 100644 --- a/go/vt/vtgate/endtoend/misc_test.go +++ b/go/vt/vtgate/endtoend/misc_test.go @@ -69,14 +69,3 @@ func TestCreateAndDropDatabase(t *testing.T) { }) } } - -func TestRenameFieldsOnOLAP(t *testing.T) { - // note that this is testing vttest and not vtgate - conn, err := mysql.Connect(ctx, &vtParams) - require.NoError(t, err) - defer conn.Close() - - _ = exec(t, conn, "set workload = olap") - qr := exec(t, conn, "show tables") - assert.Equal(t, `[[VARCHAR("aggr_test")] [VARCHAR("t1")] [VARCHAR("t1_id2_idx")] [VARCHAR("t1_last_insert_id")] [VARCHAR("t1_row_count")] [VARCHAR("t1_sharded")] [VARCHAR("t2")] [VARCHAR("t2_id4_idx")] [VARCHAR("vstream_test")]]`, fmt.Sprintf("%v", qr.Rows)) -} diff --git a/go/vt/vtgate/engine/rename_fields.go b/go/vt/vtgate/engine/rename_fields.go index b45ea04071c..7ddbabe3834 100644 --- a/go/vt/vtgate/engine/rename_fields.go +++ b/go/vt/vtgate/engine/rename_fields.go @@ -73,12 +73,9 @@ func (r *RenameFields) Execute(vcursor VCursor, bindVars map[string]*querypb.Bin } func (r *RenameFields) renameFields(qr *sqltypes.Result) { - if len(r.Indices) != len(qr.Fields) { - return - } - for ind, index := range r.Indices { - colName := r.Cols[ind] - qr.Fields[index].Name = colName + for i := 0; i < len(r.Indices) && i < len(qr.Fields); i++ { + colName := r.Cols[r.Indices[i]] + qr.Fields[i].Name = colName } } From 5e5e8c02baab554cdb5ee34be5d452c0f0aa326d Mon Sep 17 00:00:00 2001 From: Florent Poinsard Date: Wed, 1 Sep 2021 08:13:27 +0200 Subject: [PATCH 4/6] Fixing renameFields for loop logic Signed-off-by: Florent Poinsard --- go/vt/vtgate/engine/rename_fields.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/go/vt/vtgate/engine/rename_fields.go b/go/vt/vtgate/engine/rename_fields.go index 7ddbabe3834..d3e3a16c95a 100644 --- a/go/vt/vtgate/engine/rename_fields.go +++ b/go/vt/vtgate/engine/rename_fields.go @@ -73,9 +73,12 @@ func (r *RenameFields) Execute(vcursor VCursor, bindVars map[string]*querypb.Bin } func (r *RenameFields) renameFields(qr *sqltypes.Result) { - for i := 0; i < len(r.Indices) && i < len(qr.Fields); i++ { - colName := r.Cols[r.Indices[i]] - qr.Fields[i].Name = colName + for ind, index := range r.Indices { + if ind == len(qr.Fields) { + break + } + colName := r.Cols[ind] + qr.Fields[index].Name = colName } } From 59207f422cc181c8af6e4e1f888eae15e29dd11d Mon Sep 17 00:00:00 2001 From: Florent Poinsard Date: Wed, 1 Sep 2021 12:16:27 +0200 Subject: [PATCH 5/6] Fixed loop logic erro Signed-off-by: Florent Poinsard --- go/vt/vtgate/engine/rename_fields.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/go/vt/vtgate/engine/rename_fields.go b/go/vt/vtgate/engine/rename_fields.go index d3e3a16c95a..bb14a3145a1 100644 --- a/go/vt/vtgate/engine/rename_fields.go +++ b/go/vt/vtgate/engine/rename_fields.go @@ -74,8 +74,8 @@ func (r *RenameFields) Execute(vcursor VCursor, bindVars map[string]*querypb.Bin func (r *RenameFields) renameFields(qr *sqltypes.Result) { for ind, index := range r.Indices { - if ind == len(qr.Fields) { - break + if index >= len(qr.Fields) { + continue } colName := r.Cols[ind] qr.Fields[index].Name = colName From bcd3534b7bcc78cbe578e2c9586dc142384b6f23 Mon Sep 17 00:00:00 2001 From: Harshit Gangal Date: Thu, 2 Sep 2021 11:39:19 +0530 Subject: [PATCH 6/6] added a fields len check in rename_fields engine to avoid renamefields call Signed-off-by: Harshit Gangal --- go/test/endtoend/vtgate/misc_test.go | 3 ++- go/vt/vtgate/engine/rename_fields.go | 6 +++++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/go/test/endtoend/vtgate/misc_test.go b/go/test/endtoend/vtgate/misc_test.go index 7acf2aa8f3c..4e813c0a715 100644 --- a/go/test/endtoend/vtgate/misc_test.go +++ b/go/test/endtoend/vtgate/misc_test.go @@ -717,7 +717,8 @@ func TestRenameFieldsOnOLAP(t *testing.T) { }() qr := exec(t, conn, "show tables") - assert.Equal(t, `[[VARCHAR("aggr_test")] [VARCHAR("t1")] [VARCHAR("t1_id2_idx")] [VARCHAR("t2")] [VARCHAR("t2_id4_idx")] [VARCHAR("t3")] [VARCHAR("t3_id7_idx")] [VARCHAR("t4")] [VARCHAR("t4_id2_idx")] [VARCHAR("t5_null_vindex")] [VARCHAR("t6")] [VARCHAR("t6_id2_idx")] [VARCHAR("t7_fk")] [VARCHAR("t7_xxhash")] [VARCHAR("t7_xxhash_idx")] [VARCHAR("t8")] [VARCHAR("vstream_test")]]`, fmt.Sprintf("%v", qr.Rows)) + require.Equal(t, 1, len(qr.Fields)) + assert.Equal(t, `Tables_in_ks`, fmt.Sprintf("%v", qr.Fields[0].Name)) _ = exec(t, conn, "use mysql") qr = exec(t, conn, "select @@workload") assert.Equal(t, `[[VARBINARY("OLAP")]]`, fmt.Sprintf("%v", qr.Rows)) diff --git a/go/vt/vtgate/engine/rename_fields.go b/go/vt/vtgate/engine/rename_fields.go index bb14a3145a1..68b84f5155f 100644 --- a/go/vt/vtgate/engine/rename_fields.go +++ b/go/vt/vtgate/engine/rename_fields.go @@ -87,7 +87,11 @@ func (r *RenameFields) StreamExecute(vcursor VCursor, bindVars map[string]*query if wantfields { innerCallback := callback callback = func(result *sqltypes.Result) error { - r.renameFields(result) + // Only the first callback will contain the fields. + // This check is to avoid going over the RenameFields indices when no fields are present in the result set. + if len(result.Fields) != 0 { + r.renameFields(result) + } return innerCallback(result) } }