table: Add option `DupKeyCheckLazy` to check duplicated key lazily #55246

lcwangchao · 2024-08-07T06:06:10Z

What problem does this PR solve?

Issue Number: ref #54397

What changed and how does it work?

Added a new option DupKeyCheckLazy for table.DupKeyMode to check duplicated keys lazily in table operations. After this, the key check mode is determined by outside code and the logic is easier to understand.
Remove DupKeyCheckDefault and add DupKeyCheckInPlace to replace it.
Adding IgnoreError to UpdateExec and InsertExec to indicate INSERT IGNORE ... or UPDATE IGNORE .... The condition e.IgnoreError will replace ec.ErrGroupLevel(errctx.ErrGroupDupKey) != errctx.LevelError which makes code more readable.
Remove LazyCheckKeyNotExists because the lazy check mode is determined by each scene now, this method is useless now. This PR also removed PresumeKeyNotExists in SessionVars.

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.

None

tiprow · 2024-08-07T06:06:32Z

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.

lcwangchao · 2024-08-07T06:20:45Z

/retest

tiprow · 2024-08-07T06:21: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.

codecov · 2024-08-07T06:35:34Z

Codecov Report

Attention: Patch coverage is 93.81443% with 6 lines in your changes missing coverage. Please review.

Project coverage is 74.1474%. Comparing base (3dfa15c) to head (b1140a3).
Report is 19 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #55246        +/-   ##
================================================
+ Coverage   72.8486%   74.1474%   +1.2987%     
================================================
  Files          1570       1573         +3     
  Lines        439518     443407      +3889     
================================================
+ Hits         320183     328775      +8592     
+ Misses        99610      94604      -5006     
- Partials      19725      20028       +303

Flag	Coverage Δ
integration	`47.0532% <82.4742%> (?)`
unit	`71.8007% <93.8144%> (-0.0872%)`	⬇️

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

