plan, util/admin: fix admin check table error when a column of index is virtual generated #6817

crazycs520 · 2018-06-12T05:02:07Z

What have you changed? (mandatory)

admin check table will check the Integrity and consistency of index and record.

But how to excute admin check table

Before

read value of index key and record key from tikv, decode , compare the value of the Index and the corresponding column.

Problem

when a column of index is virtual generated , decode the record value will get NULL of the virtual generated column , because the virtual generated column have not stored , then compare will be not consistent.
How i fix it

after decode the record value, before compare , we have to calculate the value of virtual generated column.

Test SQL:

create table test ( b json , c int as (JSON_EXTRACT(b,'$.d')) , index idxc(c));
insert into test set b='{"d": 100}';
admin check table test;

What are the type of the changes (mandatory)?

Bug fix

How has this PR been tested (mandatory)?

Unit tests.

CLAassistant · 2018-06-12T05:02:14Z

All committers have signed the CLA.

shenli · 2018-06-12T05:58:41Z

@crazycs520 Please sign the CLA.

shenli · 2018-06-12T05:59:33Z

model/model.go

@@ -17,6 +17,7 @@ import (
 	"strings"
 	"time"

+	"fmt"


Move the fmt into the upper group.

shenli · 2018-06-12T05:59:45Z

model/model.go

@@ -493,3 +494,8 @@ func collationToProto(c string) int32 {
 	// For the data created when we didn't enforce utf8_bin collation in create table.
 	return int32(mysql.DefaultCollationID)
 }
+
+// GetTableColumnID get a ID of a column with table ID


s/get/gets/

shenli · 2018-06-12T06:01:14Z

plan/planbuilder.go

+	p := &CheckTable{Tables: as.Tables}
+	p.GenExprs = make(map[string]expression.Expression, 0)
+
+	mockTablePlan := LogicalTableDual{}.init(b.ctx)


Why use LogicalTableDual?

I mock from buildInsert()
LogicalTableDual is use to b.rewrite(column.GeneratedExpr, mockTablePlan, nil, true)
I wanna directly use CheckTable instead of LogicalTableDual , but it seem not fit

So what's the problem with using CheckTable?

CheckTable don't implement LogicalPlan , expr, _, err := b.rewrite need a LogicalPlan

zimulala · 2018-06-12T08:59:48Z

model/model.go

@@ -493,3 +494,8 @@ func collationToProto(c string) int32 {
 	// For the data created when we didn't enforce utf8_bin collation in create table.
 	return int32(mysql.DefaultCollationID)
 }
+
+// GetTableColumnID gets a ID of a column with table ID
+func GetTableColumnID(tableInfo *TableInfo, col *ColumnInfo) string {


If this function is only used in buildAdminCheckTable, you can put it to planbuilder.go and update its name as getTableColumnID.

this function also call on util/admin/admin.go

zz-jason · 2018-06-13T04:53:55Z

util/admin/admin.go

+			rowMap[col.ID] = val
+		}
+	}
+	if err != nil {


this check can be removed.

alivxxx · 2018-06-13T06:16:02Z

util/admin/admin.go

@@ -490,12 +491,30 @@ func rowWithCols(sessCtx sessionctx.Context, txn kv.Retriever, t table.Table, h
 			}
 			continue
 		}
