Skip to content
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 auto-id allocation during statements retry #20659

Merged
merged 8 commits into from
Nov 9, 2020

Conversation

tangenta
Copy link
Contributor

What problem does this PR solve?

Issue Number: close #20629

What is changed and how it works?

What's Changed:

Since the inserting rows might be different between transaction retries, the auto IDs array in RetryInfo can be treated as a buffer: TiDB always try to reuse the IDs in this buffer. Only if the buffer is empty, a new auto ID is allocated from TiKV.

Related changes

  • Need to cherry-pick to the release branch

Check List

Tests

  • Unit test
  • Integration test

Side effects

N/A

Release note

  • Fix auto-id allocation failed because of transaction's write-conflict retry.

@tangenta tangenta requested a review from a team as a code owner October 27, 2020 06:06
@tangenta tangenta requested review from qw4990 and removed request for a team October 27, 2020 06:06
@github-actions github-actions bot added the sig/execution SIG execution label Oct 27, 2020
@bb7133
Copy link
Member

bb7133 commented Oct 28, 2020

There is a failed case(https://internal.pingcap.net/idc-jenkins/blue/organizations/jenkins/tidb_ghpr_check_2/detail/tidb_ghpr_check_2/57413/pipeline):

[2020-10-27T09:41:30.036Z] session_test.go:1167:
[2020-10-27T09:41:30.036Z]     tk.MustExec("commit")
[2020-10-27T09:41:30.036Z] /home/jenkins/agent/workspace/tidb_ghpr_check_2/go/src/github.com/pingcap/tidb/util/testkit/testkit.go:208:
[2020-10-27T09:41:30.036Z]     tk.c.Assert(err, check.IsNil, check.Commentf("sql:%s, %v, error stack %v", sql, args, errors.ErrorStack(err)))
[2020-10-27T09:41:30.036Z] ... value *errors.withMessage = previous statement: update t set c2 = 33 where c2 = 1: [kv:1062]Duplicate entry '0' for key 'PRIMARY'
[2020-10-27T09:41:30.036Z] ... sql:commit, [], error stack [kv:1062]Duplicate entry '0' for key 'PRIMARY'
[2020-10-27T09:41:30.036Z] previous statement: update t set c2 = 33 where c2 = 1

PTAL @tangenta

@tangenta tangenta requested a review from bb7133 October 28, 2020 07:53
@@ -598,7 +599,7 @@ func (e *InsertValues) fillRow(ctx context.Context, row []types.Datum, hasValue

// isAutoNull can help judge whether a datum is AutoIncrement Null quickly.
// This used to help lazyFillAutoIncrement to find consecutive N datum backwards for batch autoID alloc.
func (e *InsertValues) isAutoNull(ctx context.Context, d types.Datum, col *table.Column) bool {
func (e *InsertValues) isAutoNull(ctx context.Context, d *types.Datum, col *table.Column) bool {
Copy link
Member

Choose a reason for hiding this comment

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

IMHO we don't need to change it to pass-by-reference in case that it may be modified carelessly in the future?

}
}
return rows, nil
func setDatumAutoIDWithCast(ctx sessionctx.Context, d *types.Datum, id int64, col *table.Column) error {
Copy link
Member

Choose a reason for hiding this comment

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

Do we need HandleBadNull?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since we d.SetAutoID explicitly , the datum must be not null. HandleBadNull is meaningless.

Copy link
Member

@wjhuang2016 wjhuang2016 left a comment

Choose a reason for hiding this comment

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

LGTM

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Nov 4, 2020
Copy link
Contributor

@AilinKid AilinKid left a comment

Choose a reason for hiding this comment

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

Rest LGTM

}()
wg.Wait()
for _, e := range err {
c.Assert(e, IsNil)
Copy link
Contributor

@AilinKid AilinKid Nov 5, 2020

Choose a reason for hiding this comment

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

should we use the begin/commit to control the ts of txn2 between the begin and commit of txn1? that will make sure the retry is absolutely necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems the explicit commit does not trigger auto-retry:

mysql> create table t (id int auto_increment unique key, idx int unique key, c int);
Query OK, 0 rows affected (0.00 sec)

mysql> begin;
Query OK, 0 rows affected (0.00 sec)

mysql> insert into t(idx, c) values (1, 1) on duplicate key update c = 2;
Query OK, 1 row affected (0.01 sec)

mysql> commit;
Query OK, 0 rows affected (0.00 sec)
mysql> begin;
Query OK, 0 rows affected (0.00 sec)

mysql> insert into t(idx, c) values (1, 1) on duplicate key update c = 2;
Query OK, 1 row affected (0.00 sec)

mysql> commit;
ERROR 9007 (HY000): Write conflict, txnStartTS=420644278338322432, conflictStartTS=420644277094973440, conflictCommitTS=420644294221365248, key={tableID=49, indexID=2, indexValues={1, }} primary=[]byte(nil) [try again later]

Copy link
Contributor

Choose a reason for hiding this comment

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

Because PessimisticTxn is set to true by default ...

Copy link
Contributor

@AilinKid AilinKid left a comment

Choose a reason for hiding this comment

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

LGTM

@ti-srebot ti-srebot removed the status/LGT1 Indicates that a PR has LGTM 1. label Nov 9, 2020
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Nov 9, 2020
@AilinKid
Copy link
Contributor

AilinKid commented Nov 9, 2020

/run-all-tests

@AilinKid AilinKid merged commit 5222757 into pingcap:master Nov 9, 2020
tangenta added a commit to tangenta/tidb that referenced this pull request Nov 16, 2020
ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Nov 17, 2020
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot
Copy link
Contributor

cherry pick to release-4.0 in PR #21105

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/execution SIG execution sig/sql-infra SIG: SQL Infra status/LGT2 Indicates that a PR has LGTM 2. type/bug-fix This PR fixes a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot get auto-id in retry 'INSERT INTO ... SELECT FROM'
6 participants