*: fix bad column offsets. #3754

hicqu · 2017-07-14T07:56:56Z

Sort Table.Cols() and Table.WritableCols() by model.ColumnInfo.Offset, so we can use the offset in any schema states correctly.

And, fix some other bugs.

shenli · 2017-07-14T14:10:28Z

@hicqu Please add a few description.

shenli · 2017-07-17T09:53:17Z

This PR needs three LGTMs.

tiancaiamao

Please add test for bug fix.

tiancaiamao · 2017-07-17T09:42:05Z

executor/write.go

+	var sc = ctx.GetSessionVars().StmtCtx
+	var changed = false
+	var handleChanged = false
+	var onUpdate = make([]bool, len(touched))


Just declare it var onUpdate []bool is enough, avoid the allocation here.

tiancaiamao · 2017-07-17T09:46:35Z

executor/write.go

+	var onUpdate = make([]bool, len(touched))
+
+	// We can iterate on public columns not writable columns,
+	// because all writable columns are after public columns.


"because all writable columns are after public columns"
I think public columns are after writable columns ...

they are sorted by their Offset, and write-only columns' Offset must be larger than public columns.

tiancaiamao · 2017-07-17T10:03:44Z

executor/write.go

-	}
+		if cmp != 0 {
+			changed = true
+			touched[i] = true


Maybe you misunderstand what touched means.

touched means the update statement changes the record really. is that correct?

tiancaiamao · 2017-07-17T10:31:47Z

plan/logical_plan_builder.go

@@ -1090,9 +1090,7 @@ func (b *planBuilder) buildDataSource(tn *ast.TableName) LogicalPlan {
 	} else {
 		statisticTable = handle.GetTableStats(tn.TableInfo.ID)
 	}
-	if b.err != nil {


why remove this?

because it's a redundant error check.

tiancaiamao · 2017-07-17T10:46:19Z

table/tables/tables.go

+	writable := make([]table.Index, 0, len(t.indices))
+	for _, index := range t.indices {
+		s := index.Meta().State
+		if s != model.StateNone && s != model.StateDeleteOnly && s != model.StateDeleteReorganization {


model.StateDeleteReorganization state is writable ?

I think we don't need to consider the state of StateNone. The same as line 145.

tiancaiamao · 2017-07-17T11:02:20Z

table/tables/tables.go

 		}
 	}
-
-	return t.publicColumns
+	return publicColumns[0 : maxOffset+1]


Some empty hole in the result columns ?

ddl.adjustColumnOffset guarantees the offsets are continuous.

tiancaiamao · 2017-07-17T11:40:09Z

table/tables/tables.go

+// 1. the column is included in primary key;
+// 2. the column's default value is null, and the value equals to that;
+// 3. the column is virtual generated.
+func (t *Table) CanSkip(col *table.Column, value types.Datum) bool {


Is it necessary to make the method public?

shenli · 2017-07-18T13:14:02Z

@tiancaiamao @zimulala @hanfei1991 PTAL

shenli · 2017-07-19T03:01:40Z

executor/write.go

+func updateRecord(ctx context.Context, h int64, oldData, newData []types.Datum, touched []bool, t table.Table, onDup bool) (bool, error) {
+	var sc = ctx.GetSessionVars().StmtCtx
+	var changed, handleChanged = false, false
+	var onUpdateNoChange = make(map[int]bool)


Add comment for this variable.

shenli · 2017-07-19T03:06:51Z

executor/write.go

-	// If no assign list for this table, no need to update.
-	if !assignExists {
-		return false, nil
+	for i, col := range t.Cols() {


Any test case for this?

shenli · 2017-07-19T03:08:18Z

executor/write.go

+			if err != nil {
+				return false, errors.Trace(err)
+			}
+			newData[i] = v


Should we update the variable changed here?

no, but there is another bug: only if changed == true we can change those on-update fields.

shenli · 2017-07-19T03:10:13Z

executor/write.go

 			rowCount = 0
 		}
+		txn := e.ctx.Txn()


We only need to get a new txn after commit a batch of data. So it is better to move this back into the if statement.

OK, I made a mistake.

shenli · 2017-07-19T03:10:57Z

executor/write.go

@@ -974,15 +969,16 @@ func (e *InsertValues) adjustAutoIncrementDatum(row []types.Datum, i int, c *tab
 // onDuplicateUpdate updates the duplicate row.
 // TODO: Report rows affected and last insert id.
 func (e *InsertExec) onDuplicateUpdate(row []types.Datum, h int64, cols []*expression.Assignment) error {
-	data, err := e.Table.Row(e.ctx, h)
+	// See http://dev.mysql.com/doc/refman/5.7/en/miscellaneous-functions.html#function_values
+	e.ctx.GetSessionVars().CurrInsertValues = row


Why move it here?

I thinks it looks better. :)

FYI: it's not always better, if error happen, and statement retry, CurrInsertValues should set to the real current insert value.

OK, I will restore that.

shenli · 2017-07-19T03:20:39Z

plan/logical_plan_builder.go

 	var columns []*table.Column
 	if b.inUpdateStmt {
 		columns = tbl.WritableCols()
 	} else {
 		columns = tbl.Cols()
 	}
+	p.Columns = make([]*model.ColumnInfo, 0, len(columns))


This is performance improvement?

I think it's a little improvement, because we only need alloc space with length as len(columns), not len(tableInfo.Columns).

shenli · 2017-07-19T03:21:14Z

table/table.go

 	AddRecord(ctx context.Context, r []types.Datum) (recordID int64, err error)

-	// UpdateRecord updates a row in the table.
-	UpdateRecord(ctx context.Context, h int64, currData []types.Datum, newData []types.Datum, touched map[int]bool) error
+	// UpdateRecord updates a row which should contain only writable columns.


writable or public?

For AddRecord, it should be public. But for UpdateRecord, it should be write-only + public because we need fetch write-only fields back, and write them into tikv again.

shenli · 2017-07-19T03:22:45Z

table/tables/tables.go

-func (t *Table) rebuildIndices(rm kv.RetrieverMutator, h int64, touched map[int]bool, oldData []types.Datum, newData []types.Datum) error {
-	for _, idx := range t.Indices() {
-		idxTouched := false
+	for _, idx := range t.WritableIndices() {


This is a bug fix?

Previously check the index is writable or not is done in buildIndexForRow. I moved that here for more easily to understand.

shenli · 2017-07-19T03:23:53Z

table/tables/tables.go

@@ -137,33 +159,35 @@ func (t *Table) Cols() []*table.Column {
 	if len(t.publicColumns) > 0 {
 		return t.publicColumns
 	}
-
-	t.publicColumns = make([]*table.Column, 0, len(t.Columns))
+	publicColumns := make([]*table.Column, len(t.Columns))


So line 159 will always be false?

just for the first time it will be false. When everytimes we load schemas from tikv, t.publicColumns = t.Cols() is called.

zimulala · 2017-07-19T15:01:13Z

table/tables/tables.go

-
-	t.publicColumns = make([]*table.Column, 0, len(t.Columns))
+	publicColumns := make([]*table.Column, len(t.Columns))
+	maxOffset := -1
 	for _, col := range t.Columns {
 		if col.State == model.StatePublic {


The following code style is more like Go's style.

if col.State != model.StatePublix{ continue }

zimulala · 2017-07-20T02:24:17Z

executor/write.go

+			} else {
+				continue
+			}
+		}
 	}


Move line 92-103 behind line 118. Then you can move the judgment condition of changed.

zimulala · 2017-07-20T02:25:28Z

executor/write.go

+				}
+				newData[i] = v
+			} else {
+				continue


This else is not needed.

shenli · 2017-07-21T10:15:33Z

executor/write.go

 		// See https://dev.mysql.com/doc/refman/5.7/en/mysql-real-connect.html  CLIENT_FOUND_ROWS
 		if ctx.GetSessionVars().ClientCapability&mysql.ClientFoundRows > 0 {
 			sc.AddAffectedRows(1)
 		}
 		return false, nil
 	}

-	var err error
-	if !newHandle.IsNull() {
+	// Fill values into on-update-now fields, only if they are really changed.


Any test case covers this?

Test cases covering this are in tidb-test/mysqltest. If here the logic is wrong, this error will happen.

zimulala · 2017-07-22T06:39:00Z

table/tables/tables.go

+	writable := make([]table.Index, 0, len(t.indices))
+	for _, index := range t.indices {
+		s := index.Meta().State
+		if s != model.StateNone && s != model.StateDeleteOnly && s != model.StateDeleteReorganization {


I think we don't need to consider the state of StateNone. The same as line 145.

zimulala · 2017-07-25T03:32:22Z

table/tables/tables.go

 		var value types.Datum
-		if col.State == model.StateWriteOnly || col.State == model.StateWriteReorganization {
+		if col.State != model.StatePublic {
 			// if col is in write only or write reorganization state, we must add it with its default value.


s/if/If.
This comment may need to be updated.

zimulala · 2017-07-25T06:02:28Z

table/tables/tables.go

+			row = append(row, value)
+		}
+		if shouldWriteBinlog(ctx) && !t.canSkipUpdateBinlog(col, value) {
+			binlogColIDs = append(binlogColIDs, col.ID)


Why not use the same colIDs?
I think that using the same colIDs and row can reduce memory and reduce encoding times.

Because colIDs is for encoding values into row stored in Tikv, but binlogColIDs is used for binlog. They are differant. For example, if a column is PK handle, it won't be encoded into row for saving space, but it should appear in binlog.

zimulala · 2017-07-25T09:39:05Z

LGTM

hicqu · 2017-07-26T06:06:52Z

Now this PR has merged #3804 , and it can deal with the two state bug.

zimulala · 2017-07-26T06:57:06Z

PTAL @tiancaiamao @shenli

tiancaiamao · 2017-07-27T05:11:29Z

LGTM

hicqu · 2017-07-27T14:40:27Z

@shenli , PTAL again, thanks.

shenli · 2017-07-28T10:17:38Z

LGTM

This reverts commit 597796e.

hicqu added 5 commits July 14, 2017 15:41

*: fix bad column offsets.

669e2e8

fix ci.

50c258c

improve code.

631a8ca

fix bug.

c7a74c7

Merge branch 'master' into qupeng/column-offset

d2f8a91

hicqu requested review from zimulala, shenli, tiancaiamao and hanfei1991 July 14, 2017 09:25

hicqu mentioned this pull request Jul 14, 2017

plan, executor: support almost full features about generated column. #3516

Closed

hicqu added 2 commits July 17, 2017 12:00

Merge branch 'master' into qupeng/column-offset

b297477

Merge branch 'master' into qupeng/column-offset

22bb700

tiancaiamao reviewed Jul 17, 2017

View reviewed changes

address comments.

93240dd

hicqu closed this Jul 18, 2017

hicqu deleted the qupeng/column-offset branch July 18, 2017 06:14

hicqu restored the qupeng/column-offset branch July 18, 2017 06:14

hicqu reopened this Jul 18, 2017

Merge branch 'master' into qupeng/column-offset

c8dde90

hicqu added the priority/P1 The issue has P1 priority. label Jul 18, 2017

hicqu added 2 commits July 18, 2017 16:15

fix ci.

96a667a

Merge branch 'master' into qupeng/column-offset

397a44d

shenli reviewed Jul 19, 2017

View reviewed changes

hicqu added 2 commits July 19, 2017 15:00

address comments.

5b1cddc

Merge branch 'master' into qupeng/column-offset

5846f7f

zimulala reviewed Jul 20, 2017

View reviewed changes

hicqu added 3 commits July 21, 2017 13:59

Merge branch 'master' into qupeng/column-offset

bb70730

fix bad comment.

906cf4e

Merge branch 'master' into qupeng/column-offset

370c1e4

shenli reviewed Jul 21, 2017

View reviewed changes

hicqu added 2 commits July 24, 2017 13:28

Merge branch 'master' into qupeng/column-offset

1269693

Merge branch 'master' into qupeng/column-offset

f8f0df2

zimulala reviewed Jul 25, 2017

View reviewed changes

hicqu added 2 commits July 25, 2017 14:40

address comments.

5b9e9e5

Merge branch 'master' into qupeng/column-offset

7665b13

hicqu force-pushed the qupeng/column-offset branch from a486785 to 7665b13 Compare July 25, 2017 08:47

zimulala added the status/LGT1 Indicates that a PR has LGTM 1. label Jul 25, 2017

hicqu force-pushed the qupeng/column-offset branch from 4443787 to 7665b13 Compare July 26, 2017 05:01

hicqu added 2 commits July 26, 2017 13:02

Merge branch 'master' into qupeng/column-offset

2f7b85f

ddl: fix ddl_db_change_test.go.

1661ec3

address comments.

42939df

shenli approved these changes Jul 28, 2017

View reviewed changes

Merge branch 'master' into qupeng/column-offset

dbf52ea

zimulala added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jul 28, 2017

Merge branch 'master' into qupeng/column-offset

044478d

shenli merged commit 597796e into master Jul 28, 2017

shenli deleted the qupeng/column-offset branch July 28, 2017 12:56

shenli added a commit that referenced this pull request Jul 28, 2017

Revert "*: fix bad column offsets in DML. (#3754)"

7b6d757

This reverts commit 597796e.

coocood mentioned this pull request Feb 8, 2018

plan: get correct columns #5818

Merged

*: fix bad column offsets. #3754

*: fix bad column offsets. #3754

Conversation

hicqu commented Jul 14, 2017 • edited Loading

shenli commented Jul 14, 2017

shenli commented Jul 17, 2017

tiancaiamao left a 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

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

shenli commented Jul 18, 2017

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

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

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

hicqu Jul 21, 2017 • edited Loading

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

zimulala commented Jul 25, 2017

hicqu commented Jul 26, 2017

zimulala commented Jul 26, 2017

tiancaiamao commented Jul 27, 2017

hicqu commented Jul 27, 2017

shenli commented Jul 28, 2017

hicqu commented Jul 14, 2017 •

edited

Loading

hicqu Jul 21, 2017 •

edited

Loading