executor: fix update ignore still report dup-key error in statement retry #54495

lcwangchao · 2024-07-08T08:02:16Z

What problem does this PR solve?

Issue Number: close #54489

What changed and how does it work?

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No need to test
- I checked and no code files have been changed.

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

fix some dup-key cases in UpdateRecord

tiprow · 2024-07-08T08:02:36Z

Hi @lcwangchao. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

codecov · 2024-07-08T08:17:04Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 56.2582%. Comparing base (0c9a679) to head (1f34c5f).
Report is 72 commits behind head on master.

Additional details and impacted files

@@                Coverage Diff                @@
##             master     #54495         +/-   ##
=================================================
- Coverage   74.7741%   56.2582%   -18.5159%     
=================================================
  Files          1539       1669        +130     
  Lines        361871     615209     +253338     
=================================================
+ Hits         270586     346106      +75520     
- Misses        71638     245607     +173969     
- Partials      19647      23496       +3849

Flag	Coverage Δ
integration	`37.1187% <100.0000%> (?)`
unit	`71.7767% <100.0000%> (-1.9076%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
dumpling	`52.9656% <ø> (-2.2339%)`	⬇️
parser	`∅ <ø> (∅)`
br	`52.2160% <ø> (+4.3332%)`	⬆️

ekexium · 2024-07-10T05:48:50Z

pkg/table/tables/tables.go

-		if err != nil {
-			return err
-		}
+	err = t.rebuildIndices(sctx, txn, h, touched, oldData, newData, table.WithCtx(ctx))


Why change these lines? The test still passes if these lines are unchanged.

Because this code is unnecessary now. Deleting these lines can make code clear. The current LazyCheckKeyNotExists is:

func (s *SessionVars) LazyCheckKeyNotExists() bool { if s.StmtCtx.ErrGroupLevel(errctx.ErrGroupDupKey) != errctx.LevelError { // This branch means we are in `insert/update ignore`. // The executor will handle the dup-key error and ignore it in executor, // so we must check the dup-key error in place to make sure the executor can get the error. return false } return s.PresumeKeyNotExists || (s.TxnCtx != nil && s.TxnCtx.IsPessimistic) }

You can see that if sessVars.TxnCtx.IsPessimistic is true, s.PresumeKeyNotExists || (s.TxnCtx != nil && s.TxnCtx.IsPessimistic) will always return true, we do not need to set PresumeKeyNotExists.

reverted diffs in LazyCheckKeyNotExists can also fix this bug...

‌‌Let's go back to the original place. The purpose of tidb_constraint_check_in_place is only to make the lazy unique index check for insert effective. Historically, only inserts had a lazy unique index check. tidb_constraint_check_in_place was actually designed to remove this exclusive behavior for inserts, so that all statements execute consistent in-place checks. Therefore, update statements have never had a lazy check. So even if tidb_constraint_check_in_place is off, updates should still report an error. @lcwangchao @ekexium

As for why the name was designed this way, that was the product manager's decision at the time. However, I don't think it's necessary to make updates lazy check as well. The reason insert needs lazy check is because it performs more efficiently during bulk loading. Bulk loading should rarely involve updates, right?

If you want the update to also perform a lazy check, handling it only at

tidb/pkg/table/tables/tables.go

Lines 579 to 589 in 08147e7

if !sessVars.InTxn() {

savePresumeKeyNotExist := sessVars.PresumeKeyNotExists

if !sessVars.ConstraintCheckInPlace && sessVars.TxnCtx.IsPessimistic {

sessVars.PresumeKeyNotExists = true

}

err = t.rebuildIndices(sctx, txn, h, touched, oldData, newData, table.WithCtx(ctx))

sessVars.PresumeKeyNotExists = savePresumeKeyNotExist

if err != nil {

return err

}

} else {

is not enough. It also requires the 2PC phase to first check for unique constraints like insert before committing. This way, for #54489, there will be no retry during an update; instead, a duplicate entry error reported by TiKV will be received by TiDB, and it will convert it to a warning.

I think it's a discuss about issue #54492 . This PR just disable force lazy check for pessimistic txn when update ignore. For #54492, I think we can left above comments there and just close it.

ekexium

Consider a pessimistic txn, constraint_check_in_place=off, and error level is LevelWarn. In original code LazyCheckKeyNotExists will return true, in this PR it will return false, right?
If we let the original value of vars.PresumeKeyNotExists be !vars.ConstraintCheckInPlace we will have the table:

InTxn	constraint_check_in_place	mode	levelError	master, lazy?	PR, lazy?	Equivalent
FALSE	FALSE	opt	FALSE	TRUE	FALSE	FALSE
FALSE	FALSE	opt	TRUE	TRUE	TRUE	TRUE
FALSE	FALSE	pes	FALSE	TRUE	FALSE	FALSE
FALSE	FALSE	pes	TRUE	TRUE	TRUE	TRUE
FALSE	TRUE	opt	FALSE	FALSE	FALSE	TRUE
FALSE	TRUE	opt	TRUE	FALSE	FALSE	TRUE
FALSE	TRUE	pes	FALSE	FALSE	FALSE	TRUE
FALSE	TRUE	pes	TRUE	TRUE	TRUE	TRUE
TRUE	FALSE	opt	FALSE	TRUE	FALSE	FALSE
TRUE	FALSE	opt	TRUE	TRUE	TRUE	TRUE
TRUE	FALSE	pes	FALSE	TRUE	FALSE	FALSE
TRUE	FALSE	pes	TRUE	TRUE	TRUE	TRUE
TRUE	TRUE	opt	FALSE	FALSE	FALSE	TRUE
TRUE	TRUE	opt	TRUE	FALSE	FALSE	TRUE
TRUE	TRUE	pes	FALSE	FALSE	FALSE	TRUE
TRUE	TRUE	pes	TRUE	TRUE	TRUE	TRUE

Besides, s.PresumeKeyNotExists only makes the reasoning more complex. Since it's only set in one place, I think we should consider removing it.

lcwangchao · 2024-07-10T08:00:47Z

Consider a pessimistic txn, constraint_check_in_place=off, and error level is LevelWarn. In original code LazyCheckKeyNotExists will return true, in this PR it will return false, right? If we let the original value of vars.PresumeKeyNotExists be !vars.ConstraintCheckInPlace we will have the table:

InTxn constraint_check_in_place mode levelError master, lazy? PR, lazy? Equivalent
FALSE FALSE opt FALSE TRUE FALSE FALSE
FALSE FALSE opt TRUE TRUE TRUE TRUE
FALSE FALSE pes FALSE TRUE FALSE FALSE
FALSE FALSE pes TRUE TRUE TRUE TRUE
FALSE TRUE opt FALSE FALSE FALSE TRUE
FALSE TRUE opt TRUE FALSE FALSE TRUE
FALSE TRUE pes FALSE FALSE FALSE TRUE
FALSE TRUE pes TRUE TRUE TRUE TRUE
TRUE FALSE opt FALSE TRUE FALSE FALSE
TRUE FALSE opt TRUE TRUE TRUE TRUE
TRUE FALSE pes FALSE TRUE FALSE FALSE
TRUE FALSE pes TRUE TRUE TRUE TRUE
TRUE TRUE opt FALSE FALSE FALSE TRUE
TRUE TRUE opt TRUE FALSE FALSE TRUE
TRUE TRUE pes FALSE FALSE FALSE TRUE
TRUE TRUE pes TRUE TRUE TRUE TRUE
Besides, s.PresumeKeyNotExists only makes the reasoning more complex. Since it's only set in one place, I think we should consider removing it.

This PR only changes LazyCheckKeyNotExists in the pessimistic mode. In optimistic mode, it's return value is affected by PresumeKeyNotExists. In the update statement, it is not set anyway, so this PR does not change the return value here. In the insert statement, it only changes the return value when error level is LevelWarn, however, this only happens in insert ignore. The LazyCheckKeyNotExists is not used because BatchCheck is set to true...

Yes, I think PresumeKeyNotExists is boring and we should remove it. But I think we should also remove LazyCheckKeyNotExists and use a new option WithDupKeyCheckMode (default is checkInPlace) to indicate it. Each scene should compute the DupKeyCheckMode separately.

ekexium · 2024-07-10T08:20:36Z

So the correctness of this fix depends on how upper layer uses it. I suggest that we make LazyCheckKeyNotExists unchanged for optimistic transactions, instead of depending on the callers. It seems a safer approach.

lcwangchao · 2024-07-10T08:39:47Z

/retest

tiprow · 2024-07-10T08:40:08Z

@lcwangchao: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

lcwangchao · 2024-07-10T09:11:06Z

/retest

tiprow · 2024-07-10T09:11:29Z

@lcwangchao: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

lcwangchao · 2024-07-10T09:22:39Z

/retest

tiprow · 2024-07-10T09:23:01Z

@lcwangchao: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

lcwangchao · 2024-07-10T09:34:11Z

/retest

tiprow · 2024-07-10T09:34:32Z

@lcwangchao: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

lcwangchao · 2024-07-10T13:20:54Z

/retest

tiprow · 2024-07-10T13:21:17Z

@lcwangchao: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

ekexium

Have you verified #20484? Seems the part was introduced to solve that issue.

lcwangchao · 2024-07-12T08:35:56Z

Have you verified #20484? Seems the part was introduced to solve that issue.

Seems it is trying to make a lazy check for update... But I don't understand this PR, if a transaction is pessimistic and the statement is not update ignore, LazyCheckKeyNotExists should always return true. If the statement is update ignore, this bug will occur...

@tiancaiamao could you PTAL?

tiancaiamao

The current master branch does not work as expected.
If there is no begin, the expected behavior is that index.Create does not need tikvSnapshotGet to check unique key exist or not.

If there is a begin, both master branch and this PR don't use tikvSnapshotGet under index.Create:

This PR does not make thing change, and it simplify the logic, so LGTM
As for why the current master branch does not work as expected, we can take a separate thread to trace...

cfzjywxk

It's difficult to verify the correctness for all the combinations of optimistic/pessimistic mods and check in-place or not directly.

We may need to file a refactor and test coverage task to clarify the related code path, especially the optimistic mode would be deprecated in the future. It could be also considered as one of the sub-tasks of the deprecation.
/cc @lcwangchao @ekexium

ti-chi-bot · 2024-07-16T12:56:09Z

@cfzjywxk: GitHub didn't allow me to request PR reviews from the following users: lcwangchao.

Note that only pingcap members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

It's difficult to verify the correctness for all the combinations of optimistic/pessimistic mods and check in-place or not directly.

We may need to file a refactor and test coverage task to clarify the related code path, especially the optimistic mode would be deprecated in the future. It could be also considered as one of the sub-tasks of the deprecation.
/cc @lcwangchao @ekexium

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

ti-chi-bot · 2024-07-16T12:56:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cfzjywxk, tiancaiamao

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [cfzjywxk,tiancaiamao]
~~pkg/table/OWNERS~~ [cfzjywxk]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ti-chi-bot · 2024-07-16T12:56:17Z

[LGTM Timeline notifier]

Timeline:

2024-07-12 13:53:15.192697164 +0000 UTC m=+16417.183638635: ☑️ agreed by tiancaiamao.
2024-07-16 12:56:16.413604905 +0000 UTC m=+358598.404546359: ☑️ agreed by cfzjywxk.

executor: fix some dup-key cases in UpdateRecord

127e4b5

ti-chi-bot bot added release-note size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 8, 2024

ti-chi-bot bot added the do-not-merge/needs-linked-issue label Jul 8, 2024

lcwangchao changed the title ~~executor: fix some dup-key cases in UpdateRecord~~ Jul 8, 2024

ti-chi-bot bot removed the do-not-merge/needs-linked-issue label Jul 8, 2024

only fix pingcap#54489

8ff6dca

lcwangchao force-pushed the fix_dupkey branch from b2ba885 to 8ff6dca Compare July 8, 2024 10:27

lcwangchao requested a review from ekexium July 9, 2024 02:28

ekexium reviewed Jul 10, 2024

View reviewed changes

ekexium requested a review from cfzjywxk July 10, 2024 08:14

ingore optimistic

f1063bc

update

1f34c5f

ekexium reviewed Jul 12, 2024

View reviewed changes

tiancaiamao approved these changes Jul 12, 2024

View reviewed changes

ti-chi-bot bot added the needs-1-more-lgtm label Jul 12, 2024

lcwangchao mentioned this pull request Jul 16, 2024

optimistic transaction does not respect tidb_constraint_check_in_place #54492

Closed

cfzjywxk approved these changes Jul 16, 2024

View reviewed changes

ti-chi-bot bot requested a review from ekexium July 16, 2024 12:56

ti-chi-bot bot added approved lgtm and removed needs-1-more-lgtm labels Jul 16, 2024

ti-chi-bot bot merged commit 7a09434 into pingcap:master Jul 16, 2024
22 of 23 checks passed

lcwangchao deleted the fix_dupkey branch July 17, 2024 01:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

executor: fix update ignore still report dup-key error in statement retry #54495

executor: fix update ignore still report dup-key error in statement retry #54495

lcwangchao commented Jul 8, 2024 •

edited

Loading

tiprow bot commented Jul 8, 2024

codecov bot commented Jul 8, 2024 •

edited

Loading

ekexium Jul 10, 2024

lcwangchao Jul 10, 2024 •

edited

Loading

lcwangchao Jul 10, 2024

jackysp Jul 15, 2024 •

edited

Loading

jackysp Jul 15, 2024 •

edited

Loading

jackysp Jul 15, 2024 •

edited

Loading

lcwangchao Jul 16, 2024

ekexium left a comment •

edited

Loading

lcwangchao commented Jul 10, 2024

ekexium commented Jul 10, 2024 •

edited

Loading

lcwangchao commented Jul 10, 2024

tiprow bot commented Jul 10, 2024

lcwangchao commented Jul 10, 2024

tiprow bot commented Jul 10, 2024

lcwangchao commented Jul 10, 2024

tiprow bot commented Jul 10, 2024

lcwangchao commented Jul 10, 2024

tiprow bot commented Jul 10, 2024

lcwangchao commented Jul 10, 2024

tiprow bot commented Jul 10, 2024

ekexium left a comment

lcwangchao commented Jul 12, 2024 •

edited

Loading

tiancaiamao left a comment

cfzjywxk left a comment

ti-chi-bot bot commented Jul 16, 2024

ti-chi-bot bot commented Jul 16, 2024

ti-chi-bot bot commented Jul 16, 2024

	if !sessVars.InTxn() {
	savePresumeKeyNotExist := sessVars.PresumeKeyNotExists
	if !sessVars.ConstraintCheckInPlace && sessVars.TxnCtx.IsPessimistic {
	sessVars.PresumeKeyNotExists = true
	}
	err = t.rebuildIndices(sctx, txn, h, touched, oldData, newData, table.WithCtx(ctx))
	sessVars.PresumeKeyNotExists = savePresumeKeyNotExist
	if err != nil {
	return err
	}
	} else {

executor: fix update ignore still report dup-key error in statement retry #54495

executor: fix update ignore still report dup-key error in statement retry #54495

Conversation

lcwangchao commented Jul 8, 2024 • edited Loading

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

tiprow bot commented Jul 8, 2024

codecov bot commented Jul 8, 2024 • edited Loading

Codecov Report

ekexium Jul 10, 2024

Choose a reason for hiding this comment

lcwangchao Jul 10, 2024 • edited Loading

Choose a reason for hiding this comment

lcwangchao Jul 10, 2024

Choose a reason for hiding this comment

jackysp Jul 15, 2024 • edited Loading

Choose a reason for hiding this comment

jackysp Jul 15, 2024 • edited Loading

Choose a reason for hiding this comment

jackysp Jul 15, 2024 • edited Loading

Choose a reason for hiding this comment

lcwangchao Jul 16, 2024

Choose a reason for hiding this comment

ekexium left a comment • edited Loading

Choose a reason for hiding this comment

lcwangchao commented Jul 10, 2024

ekexium commented Jul 10, 2024 • edited Loading

lcwangchao commented Jul 10, 2024

tiprow bot commented Jul 10, 2024

lcwangchao commented Jul 10, 2024

tiprow bot commented Jul 10, 2024

lcwangchao commented Jul 10, 2024

tiprow bot commented Jul 10, 2024

lcwangchao commented Jul 10, 2024

tiprow bot commented Jul 10, 2024

lcwangchao commented Jul 10, 2024

tiprow bot commented Jul 10, 2024

ekexium left a comment

Choose a reason for hiding this comment

lcwangchao commented Jul 12, 2024 • edited Loading

tiancaiamao left a comment

Choose a reason for hiding this comment

cfzjywxk left a comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Jul 16, 2024

ti-chi-bot bot commented Jul 16, 2024

ti-chi-bot bot commented Jul 16, 2024

[LGTM Timeline notifier]

lcwangchao commented Jul 8, 2024 •

edited

Loading

codecov bot commented Jul 8, 2024 •

edited

Loading

lcwangchao Jul 10, 2024 •

edited

Loading

jackysp Jul 15, 2024 •

edited

Loading

jackysp Jul 15, 2024 •

edited

Loading

jackysp Jul 15, 2024 •

edited

Loading

ekexium left a comment •

edited

Loading

ekexium commented Jul 10, 2024 •

edited

Loading

lcwangchao commented Jul 12, 2024 •

edited

Loading