ddl notifier: use pessimistic txn and fix updating memory state too early | tidb-test=pr/2477 #59157

lance6716 · 2025-01-23T09:28:57Z

What problem does this PR solve?

Issue Number: close #59055

Problem Summary:

What changed and how does it work?

in pessimistic txn, errors can be found earlier, such as when execute the SQL. So handler can see the error and process it. Otherwise the error is reported when COMMIT, and in current handler interface it can't be proccessed

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

Signed-off-by: lance6716 <[email protected]>

tiprow · 2025-01-23T09:29:16Z

Hi @lance6716. 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.

Copilot

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

Signed-off-by: lance6716 <[email protected]>

codecov · 2025-01-23T09:55:50Z

Codecov Report

Attention: Patch coverage is 88.57143% with 4 lines in your changes missing coverage. Please review.

Project coverage is 74.9913%. Comparing base (444c38f) to head (e2cd37f).
Report is 9 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #59157        +/-   ##
================================================
+ Coverage   73.0574%   74.9913%   +1.9338%     
================================================
  Files          1689       1736        +47     
  Lines        466990     475883      +8893     
================================================
+ Hits         341171     356871     +15700     
+ Misses       104844      96590      -8254     
- Partials      20975      22422      +1447

Flag	Coverage Δ
integration	`48.9161% <62.8571%> (?)`
unit	`72.3803% <88.5714%> (+0.1222%)`	⬆️

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

