ddl: update the reorg range after delay for async txn #38195

tangenta · 2022-09-27T07:38:43Z

What problem does this PR solve?

Issue Number: close #38165

Problem Summary:

Previously, reorgInfo is initialized in getReorgInfo(), which happens before delayForAsyncCommit(). However, the reorg range may change after the delay(for example, an async commit transaction inserts a record). As a result, the backfill transaction does not add the corresponding index record.

What is changed and how it works?

This PR moves the reorg info range initialization after delayForAsyncCommit. It makes sure no async transactions are active.

Check List

Tests

Unit test
Integration test

Manual test (add detailed scripts or steps below)
It is not easy to write the unit test because client-go is involved. I follow the steps described in issue, the problem does not occur anymore.

["[ddl] split table range from PD"] [physicalTableID=169] [startHandle=1] [endHandle=1]

-- session 1
mysql> alter table t add index idx(a);
Query OK, 0 rows affected (3.86 sec)
mysql> admin check table t;
Query OK, 0 rows affected (0.00 sec)
-- session 2
mysql> insert into t values (1);
Query OK, 1 row affected (2.01 sec)

No code

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 the data inconsistent issue after creating index.

ti-chi-bot · 2022-09-27T07:38:45Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

xiongjiwei
zimulala

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

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

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

ddl/reorg.go

zimulala · 2022-09-28T01:50:06Z

ddl/reorg.go

