New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

executor: fix panic and update error data when table has column in write only state #8792

Merged
merged 3 commits into from Jan 2, 2019

Conversation

@crazycs520
Copy link
Contributor

crazycs520 commented Dec 24, 2018

What problem does this PR solve?

When table have a column in write only state, there are some problem.
For the following example, surpose table have 3 columns: a int key,b int,c int not null, but c column is in write only state. column a is primary key, also handle key.

  1. because insert only write public column, but update will also write column on write only state. then, in a transaction, insert first, then update will panic because update try to look up the value of write-only column from dirty table when union scan, because insert doesn't write the value of write-only column to dirty table, so update panic by index out of range.

  2. when do update, if handle is changed, update will remove recored first, then call table.AddRecord, update will input newData with write-only column value to table.AddRecord, but table.AddRecord will think the last value is _tidb_rowid, actualy the last value of newData is the value of write-only column. Then will cause update wrong record or overwrite the other row record. see code here: https://github.com/pingcap/tidb/pull/5984/files#diff-f92a6a9ee57cf92f8d63bd55d2edc05bR356

  3. Because online change scheme will have atmostly 2 state in cluster. Add column c, state change is: none -> delete only -> write only -> write reorganization -> public. Suppose TiDB-A is in public state , TiDB-B is in write reorganization.

first, execute in TiDB-A: insert into t (a,b,c) value (1,1,1);
then, execute in TiDB-B: update t set a=2 where a=1;

finally, the result of select a,b,c from t should be 2,1,1, but in fact, the result is 2,1,0, the value of column c is overwritten when TiDB-B executes update.

What is changed and how it works?

For problem 1: insert should also write value of write-only column to dirty table, to avoid update panic.

For problem 2 and 3, change table.AddRecord interface, add a flag to indicate this is call for insert or update. if for update,

  1. can not think the last value is _tidb_rowid, because not allow update _tidb_rowid.
  2. should get the value of write-only column from input newData, should not get from default value.

Check List

Tests

  • Unit test

Code changes

Side effects

Related changes

  • Need to cherry-pick to the release branch

This change is Reviewable

@crazycs520

This comment has been minimized.

Copy link
Contributor

crazycs520 commented Dec 24, 2018

/run-all-tests

@crazycs520 crazycs520 requested a review from zimulala Dec 24, 2018

@crazycs520

This comment has been minimized.

Copy link
Contributor

crazycs520 commented Dec 24, 2018

@crazycs520 crazycs520 force-pushed the crazycs520:panic-by-not-public-column branch from 84ae70a to 0cdf468 Dec 24, 2018

@crazycs520

This comment has been minimized.

Copy link
Contributor

crazycs520 commented Dec 24, 2018

/rebuild

@crazycs520

This comment has been minimized.

Copy link
Contributor

crazycs520 commented Dec 24, 2018

/run-all-tests

@jackysp
Copy link
Member

jackysp left a comment

Do we need to append value to newData after

value = oldData[col.Offset]
?

@crazycs520

This comment has been minimized.

Copy link
Contributor

crazycs520 commented Dec 25, 2018

@jackysp newData will already have the value of writeOnly column.

columns = tbl.WritableCols()

@jackysp

This comment has been minimized.

Copy link
Member

jackysp commented Dec 25, 2018

How about insert on duplicate key update?

@crazycs520 crazycs520 force-pushed the crazycs520:panic-by-not-public-column branch from e40f54a to 7ce2a24 Dec 26, 2018

@crazycs520

This comment has been minimized.

Copy link
Contributor

crazycs520 commented Dec 26, 2018

/run-all-tests

@crazycs520 crazycs520 removed the status/WIP label Dec 26, 2018

@crazycs520

This comment has been minimized.

Copy link
Contributor

crazycs520 commented Dec 26, 2018

/run-unit-test

@@ -143,7 +143,7 @@ func updateRecord(ctx sessionctx.Context, h int64, oldData, newData []types.Datu
return false, false, 0, errors.Trace(err)
}
// the `affectedRows` is increased when adding new record.
newHandle, err = t.AddRecord(ctx, newData, skipHandleCheck)
newHandle, err = t.AddRecord(ctx, newData[:len(t.Cols())], skipHandleCheck)

This comment has been minimized.

@winkyao

winkyao Dec 26, 2018

Member

Add some comments to explain.

@crazycs520

This comment has been minimized.

Copy link
Contributor