+		if col.IsGenerated() && col.GeneratedStored == false {
+			genFlag = true
+			break


Why break here? If there are handle column after this column, the check would fail because it did set the handle as line 486 do.

Add some comments here. Why break?

I have remove break.

winkyao · 2018-06-14T06:25:19Z

@crazycs520 Please address comments.

winkyao · 2018-06-14T06:51:54Z

util/admin/admin.go

@@ -244,13 +245,13 @@ func ScanIndexData(sc *stmtctx.StatementContext, txn kv.Transaction, kvIndex tab
 // CompareIndexData compares index data one by one.
 // It returns nil if the data from the index is equal to the data from the table columns,
 // otherwise it returns an error with a different set of records.
-func CompareIndexData(sessCtx sessionctx.Context, txn kv.Transaction, t table.Table, idx table.Index) error {
-	err := checkIndexAndRecord(sessCtx, txn, t, idx)
+func CompareIndexData(sessCtx sessionctx.Context, txn kv.Transaction, t table.Table, idx table.Index, genExprs map[string]expression.Expression) error {


Add some comment to genExprs param.

winkyao · 2018-06-14T08:21:37Z

plan/planbuilder.go

+		schema := expression.TableInfo2SchemaWithDBName(tbl.Schema, tableInfo)
+		table, ok := b.is.TableByID(tableInfo.ID)
+		if !ok {
+			b.err = errors.Errorf("Can't get table %s.", tableInfo.Name.O)


modify to b.err = infoschema.ErrTableNotExists.GenByArgs(....)

winkyao · 2018-06-14T08:37:55Z

plan/planbuilder.go

@@ -494,6 +493,50 @@ func (b *planBuilder) buildAdmin(as *ast.AdminStmt) Plan {
 	return ret
 }

+func (b *planBuilder) buildAdminCheckTable(as *ast.AdminStmt) Plan {
+	p := &CheckTable{Tables: as.Tables}
+	p.GenExprs = make(map[string]expression.Expression, 0)


make(map[string]expression.Expression) is ok.

winkyao · 2018-06-14T08:53:17Z

util/admin/admin.go

@@ -539,10 +558,21 @@ func iterRecords(sessCtx sessionctx.Context, retriever kv.Retriever, t table.Tab

 	log.Debugf("startKey:%q, key:%q, value:%q", startKey, it.Key(), it.Value())

+	genFlag := false


s/genFlag/genColFlag/g

winkyao · 2018-06-14T09:06:01Z

util/admin/admin.go

 		colTps[col.ID] = &col.FieldType
 	}
-	row, err := tablecodec.DecodeRow(value, colTps, sessCtx.GetSessionVars().GetTimeZone())
+	if genFlag {


Add some comments.

…is virtual generated

crazycs520 · 2018-06-28T08:59:15Z

/run-all-tests

crazycs520 · 2018-07-19T09:37:33Z

@winkyao @zimulala @zz-jason PTAL

zz-jason · 2018-07-24T05:59:25Z

executor/executor.go

@@ -349,6 +349,8 @@ type CheckTableExec struct {
 	tables []*ast.TableName
 	done   bool
 	is     infoschema.InfoSchema
+
+	GenExprs map[string]expression.Expression


I think we don't need to export this variable.

zz-jason · 2018-07-24T06:00:23Z

plan/planbuilder.go

@@ -495,6 +494,50 @@ func (b *planBuilder) buildAdmin(as *ast.AdminStmt) Plan {
 	return ret
 }

+func (b *planBuilder) buildAdminCheckTable(as *ast.AdminStmt) Plan {


Change the type of return value from Plan to *CheckTable would be better.

zz-jason

LGTM

crazycs520 · 2018-08-01T13:43:08Z

@shenli @winkyao @winkyao PTAL

winoros · 2018-08-01T17:17:11Z

plan/common_plans.go

@@ -56,6 +56,8 @@ type CheckTable struct {
 	baseSchemaProducer

 	Tables []*ast.TableName
+
+	GenExprs map[string]expression.Expression


What about map[int]expression.Expression where the map key is ColumnInfo.ID.

There maybe have multiple tables, only use ColumnInfo.ID may leads to conflict in same ColumnInfo.ID but in different table.

winoros

Lgtm

winkyao

LGTM

ciscoxll

reset LGTM

ciscoxll · 2018-08-27T17:32:03Z

executor/admin_test.go

@@ -462,6 +462,11 @@ func (s *testSuite) TestAdminCheckTable(c *C) {
 	}
 	tk.MustExec(`admin check table test;`)

+	//test index in virtual generated column
+	tk.MustExec(`drop table if exists test`)


s /test/Test

ciscoxll · 2018-08-27T17:36:03Z

plan/planbuilder.go

+		schema := expression.TableInfo2SchemaWithDBName(b.ctx, tbl.Schema, tableInfo)
+		table, ok := b.is.TableByID(tableInfo.ID)
+		if !ok {
+			return nil, infoschema.ErrTableNotExists.GenByArgs(tbl.DBInfo.Name.O, tableInfo.Name.O)


s/ tbl.DBInfo.Name.O/ tbl.DBInfo.Name.L

I think the original is better. here is for log, not for compare.

ciscoxll · 2018-08-27T17:38:48Z

util/admin/admin.go

@@ -558,12 +562,34 @@ func rowWithCols(sessCtx sessionctx.Context, txn kv.Retriever, t table.Table, h
 			}
 			continue
 		}
+		// if have virtual generate column , decode all columns.


s / if /If

crazycs520 · 2018-08-28T01:39:22Z

/run-all-tests

crazycs520 · 2018-08-28T03:04:20Z

/run-unit-test

crazycs520 requested review from winkyao and zimulala June 12, 2018 05:02

shenli reviewed Jun 12, 2018

View reviewed changes

crazycs520 force-pushed the cs branch from 362c8ea to 854d76b Compare June 12, 2018 07:16

zimulala reviewed Jun 12, 2018

View reviewed changes

zz-jason reviewed Jun 13, 2018

View reviewed changes

util/admin/admin.go Outdated

rowMap[col.ID] = val

}

}

if err != nil {

Copy link

Member

zz-jason Jun 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this check can be removed.

alivxxx reviewed Jun 13, 2018

View reviewed changes

winkyao reviewed Jun 14, 2018

View reviewed changes

crazycs520 added 2 commits June 20, 2018 19:13

plan, util/admin: fix admin check table error when a column of index …

7ff630b

…is virtual generated

plan, util/admin: add comment and remove redundant code

c8e5982

crazycs520 force-pushed the cs branch from 854d76b to c8e5982 Compare June 20, 2018 11:15

util/admin: rename variable

0a5164f

crazycs520 added the status/WIP label Jul 12, 2018

crazycs520 added 2 commits July 19, 2018 16:48

Merge branch 'master' of https://github.com/pingcap/tidb into cs

bd3786f

add comment

c0fe0db

crazycs520 added component/DDL-need-LGT3 and removed status/WIP labels Jul 19, 2018

zz-jason reviewed Jul 24, 2018

View reviewed changes

rename variable

589e375

zz-jason previously approved these changes Jul 27, 2018

View reviewed changes

Merge branch 'master' into cs

53b572e

crazycs520 dismissed zz-jason’s stale review via 53b572e August 1, 2018 09:21

update after merge master

01e4e4a

winoros reviewed Aug 1, 2018

View reviewed changes

crazycs520 added 2 commits August 21, 2018 15:15

Merge branch 'master' of https://github.com/pingcap/tidb into HEAD

47ec2bc

Merge branch 'master' of https://github.com/pingcap/tidb into cs

79c71b1

crazycs520 added the status/LGT1 Indicates that a PR has LGTM 1. label Aug 27, 2018

winoros reviewed Aug 27, 2018

View reviewed changes

winkyao previously approved these changes Aug 27, 2018

View reviewed changes

ciscoxll reviewed Aug 27, 2018

View reviewed changes

crazycs520 added 2 commits August 28, 2018 09:32

address comment

654ca7c

Merge branch 'master' of https://github.com/pingcap/tidb into cs

dfbce56

crazycs520 dismissed winkyao’s stale review via dfbce56 August 28, 2018 01:36

Merge branch 'master' into cs

2ae4bab

winkyao approved these changes Aug 28, 2018

View reviewed changes

crazycs520 merged commit bc97e2d into pingcap:master Aug 28, 2018

you06 added the sig/sql-infra SIG: SQL Infra label Mar 4, 2020

		@@ -539,10 +558,21 @@ func iterRecords(sessCtx sessionctx.Context, retriever kv.Retriever, t table.Tab

		log.Debugf("startKey:%q, key:%q, value:%q", startKey, it.Key(), it.Value())

		genFlag := false

plan, util/admin: fix admin check table error when a column of index is virtual generated #6817

plan, util/admin: fix admin check table error when a column of index is virtual generated #6817

Conversation

crazycs520 commented Jun 12, 2018 • edited

What have you changed? (mandatory)

What are the type of the changes (mandatory)?

How has this PR been tested (mandatory)?

CLAassistant commented Jun 12, 2018 • edited

shenli commented Jun 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crazycs520 Jun 12, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

winkyao commented Jun 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crazycs520 commented Jun 28, 2018

crazycs520 commented Jul 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zz-jason left a comment

Choose a reason for hiding this comment

crazycs520 commented Aug 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

winoros left a comment

Choose a reason for hiding this comment

winkyao left a comment

Choose a reason for hiding this comment

ciscoxll left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ciscoxll Aug 27, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crazycs520 commented Aug 28, 2018

crazycs520 commented Aug 28, 2018

crazycs520 commented Jun 12, 2018 •

edited

CLAassistant commented Jun 12, 2018 •

edited

crazycs520 Jun 12, 2018 •

edited

ciscoxll left a comment •

edited

ciscoxll Aug 27, 2018 •

edited