+	if reorgInfo.mergingTmpIdx {
+		start, end = tablecodec.GetTableIndexKeyRange(pid, tablecodec.TempIndexPrefix|elem.ID)
+	} else {
+		currentVer, err := getValidCurrentVersion(dCtx.store)


It seems that the currentVer value is changed. Before this PR, the version is got when region.first is true. Maybe we can use job.SnapshotVer?

If we use job.SnapshotVer, we cannot get the latest data.

Indeed, it went back.
But if not, does it feel like whether it is "first" doesn't make much of a difference?

I don't think the reorgInfo.first is necessary right from the start... Let me refactor these code later.

OK~, I mean that if it isn't necessary, we can do #38195 (comment) .

zimulala

LGTM

xiongjiwei · 2022-09-29T02:57:53Z

ddl/stat_test.go

@@ -89,7 +89,7 @@ func TestDDLStatsInfo(t *testing.T) {
 			varMap, err := d.Stats(nil)
 			wg.Done()
 			require.NoError(t, err)
-			require.Equal(t, "1", varMap[ddlJobReorgHandle])
+			require.Equal(t, "", varMap[ddlJobReorgHandle])


why the test is changed

Because the channel ddl.TestCheckWorkerNumCh sends, the transaction that updating ddl_job_reorg_handle is not committed.

xiongjiwei · 2022-09-29T03:20:14Z

/merge

ti-chi-bot · 2022-09-29T03:20:18Z

This pull request has been accepted and is ready to merge.

Commit hash: 333e389c93b956ef54f0344c23dd9c64aa26b091

tangenta · 2022-09-29T09:01:41Z

/hold because add index test is timed out.

hawkingrei · 2022-09-30T01:28:50Z

In the Cloud CI，check_2 only have 8 core cpu to run this test case. it maybe an reason。

xiongjiwei · 2022-10-08T07:18:01Z

/merge

ti-chi-bot · 2022-10-08T07:18:05Z

This pull request has been accepted and is ready to merge.

Commit hash: b4fadfd

hawkingrei · 2022-10-08T07:27:38Z

ddl/reorg.go

-		// get the current version for reorganization if we don't have
+		if d.lease > 0 { // Only delay when it's not in test.
+			delayForAsyncCommit()
+		}
 		ver, err := getValidCurrentVersion(d.store)


There are the same codes. I think it can be merged into a single function.

info.first = true ... ver, err := getValidCurrentVersion(d.store)

Frankly speaking, getReorgInfoFromPartitions should reuse getReorgInfo since they are quite similar. Please note that getReorgInfoFromPartitions is only used by the global index feature, which is experimental, unstable, and lacks maintenance.

We need to discuss how to handle this piece of code properly.

OK, it may be improved after GA the global index feature.

tangenta · 2022-10-08T08:20:41Z

/unhold
/merge

ti-chi-bot · 2022-10-08T08:44:30Z

In response to a cherrypick label: cannot checkout 5.3: error checking out 5.3: exit status 1. output: error: pathspec '5.3' did not match any file(s) known to git

ti-chi-bot · 2022-10-08T08:45:06Z

In response to a cherrypick label: cannot checkout 5.4: error checking out 5.4: exit status 1. output: error: pathspec '5.4' did not match any file(s) known to git

ti-chi-bot · 2022-10-08T08:45:41Z

In response to a cherrypick label: cannot checkout 6.1: error checking out 6.1: exit status 1. output: error: pathspec '6.1' did not match any file(s) known to git

ti-chi-bot · 2022-10-08T08:46:16Z

In response to a cherrypick label: cannot checkout 6.3: error checking out 6.3: exit status 1. output: error: pathspec '6.3' did not match any file(s) known to git

sre-bot · 2022-10-08T09:21:05Z

TiDB MergeCI notify

🔴 Bad News! New failing [1] after this pr merged.
These new failed integration tests seem to be caused by the current PR, please try to fix these new failed integration tests, thanks!

CI Name	Result	Duration	Compare with Parent commit
idc-jenkins-ci-tidb/integration-ddl-test	🟥 failed 1, success 5, total 6	30 min	New failing
idc-jenkins-ci-tidb/common-test	🔴 failed 1, success 10, total 11	9 min 22 sec	Existing failure
idc-jenkins-ci/integration-cdc-test	🟢 all 37 tests passed	25 min	Existing passed
idc-jenkins-ci-tidb/integration-common-test	🟢 all 17 tests passed	11 min	Existing passed
idc-jenkins-ci-tidb/tics-test	🟢 all 1 tests passed	5 min 23 sec	Existing passed
idc-jenkins-ci-tidb/sqllogic-test-2	🟢 all 28 tests passed	4 min 42 sec	Existing passed
idc-jenkins-ci-tidb/sqllogic-test-1	🟢 all 26 tests passed	4 min 12 sec	Existing passed
idc-jenkins-ci-tidb/mybatis-test	🟢 all 1 tests passed	3 min 10 sec	Existing passed
idc-jenkins-ci-tidb/integration-compatibility-test	🟢 all 1 tests passed	2 min 52 sec	Existing passed
idc-jenkins-ci-tidb/plugin-test	🟢 build success, plugin test success	4min	Existing passed

zimulala reviewed Sep 27, 2022

View reviewed changes

ddl/reorg.go Outdated Show resolved Hide resolved

zimulala reviewed Sep 28, 2022

View reviewed changes

zimulala approved these changes Sep 28, 2022

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Sep 28, 2022

xiongjiwei reviewed Sep 29, 2022

View reviewed changes

xiongjiwei approved these changes Sep 29, 2022

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Sep 29, 2022

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Sep 29, 2022

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

ddl: update the reorg range after delay for async txn

b4fadfd

tangenta force-pushed the ddl-reorg-range branch from c1e99f4 to b4fadfd Compare September 30, 2022 07:35

ti-chi-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed status/can-merge Indicates a PR has been approved by a committer. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 30, 2022

xiongjiwei approved these changes Oct 8, 2022

View reviewed changes

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Oct 8, 2022

hawkingrei reviewed Oct 8, 2022

View reviewed changes

hawkingrei approved these changes Oct 8, 2022

View reviewed changes

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

Merge branch 'master' into ddl-reorg-range

1080317

ti-chi-bot merged commit 543417b into pingcap:master Oct 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ddl: update the reorg range after delay for async txn #38195

ddl: update the reorg range after delay for async txn #38195

tangenta commented Sep 27, 2022 •

edited

ti-chi-bot commented Sep 27, 2022 •

edited

zimulala Sep 28, 2022

tangenta Sep 28, 2022

zimulala Sep 28, 2022

tangenta Sep 28, 2022

zimulala Sep 28, 2022 •

edited

zimulala left a comment

xiongjiwei Sep 29, 2022

tangenta Sep 29, 2022 •

edited

xiongjiwei commented Sep 29, 2022

ti-chi-bot commented Sep 29, 2022

tangenta commented Sep 29, 2022

hawkingrei commented Sep 30, 2022

xiongjiwei commented Oct 8, 2022

ti-chi-bot commented Oct 8, 2022

hawkingrei Oct 8, 2022

tangenta Oct 8, 2022

hawkingrei Oct 8, 2022

tangenta commented Oct 8, 2022

ti-chi-bot commented Oct 8, 2022

ti-chi-bot commented Oct 8, 2022

ti-chi-bot commented Oct 8, 2022

ti-chi-bot commented Oct 8, 2022

sre-bot commented Oct 8, 2022

ddl: update the reorg range after delay for async txn #38195

ddl: update the reorg range after delay for async txn #38195

Conversation

tangenta commented Sep 27, 2022 • edited

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Sep 27, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zimulala Sep 28, 2022 • edited

Choose a reason for hiding this comment

zimulala left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tangenta Sep 29, 2022 • edited

Choose a reason for hiding this comment

xiongjiwei commented Sep 29, 2022

ti-chi-bot commented Sep 29, 2022

tangenta commented Sep 29, 2022

hawkingrei commented Sep 30, 2022

xiongjiwei commented Oct 8, 2022

ti-chi-bot commented Oct 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tangenta commented Oct 8, 2022

ti-chi-bot commented Oct 8, 2022

ti-chi-bot commented Oct 8, 2022

ti-chi-bot commented Oct 8, 2022

ti-chi-bot commented Oct 8, 2022

sre-bot commented Oct 8, 2022

TiDB MergeCI notify

tangenta commented Sep 27, 2022 •

edited

ti-chi-bot commented Sep 27, 2022 •

edited

zimulala Sep 28, 2022 •

edited

tangenta Sep 29, 2022 •

edited