ddl: Corrected index management during REORGANIZE PARTITION #56786

mjonss · 2024-10-22T14:46:35Z

What problem does this PR solve?

Issue Number: close #56634

Problem Summary:
issue triggered by injected error, resulting in rollback and panic

Uncovered issues:

Since Global Indexes needs to be replaced (copied with new references to the new partition ids, since we cannot reference two different partitions in the same unique index entry) both indexes were used during StateWriteOnly, StateWriteReorganization and StateDeleteReorganization, which would cause duplicate entry errors when trying to insert new values during those states for Global Indexes.

What changed and how does it work?

Avoiding to use StateDeleteReorganization as index state, since during that state, the global indexes may still be read or and should also be kept up-to-date in case of rollback.
Remove indexes that are not connected to the partition, like for new partitions it should not update indexes that will be removed, those indexes will be kept up-to-date by the old partitions. So double writing, including indexes, will be on partition level, where the data matching:
- the non-touched partitions should update:
  - all original indexes
  - new indexes
  - So it needs to include all indexes!
- the old/reorganized partitions should update:
  - all original indexes
  - So it needs to skip new indexes!
- the new partitions should update:
  - all non-changed indexes
  - new indexes
  - So it needs to skip old indexes that will be replaced with new indexes (All old Global Indexes, and all indexes that will be converted to Global indexes).

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.

Fixed an issue where REORGANIZE PARTITION block inserts during some states, as well as it could fail rollback, in case of internal errors.

tiprow · 2024-10-22T14:46:55Z

Hi @mjonss. 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-10-22T15:10:42Z

Codecov Report

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

Project coverage is 74.7688%. Comparing base (9b26569) to head (a9d576e).
Report is 1 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #56786        +/-   ##
================================================
+ Coverage   72.8906%   74.7688%   +1.8782%     
================================================
  Files          1672       1717        +45     
  Lines        462275     470327      +8052     
================================================
+ Hits         336955     351658     +14703     
+ Misses       104560      96586      -7974     
- Partials      20760      22083      +1323

Flag	Coverage Δ
integration	`49.1777% <74.1573%> (?)`
unit	`72.2827% <95.5056%> (-0.0042%)`	⬇️

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