Components	Coverage Δ
dumpling	`52.9567% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`46.2527% <ø> (+0.3905%)`	⬆️

lcwangchao · 2024-08-07T08:25:13Z

pkg/executor/insert.go

+ // https://github.com/pingcap/tidb/issues/54492#issuecomment-2229941881
+ // Though it is still in an insert statement, but it seems some old tests still think it should
+ // check constraints in place, see test:
+ // https://github.com/pingcap/tidb/blob/3117d3fae50bbb5dabcde7b9589f92bfbbda5dc6/pkg/executor/test/writetest/write_test.go#L419-L426


There are same corner cases here, just check the test case:

tidb/pkg/executor/test/writetest/write_test.go

Lines 419 to 426 in 3117d3f

tk.MustExec(`set tidb_constraint_check_in_place = 0;`)

tk.MustExec("replace into t values (1),(2)")

tk.MustExec("begin")

err = tk.ExecToErr("update t set i = 2 where i = 1")

require.Error(t, err)

err = tk.ExecToErr("insert into t values (1) on duplicate key update i = i + 1")

require.Error(t, err)

tk.MustExec("rollback")

And some comments:

#54492 (comment)

lcwangchao · 2024-08-07T09:00:13Z

/retest

tiprow · 2024-08-07T09:00:35Z

@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-08-07T09:34:06Z

/retest

tiprow · 2024-08-07T09:34: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.

pkg/executor/insert.go

cfzjywxk · 2024-08-08T02:56:31Z

pkg/executor/insert.go

+ // https://github.com/pingcap/tidb/issues/54492#issuecomment-2229941881
+ // Though it is still in an insert statement, but it seems some old tests still think it should
+ // check constraints in place, see test:
+ // https://github.com/pingcap/tidb/blob/3117d3fae50bbb5dabcde7b9589f92bfbbda5dc6/pkg/executor/test/writetest/write_test.go#L419-L426


Should the tidb_constraint_check_in_place_pessimistic also be explained here?

tidb_constraint_check_in_place_pessimistic is not considerate yet. It will be handled in another PR to introduce a new option like PessimisticLazyCheckMode which only works for pessimistic txn and DupKeyCheckMode is DupKeyCheckLazy

cfzjywxk · 2024-08-08T02:57:32Z

pkg/executor/insert.go

@@ -236,14 +237,42 @@ func (e *InsertExec) batchUpdateDupRows(ctx context.Context, newRows [][]types.D
 e.stats.Prefetch += time.Since(prefetchStart)
 }

+ // If the current row has some conflicts, the operation will be changed to update.


Is it necessary to wrap L240-L260 to a txn util function like optimizeDupKeyCheckForNormalInsert? And duplications from L284 to L297 in update.go could be avoided.

Co-authored-by: cfzjywxk <cfzjywxk@gmail.com>

cfzjywxk

LGTM

cfzjywxk · 2024-08-08T08:37:46Z

pkg/sessionctx/variable/session.go

@@ -1265,9 +1264,6 @@ type SessionVars struct {
 // EnableGlobalIndex indicates whether we could create an global index on a partition table or not.
 EnableGlobalIndex bool

- // PresumeKeyNotExists indicates lazy existence checking is enabled.
- PresumeKeyNotExists bool


Good to see the complexity is reduced by removing the unncessary states.

cfzjywxk · 2024-08-08T08:38:24Z

@crazycs520 @ekexium
PTAL if you have time.

easonn7 · 2024-08-08T09:16:05Z

/approve

qw4990

LGTM for the optimizer part

ti-chi-bot · 2024-08-08T09:17:32Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cfzjywxk, easonn7, qw4990

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,qw4990]
~~pkg/planner/OWNERS~~ [qw4990]
~~pkg/sessionctx/variable/OWNERS~~ [easonn7]
~~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-08-08T09:17:36Z

[LGTM Timeline notifier]

Timeline:

2024-08-08 08:38:17.989054514 +0000 UTC m=+514027.856153601: ☑️ agreed by cfzjywxk.
2024-08-08 09:17:35.021049449 +0000 UTC m=+516384.888148537: ☑️ agreed by qw4990.

tiprow · 2024-08-08T09:57:57Z

@lcwangchao: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
fast_test_tiprow	`b1140a3`	link	true	`/test fast_test_tiprow`

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. sig/planner SIG: Planner size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 7, 2024

lcwangchao force-pushed the dupkeychecklazy branch 3 times, most recently from a93279b to b0381d4 Compare August 7, 2024 06:30

lcwangchao force-pushed the dupkeychecklazy branch from b0381d4 to efc9de6 Compare August 7, 2024 07:21

table: Add option DupKeyCheckLazy to check duplicated key lazily

bfb479d

lcwangchao force-pushed the dupkeychecklazy branch from efc9de6 to bfb479d Compare August 7, 2024 08:24

lcwangchao commented Aug 7, 2024

View reviewed changes

lcwangchao requested a review from cfzjywxk August 7, 2024 09:00

cfzjywxk requested review from crazycs520 and ekexium August 8, 2024 02:35

cfzjywxk reviewed Aug 8, 2024

View reviewed changes

lcwangchao and others added 2 commits August 8, 2024 12:07

Update pkg/executor/insert.go

e15eac2

Co-authored-by: cfzjywxk <cfzjywxk@gmail.com>

address comments

b1140a3

lcwangchao force-pushed the dupkeychecklazy branch from d67f81b to b1140a3 Compare August 8, 2024 06:39

cfzjywxk approved these changes Aug 8, 2024

View reviewed changes

ti-chi-bot bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Aug 8, 2024

qw4990 approved these changes Aug 8, 2024

View reviewed changes

ti-chi-bot bot added approved lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Aug 8, 2024

ti-chi-bot bot merged commit d4c26ca into pingcap:master Aug 8, 2024
22 of 23 checks passed

lcwangchao deleted the dupkeychecklazy branch August 8, 2024 10:16

lcwangchao mentioned this pull request Aug 9, 2024

Remove GetSessionVars in table.MutateContext #54397

Closed

ekexium mentioned this pull request Aug 22, 2024

table: specify DupKeyCheckLazy in BenchmarkAddRecordInPipelinedDML #55597

Merged

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

table: Add option `DupKeyCheckLazy` to check duplicated key lazily #55246

table: Add option `DupKeyCheckLazy` to check duplicated key lazily #55246

lcwangchao commented Aug 7, 2024 •

edited

Loading

tiprow bot commented Aug 7, 2024

lcwangchao commented Aug 7, 2024

tiprow bot commented Aug 7, 2024

codecov bot commented Aug 7, 2024 •

edited

Loading

lcwangchao Aug 7, 2024 •

edited

Loading

lcwangchao commented Aug 7, 2024

tiprow bot commented Aug 7, 2024

lcwangchao commented Aug 7, 2024

tiprow bot commented Aug 7, 2024

cfzjywxk Aug 8, 2024

lcwangchao Aug 8, 2024

cfzjywxk Aug 8, 2024

cfzjywxk left a comment

cfzjywxk Aug 8, 2024

cfzjywxk commented Aug 8, 2024 •

edited

Loading

easonn7 commented Aug 8, 2024

qw4990 left a comment

ti-chi-bot bot commented Aug 8, 2024

ti-chi-bot bot commented Aug 8, 2024

tiprow bot commented Aug 8, 2024

	tk.MustExec(`set tidb_constraint_check_in_place = 0;`)
	tk.MustExec("replace into t values (1),(2)")
	tk.MustExec("begin")
	err = tk.ExecToErr("update t set i = 2 where i = 1")
	require.Error(t, err)
	err = tk.ExecToErr("insert into t values (1) on duplicate key update i = i + 1")
	require.Error(t, err)
	tk.MustExec("rollback")

table: Add option DupKeyCheckLazy to check duplicated key lazily #55246

table: Add option DupKeyCheckLazy to check duplicated key lazily #55246

Conversation

lcwangchao commented Aug 7, 2024 • edited Loading

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

tiprow bot commented Aug 7, 2024

lcwangchao commented Aug 7, 2024

tiprow bot commented Aug 7, 2024

codecov bot commented Aug 7, 2024 • edited Loading

Codecov Report

lcwangchao Aug 7, 2024 • edited Loading

Choose a reason for hiding this comment

lcwangchao commented Aug 7, 2024

tiprow bot commented Aug 7, 2024

lcwangchao commented Aug 7, 2024

tiprow bot commented Aug 7, 2024

cfzjywxk Aug 8, 2024

Choose a reason for hiding this comment

lcwangchao Aug 8, 2024

Choose a reason for hiding this comment

cfzjywxk Aug 8, 2024

Choose a reason for hiding this comment

cfzjywxk left a comment

Choose a reason for hiding this comment

cfzjywxk Aug 8, 2024

Choose a reason for hiding this comment

cfzjywxk commented Aug 8, 2024 • edited Loading

easonn7 commented Aug 8, 2024

qw4990 left a comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Aug 8, 2024

ti-chi-bot bot commented Aug 8, 2024

[LGTM Timeline notifier]

tiprow bot commented Aug 8, 2024

table: Add option `DupKeyCheckLazy` to check duplicated key lazily #55246

table: Add option `DupKeyCheckLazy` to check duplicated key lazily #55246

lcwangchao commented Aug 7, 2024 •

edited

Loading

codecov bot commented Aug 7, 2024 •

edited

Loading

lcwangchao Aug 7, 2024 •

edited

Loading

cfzjywxk commented Aug 8, 2024 •

edited

Loading