-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
Conversation
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):
PTAL @tangenta |
executor/insert_common.go
Outdated
@@ -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 { |
There was a problem hiding this comment.
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?
executor/insert_common.go
Outdated
} | ||
} | ||
return rows, nil | ||
func setDatumAutoIDWithCast(ctx sessionctx.Context, d *types.Datum, id int64, col *table.Column) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need HandleBadNull
?
There was a problem hiding this comment.
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.
59f609a
to
064e164
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this 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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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]
There was a problem hiding this comment.
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 ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/run-all-tests |
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
cherry pick to release-4.0 in PR #21105 |
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
Check List
Tests
Side effects
N/A
Release note