Components	Coverage Δ
dumpling	`52.7673% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`60.7583% <ø> (+15.6541%)`	⬆️

Defined2014 · 2024-10-23T03:40:52Z

/retest

tiprow · 2024-10-23T03:41:15Z

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

Defined2014

resulting in they would not continue to be updated, so in case of a rollback there might be missing entries.

Can you clarify which sql in the newly added test case covers this problem? And what's the result before this PR?

pkg/ddl/tests/partition/multi_domain_test.go

mjonss · 2024-10-23T07:46:50Z

resulting in they would not continue to be updated, so in case of a rollback there might be missing entries.

Can you clarify which sql in the newly added test case covers this problem? And what's the result before this PR?

Since the indexes was in StateDeleteReorganization, during job.SchemaState == model.StateDeleteReorganization, the indexes would not been written, see IsIndexWritable(). Which would mean that during normal DDL that is OK, but if an error would happen during StateDeleteReorganization and a rollback would happen, than changes to the indexes to be dropped would miss records written during that state.

Defined2014 · 2024-10-23T08:00:33Z

resulting in they would not continue to be updated, so in case of a rollback there might be missing entries.

Can you clarify which sql in the newly added test case covers this problem? And what's the result before this PR?

Since the indexes was in StateDeleteReorganization, during job.SchemaState == model.StateDeleteReorganization, the indexes would not been written, see IsIndexWritable(). Which would mean that during normal DDL that is OK, but if an error would happen during StateDeleteReorganization and a rollback would happen, than changes to the indexes to be dropped would miss records written during that state.

Thanks your comments. I have an another question, does it mean we could cancel reorg partition ddl job in any state? I can't found any information in IsRollbackable() function.

Still some debug and cleanup to do.

mjonss · 2024-10-24T10:41:32Z

resulting in they would not continue to be updated, so in case of a rollback there might be missing entries.

Can you clarify which sql in the newly added test case covers this problem? And what's the result before this PR?

Since the indexes was in StateDeleteReorganization, during job.SchemaState == model.StateDeleteReorganization, the indexes would not been written, see IsIndexWritable(). Which would mean that during normal DDL that is OK, but if an error would happen during StateDeleteReorganization and a rollback would happen, than changes to the indexes to be dropped would miss records written during that state.

Thanks your comments. I have an another question, does it mean we could cancel reorg partition ddl job in any state? I can't found any information in IsRollbackable() function.

Since we will continue to double write until all processing is done and the old partitions cannot be accessed any longer, it is possible to cancel the DDL Job in any state and the rollback should work. I have not tested the Cancel part, though, do you have any example of tests that I can replicate @Defined2014 ?

pkg/ddl/partition.go

mjonss · 2024-11-06T00:15:10Z

/retest

tiprow · 2024-11-06T00:15:31Z

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

tangenta · 2024-11-14T17:22:38Z

pkg/ddl/partition.go

-		if indexInfo.Unique &&
-			indexInfo.State == model.StateDeleteReorganization &&
-			tblInfo.Partition.DDLState == model.StateDeleteReorganization {
+		if indexInfo.State == model.StateWriteOnly {


Why only drop the write-only indexes? What about other states? For example, what will happen when we rollback from the "none -> delete-only" step?

There may be delete-only indexes before converting to rolling back job:

newIndex := index.Clone() newIndex.State = model.StateDeleteOnly newIndex.ID = AllocateIndexID(tblInfo)

There should only be either StatePublic or StateWriteOnly so only write-only indexes that should be removed. Also in the follow ups #57114 and #56974 (based on this PR) it will block the rollback here and here for the last state. One can see this PR a part 1 of 3, where the optimistic path works, then the failure/rollback/cleanup is fixed in part 2 and 3.

tangenta · 2024-11-14T18:18:15Z

It seems that rolling back only takes 1 schema version to complete. I am trying to figure out how it works...

mjonss · 2024-11-14T22:09:43Z

It seems that rolling back only takes 1 schema version to complete. I am trying to figure out how it works...

@tangenta it should take two, one when converting the failed DDL to rollback, and one as a second step in rollingbackLikeDrop there are extra changes in the follow up patch for error handling and rollback in #57114.

ti-chi-bot · 2024-11-15T11:05:00Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Defined2014, tangenta

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~~ [Defined2014,tangenta]
~~pkg/ddl/OWNERS~~ [tangenta]
~~pkg/meta/OWNERS~~ [tangenta]
~~pkg/table/OWNERS~~ [tangenta]

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-11-15T11:05:04Z

[LGTM Timeline notifier]

Timeline:

2024-11-04 11:00:30.919261758 +0000 UTC m=+865943.758417304: ☑️ agreed by Defined2014.
2024-11-15 11:05:02.952100369 +0000 UTC m=+613465.142969363: ☑️ agreed by tangenta.

tiprow · 2024-11-15T11:18:05Z

@mjonss: 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	`4f1c046`	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.

tangenta · 2024-11-15T11:49:58Z

pkg/table/tables/partition.go

+			// Untouched index
+			DroppingDefinitionIndices = append(DroppingDefinitionIndices, idx)
+			if pi.DDLState != model.StateDeleteReorganization {
+				// If pi.DDLState == DeleteReorg, then keep the StatePublic.


Why is the state of the index public?

To not skip it when inserting into the index, StateDeleteReorganization would work similar to StateDeleteOnly so that is why keeping the index state as StatePublic instead of StateDeleteReorganization, to keep it double written during the delete reorganization phase.

…56786) close pingcap#56634

ti-chi-bot · 2024-11-15T12:47:00Z

In response to a cherrypick label: new pull request created to branch release-8.5: #57426.

…57426) close #56634

Better index management during rollback of partitioning DDLs

6df378f

ti-chi-bot bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/needs-triage-completed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 22, 2024

mjonss added 2 commits October 22, 2024 16:53

Linting

27978e0

Merge remote-tracking branch 'pingcap/master' into reorg-part-fix-56634

8046319

ti-chi-bot bot removed the do-not-merge/needs-triage-completed label Oct 22, 2024

mjonss requested a review from Defined2014 October 22, 2024 15:44

Defined2014 reviewed Oct 23, 2024

View reviewed changes

pkg/ddl/tests/partition/multi_domain_test.go Outdated Show resolved Hide resolved

Reworked index handling in Reorganize Partition.

10f5f61

Still some debug and cleanup to do.

ti-chi-bot bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 23, 2024

mjonss added 4 commits October 24, 2024 00:37

Fixed assertion issues.

cc4f5a8

reverted non-needed changes.

ee82920

Reverted non-needed changes.

c45e3e9

Simplified index management during partitioned table initialization.

16d1628

mjonss changed the title ~~ddl: Better index management during rollback of partitioning DDLs~~ ddl: Corrected index management during REORGANIZE PARTITION Oct 24, 2024

mjonss added 2 commits October 24, 2024 17:23

Improved test and fixed more issues.

a45bba6

minor simplification and fixed failpoint leading to panic

f68c29d

ti-chi-bot bot added the sig/planner SIG: Planner label Oct 24, 2024

mjonss added 2 commits October 24, 2024 23:00

Added test for verifying cleanup.

ff3d980

Disabled test due to cleanup not fixed.

91fb5f9

ti-chi-bot bot removed the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Oct 24, 2024

Defined2014 reviewed Oct 31, 2024

View reviewed changes

pkg/ddl/partition.go Outdated Show resolved Hide resolved

mjonss added 3 commits November 1, 2024 16:29

Merge remote-tracking branch 'pingcap/master' into reorg-part-fix-56634

d4347d9

Removed limitation of global index cannot have all partitioning columns

f21b86e

review comments addressed, simplified if statement.

03d049e

Defined2014 approved these changes Nov 4, 2024

View reviewed changes

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

mjonss requested a review from tangenta November 5, 2024 12:22

Merge remote-tracking branch 'pingcap/master' into reorg-part-fix-56634

4f1c046

ti-chi-bot bot added needs-cherry-pick-release-8.5 Should cherry pick this PR to release-8.5 branch. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 8, 2024

tangenta reviewed Nov 14, 2024

View reviewed changes

tangenta approved these changes Nov 15, 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 Nov 15, 2024

tangenta reviewed Nov 15, 2024

View reviewed changes

Merge remote-tracking branch 'pingcap/master' into reorg-part-fix-56634

a9d576e

ti-chi-bot bot merged commit 938ed70 into pingcap:master Nov 15, 2024
24 checks passed

ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request Nov 15, 2024

ddl: Corrected index management during REORGANIZE PARTITION (pingcap#…

d9c801f

…56786) close pingcap#56634

ti-chi-bot mentioned this pull request Nov 15, 2024

ddl: Corrected index management during REORGANIZE PARTITION (#56786) #57426

Merged

13 tasks

mjonss deleted the reorg-part-fix-56634 branch November 15, 2024 13:32

ti-chi-bot bot pushed a commit that referenced this pull request Nov 16, 2024

ddl: Corrected index management during REORGANIZE PARTITION (#56786) (#…

6bcd156

…57426) close #56634

ddl: Corrected index management during REORGANIZE PARTITION #56786

ddl: Corrected index management during REORGANIZE PARTITION #56786

Uh oh!

Conversation

mjonss commented Oct 22, 2024 • 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 Oct 22, 2024

Uh oh!

codecov bot commented Oct 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Defined2014 commented Oct 23, 2024

Uh oh!

tiprow bot commented Oct 23, 2024

Uh oh!

Defined2014 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mjonss commented Oct 23, 2024

Uh oh!

Defined2014 commented Oct 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mjonss commented Oct 24, 2024

Uh oh!

Uh oh!

mjonss commented Nov 6, 2024

Uh oh!

tiprow bot commented Nov 6, 2024

Uh oh!

tangenta Nov 14, 2024

Choose a reason for hiding this comment

Uh oh!

mjonss Nov 14, 2024

Choose a reason for hiding this comment

Uh oh!

tangenta commented Nov 14, 2024

Uh oh!

mjonss commented Nov 14, 2024

Uh oh!

ti-chi-bot bot commented Nov 15, 2024

Uh oh!

ti-chi-bot bot commented Nov 15, 2024

[LGTM Timeline notifier]

Uh oh!

tiprow bot commented Nov 15, 2024

Uh oh!

tangenta Nov 15, 2024

Choose a reason for hiding this comment

Uh oh!

mjonss Nov 15, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ti-chi-bot commented Nov 15, 2024

Uh oh!

Uh oh!

mjonss commented Oct 22, 2024 •

edited

Loading

codecov bot commented Oct 22, 2024 •

edited

Loading

Defined2014 commented Oct 23, 2024 •

edited

Loading