Components	Coverage Δ
dumpling	`52.6910% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`61.1639% <ø> (+15.7552%)`	⬆️

Signed-off-by: lance6716 <[email protected]>

D3Hunter · 2025-01-24T06:38:32Z

pkg/ddl/notifier/subscribe.go


-	if err = session.Begin(ctx); err != nil {
+	if err = session.BeginPessimistic(ctx); err != nil {


what's the target issue of using pessimistic txn ?

in pessimistic txn, errors can be found earlier, such as when execute the SQL. So handler can see the error and process it. Otherwise the error is reported when COMMIT, and in current handler interface it can't be proccessed

in pessimistic txn, errors can be found earlier

for update, if there is no check about affected rows, there is no error actually in pessimistic txn

The original problem is handler uses the txn to insert some duplicate rows. In optimistic txn, there's no error after execute the INSERT. Using pessimistic txn handler can see "duplicate entry" error. #58980 is related

I also checked the affected rows in bef13bf . It's revealed by old UT after switching to pessimistic txn.

lance6716 · 2025-01-24T08:08:11Z

/retest

tiprow · 2025-01-24T08:08:34Z

@lance6716: 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.

Signed-off-by: lance6716 <[email protected]>

0xPoe

I will help test this patch along with my changes in our test infrastructure.

So let’s hold off on this PR until I finish the testing.

/hold

pkg/ddl/notifier/store.go

ti-chi-bot · 2025-01-26T04:19:45Z

[LGTM Timeline notifier]

Timeline:

2025-01-24 08:07:52.437777524 +0000 UTC m=+427399.768696928: ☑️ agreed by D3Hunter.
2025-01-26 04:19:44.956880997 +0000 UTC m=+586512.287800400: ☑️ agreed by Rustin170506.

lance6716 · 2025-01-26T06:50:02Z

/retest

tiprow · 2025-01-26T06:50:23Z

@lance6716: 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.

0xPoe · 2025-01-26T08:00:21Z

Tested in the test infra with my patch: #58980

We don't meet the duplicate key error anymore:

Also no records in the system table:

mysql> select * from mysql.tidb_ddl_notifier;
Empty set (0.05 sec)

0xPoe · 2025-01-26T08:09:13Z

/retest

0xPoe · 2025-02-06T13:07:03Z

/unhold

Tested in #59157 (comment)

lance6716 · 2025-02-06T13:31:18Z

/retest

tiprow · 2025-02-06T13:31:43Z

@lance6716: 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.

0xPoe · 2025-02-07T06:49:35Z

/retest

…n analyze tests Signed-off-by: Rustin170506 <[email protected]>

ti-chi-bot · 2025-02-07T08:02:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: D3Hunter, Rustin170506

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~~ [D3Hunter,Rustin170506]
~~pkg/ddl/OWNERS~~ [D3Hunter]

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

0xPoe · 2025-02-07T08:08:03Z

/hold

…n analyze tests Signed-off-by: Rustin170506 <[email protected]>

Signed-off-by: Rustin170506 <[email protected]>

0xPoe · 2025-02-07T10:41:22Z

/unhold

The reasons behind these commits are:

c7a8985 (#59157) If we handle the DDL events correctly, the analyze rate will be determined by the stats' row count instead of the default value. However, since DDL handling is asynchronous, it may sometimes succeed before the analysis and sometimes after.
e2cd37f (#59157) If we don't handle the DDL event, the analyze process will not analyze the val column because it is neither a predicate column nor an index column, causing the status to be missing(nil). However, if we do handle the DDL event, we will know that this column hasn't been analyzed yet, meaning it is uninitialized. There is still a race condition between the analyze process and DDL event handling.

Signed-off-by: ti-chi-bot <[email protected]>

ti-chi-bot · 2025-02-07T14:09:36Z

In response to a cherrypick label: new pull request created to branch release-8.5: #59330.
But this PR has conflicts, please resolve them!

…arly (#59157) (#59330) close #59055

…arly (pingcap#59157) close pingcap#59055

ddl: fix memory state is wrongly updated before persistence

412f4ef

Signed-off-by: lance6716 <[email protected]>

ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 23, 2025

lance6716 changed the title ~~ddl notifier: use pessimistic txn and fixu updating memory state too early~~ ddl notifier: use pessimistic txn and fix updating memory state too early Jan 23, 2025

lance6716 requested a review from Copilot January 23, 2025 09:31

Copilot AI reviewed Jan 23, 2025

View reviewed changes

fix bazel

d0839aa

Signed-off-by: lance6716 <[email protected]>

lance6716 requested a review from fzzf678 January 23, 2025 09:57

revert mdl change

d89ecf4

Signed-off-by: lance6716 <[email protected]>

lance6716 requested a review from D3Hunter January 24, 2025 06:28

D3Hunter reviewed Jan 24, 2025

View reviewed changes

lance6716 requested a review from winoros January 24, 2025 08:01

D3Hunter approved these changes Jan 24, 2025

View reviewed changes

ti-chi-bot bot added approved needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Jan 24, 2025

fix CI

bef13bf

Signed-off-by: lance6716 <[email protected]>

0xPoe approved these changes Jan 26, 2025

View reviewed changes

pkg/ddl/notifier/store.go Show resolved Hide resolved

ti-chi-bot bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Jan 26, 2025

ti-chi-bot bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 6, 2025

Merge branch 'master' of github.com:pingcap/tidb into fix59055

de30719

0xPoe force-pushed the fix59055 branch from c116984 to de30719 Compare February 7, 2025 07:48

executor: filter warnings to show only 'Warning' and 'Error' levels i…

c600f88

…n analyze tests Signed-off-by: Rustin170506 <[email protected]>

ti-chi-bot bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 7, 2025

0xPoe added 2 commits February 7, 2025 16:09

executor: filter warnings to show only 'Warning' and 'Error' levels i…

c7a8985

…n analyze tests Signed-off-by: Rustin170506 <[email protected]>

executor: update analyze table command to include all columns

e2cd37f

Signed-off-by: Rustin170506 <[email protected]>

ti-chi-bot bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 7, 2025

ti-chi-bot bot merged commit 0fdb325 into pingcap:master Feb 7, 2025
25 checks passed

0xPoe added the needs-cherry-pick-release-8.5 Should cherry pick this PR to release-8.5 branch. label Feb 7, 2025

ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request Feb 7, 2025

This is an automated cherry-pick of pingcap#59157

9d98999

Signed-off-by: ti-chi-bot <[email protected]>

ti-chi-bot mentioned this pull request Feb 7, 2025

ddl notifier: use pessimistic txn and fix updating memory state too early (#59157) #59330

Merged

13 tasks

ti-chi-bot bot pushed a commit that referenced this pull request Feb 11, 2025

ddl notifier: use pessimistic txn and fix updating memory state too e…

6f761e2

…arly (#59157) (#59330) close #59055

0xPoe changed the title ~~ddl notifier: use pessimistic txn and fix updating memory state too early~~ ddl notifier: use pessimistic txn and fix updating memory state too early | tidb-test=pr/2477 Feb 11, 2025

zeminzhou pushed a commit to zeminzhou/tidb that referenced this pull request May 6, 2025

ddl notifier: use pessimistic txn and fix updating memory state too e…

ac6e68a

…arly (pingcap#59157) close pingcap#59055


		if err = session.Begin(ctx); err != nil {
		if err = session.BeginPessimistic(ctx); err != nil {

ddl notifier: use pessimistic txn and fix updating memory state too early | tidb-test=pr/2477 #59157

ddl notifier: use pessimistic txn and fix updating memory state too early | tidb-test=pr/2477 #59157

Uh oh!

Conversation

lance6716 commented Jan 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

Uh oh!

tiprow bot commented Jan 23, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jan 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

D3Hunter Jan 24, 2025

Choose a reason for hiding this comment

Uh oh!

lance6716 Jan 24, 2025

Choose a reason for hiding this comment

Uh oh!

D3Hunter Jan 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lance6716 Jan 24, 2025

Choose a reason for hiding this comment

Uh oh!

lance6716 Jan 24, 2025

Choose a reason for hiding this comment

Uh oh!

lance6716 commented Jan 24, 2025

Uh oh!

tiprow bot commented Jan 24, 2025

Uh oh!

0xPoe left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ti-chi-bot bot commented Jan 26, 2025

[LGTM Timeline notifier]

Uh oh!

lance6716 commented Jan 26, 2025

Uh oh!

tiprow bot commented Jan 26, 2025

Uh oh!

0xPoe commented Jan 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0xPoe commented Jan 26, 2025

Uh oh!

0xPoe commented Feb 6, 2025

Uh oh!

lance6716 commented Feb 6, 2025

Uh oh!

tiprow bot commented Feb 6, 2025

Uh oh!

0xPoe commented Feb 7, 2025

Uh oh!

ti-chi-bot bot commented Feb 7, 2025

Uh oh!

0xPoe commented Feb 7, 2025

Uh oh!

0xPoe commented Feb 7, 2025

Uh oh!

Uh oh!

ti-chi-bot commented Feb 7, 2025

Uh oh!

Uh oh!

lance6716 commented Jan 23, 2025 •

edited

Loading

codecov bot commented Jan 23, 2025 •

edited

Loading

D3Hunter Jan 24, 2025 •

edited

Loading

0xPoe commented Jan 26, 2025 •

edited

Loading