crazycs520 commented Dec 26, 2018

@jackysp PTAL

@crazycs520 crazycs520 changed the title ddl: fix panic in transaction when add/drop column. executor: fix panic and update error data when table has write only table. Dec 26, 2018

@crazycs520 crazycs520 changed the title executor: fix panic and update error data when table has write only table. executor: fix panic and update error data when table has write only column Dec 26, 2018

@crazycs520 crazycs520 changed the title executor: fix panic and update error data when table has write only column executor: fix panic and update error data when table has column in write only state Dec 26, 2018

@crazycs520 crazycs520 force-pushed the crazycs520:panic-by-not-public-column branch from 59d5c39 to 0f5cc8d Dec 26, 2018

@winkyao
Copy link
Member

winkyao left a comment

LGTM

@shenli

This comment has been minimized.

Copy link
Member

shenli commented Dec 26, 2018

/run-all-tests

@crazycs520

This comment has been minimized.

Copy link
Contributor

crazycs520 commented Dec 26, 2018

/run-unit-test

@winkyao winkyao added the status/LGT1 label Dec 27, 2018

@crazycs520 crazycs520 force-pushed the crazycs520:panic-by-not-public-column branch from 203e2cd to a115126 Dec 27, 2018

@crazycs520

This comment has been minimized.

Copy link
Contributor

crazycs520 commented Dec 29, 2018

@jackysp How about adding a new interface AddRecordFForUpdate?

@crazycs520 crazycs520 force-pushed the crazycs520:panic-by-not-public-column branch from 235ae1d to e95dca1 Jan 2, 2019

@crazycs520

This comment has been minimized.

Copy link
Contributor

crazycs520 commented Jan 2, 2019

/run-all-tests

@crazycs520

This comment has been minimized.

Copy link
Contributor

crazycs520 commented Jan 2, 2019

@crazycs520

This comment has been minimized.

Copy link
Contributor

crazycs520 commented Jan 2, 2019

/run-common-test

@jackysp
Copy link
Member

jackysp left a comment

LGTM

@lysu

This comment has been minimized.

Copy link
Member

lysu commented Jan 2, 2019

LGTM

@lysu lysu added status/LGT2 and removed status/LGT1 labels Jan 2, 2019

@@ -425,7 +425,7 @@ func (t *tableCommon) AddRecord(ctx sessionctx.Context, r []types.Datum, opts ..
}
var hasRecordID bool
cols := t.Cols()
if len(r) > len(cols) {
if len(r) > len(cols) && !opt.IsUpdate {

This comment has been minimized.

@winkyao

winkyao Jan 2, 2019

Member

Add some comments about opt.IsUpdate

@winkyao
Copy link
Member

winkyao left a comment

reset LGTM

crazycs520 added some commits Jan 2, 2019

@crazycs520

This comment has been minimized.

Copy link
Contributor

crazycs520 commented Jan 2, 2019

/run-all-tests

@winkyao

winkyao approved these changes Jan 2, 2019

Copy link
Member

winkyao left a comment

LGTM

@crazycs520 crazycs520 added status/LGT3 and removed status/LGT2 labels Jan 2, 2019

@crazycs520

This comment has been minimized.

Copy link
Contributor

crazycs520 commented Jan 2, 2019

/run-unit-test

@crazycs520

This comment has been minimized.

Copy link
Contributor

crazycs520 commented Jan 2, 2019

/run-common-test

1 similar comment
@crazycs520

This comment has been minimized.

Copy link
Contributor

crazycs520 commented Jan 2, 2019

/run-common-test

@crazycs520 crazycs520 merged commit b3698f6 into pingcap:master Jan 2, 2019

12 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
idc-jenkins-ci-tidb/build Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/common-test job succeeded
Details
idc-jenkins-ci-tidb/integration-common-test Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/integration-compatibility-test Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/integration-ddl-test Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/mybatis-test job succeeded
Details
idc-jenkins-ci-tidb/sqllogic-test-1 Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/sqllogic-test-2 Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/unit-test Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details

crazycs520 added a commit to crazycs520/tidb that referenced this pull request Jan 2, 2019

crazycs520 added a commit to crazycs520/tidb that referenced this pull request Jan 2, 2019

zz-jason added a commit that referenced this pull request Jan 2, 2019

winkyao added a commit that referenced this pull request Jan 2, 2019

tiancaiamao added a commit that referenced this pull request Jan 2, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment