Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

session, table: fix listColumnPartition data race #33199

Merged
merged 22 commits into from
Apr 27, 2022

Conversation

zimulala
Copy link
Contributor

@zimulala zimulala commented Mar 17, 2022

What problem does this PR solve?

Issue Number: close #33030

Problem Summary:
ForListColumnPruning has a lot of fields, if we use the lock to fix data race, it will make the code dirty.
So I change the way to fix #32416.

#32416 :
In step 1

In the bootstrap phase, new collation is true
After bootstrap ends, new collation is false.

In step 2

a. In the bootstrap phase, new collation is true. ForListColumnPruning builds the hashMap(generate the partition hash key, by calling EncodeKey) in TableFromMeta

b. After bootstrap ends, new collation is false. The generated partition hash key(by calling EncodeKey) could not be found in the hashMap.

What is changed and how it works?

This way I will rebuild hashMap with new collation is false after step 2.a. Then we can find the key in step 2.b.

Check List

Tests

  • Unit test
    Tests already exist: TestLocatePartition

  • Integration test

  • Manual test (add detailed scripts or steps below)

  1. Set the new_collations_enabled_on_first_bootstrap item to false and run the SQL statements as follows:
set @@session.tidb_enable_list_partition = ON;
set @@tidb_partition_prune_mode = 'dynamic';
drop table if exists github_events;
CREATE TABLE `github_events` (
`id` bigint(20) DEFAULT NULL,
`type` varchar(255) COLLATE utf8mb4_unicode_ci DEFAULT NULL
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci
PARTITION BY LIST COLUMNS(`type`)
(PARTITION `push_event` VALUES IN ("PushEvent"),
PARTITION `watch_event` VALUES IN ("WatchEvent")
);
  1. Restart TiDB with new_collations_enabled_on_first_bootstrap = false and execute the following statements:
tidb> set @@session.tidb_enable_list_partition = ON;
Query OK, 0 rows affected (0.00 sec)

tidb> set @@tidb_partition_prune_mode = 'dynamic';
Query OK, 0 rows affected (0.00 sec)

tidb> desc select id,type from github_events  where  type = 'WatchEvent';
+-------------------------+----------+-----------+-----------------------+-------------------------------------------+
| id                      | estRows  | task      | access object         | operator info                             |
+-------------------------+----------+-----------+-----------------------+-------------------------------------------+
| TableReader_7           | 10.00    | root      | partition:watch_event | data:Selection_6                          |
| └─Selection_6           | 10.00    | cop[tikv] |                       | eq(test.github_events.type, "WatchEvent") |
|   └─TableFullScan_5     | 10000.00 | cop[tikv] | table:github_events   | keep order:false, stats:pseudo            |
+-------------------------+----------+-----------+-----------------------+-------------------------------------------+
3 rows in set (0.00 sec)
  • No code

Side effects

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

Documentation

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Changes MySQL compatibility

Release note

Fix the data race when using the list column partition.

@zimulala zimulala added type/bugfix This PR fixes a bug. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Mar 17, 2022
@zimulala zimulala requested a review from a team as a code owner March 17, 2022 06:21
@ti-chi-bot
Copy link
Member

ti-chi-bot commented Mar 17, 2022

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • crazycs520
  • wjhuang2016

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.

@ti-chi-bot ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/needs-linked-issue size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Mar 17, 2022
@sre-bot
Copy link
Contributor

sre-bot commented Mar 17, 2022

@zimulala
Copy link
Contributor Author

/run-unit-test

@ti-chi-bot ti-chi-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Mar 17, 2022
@zimulala zimulala added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 17, 2022
@zimulala
Copy link
Contributor Author

/run-unit-test

1 similar comment
@zimulala
Copy link
Contributor Author

/run-unit-test

@ti-chi-bot ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 23, 2022
@ti-chi-bot ti-chi-bot added needs-cherry-pick-6.0 and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Apr 1, 2022
@zimulala zimulala removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 1, 2022
@zimulala
Copy link
Contributor Author

zimulala commented Apr 1, 2022

@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Apr 1, 2022
@tiancaiamao
Copy link
Contributor

PTAL @wjhuang2016

@Defined2014
Copy link
Contributor

/hold

@ti-chi-bot ti-chi-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 21, 2022
@Defined2014
Copy link
Contributor

@morgo @bb7133 @cfzjywxk
PTAL

@hawkingrei
Copy link
Member

/unhold

@ti-chi-bot ti-chi-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 21, 2022
@wjhuang2016
Copy link
Member

/hold

@ti-chi-bot ti-chi-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 21, 2022
@hawkingrei hawkingrei requested a review from XuHuaiyu April 21, 2022 15:29
continue
}

pe, err := t.(partitionExpr).PartitionExpr()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's a good style, IMO a better way to do it is something like:

pt := t.(table.PartitionedTable); err != nil {
    panic(...)
}
pe, err := t.PartitionExpr()

Which is more clear. However, some extra changes are required and it might not be a good idea to make it in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, it is already confirmed that it is a partitioned table, so theoretically this(t.(partitionExpr)) should not report an error.

@zimulala
Copy link
Contributor Author

PTAL @bb7133

Copy link
Member

@bb7133 bb7133 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@bb7133
Copy link
Member

bb7133 commented Apr 26, 2022

/merge

@zimulala
Copy link
Contributor Author

/merge

@zimulala zimulala removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 27, 2022
@ti-chi-bot ti-chi-bot merged commit 1a19f95 into pingcap:master Apr 27, 2022
ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Apr 27, 2022
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot
Copy link
Contributor

cherry pick to release-6.0 in PR #34285

@zimulala zimulala deleted the zimuxia/fix-data-race branch April 27, 2022 07:43
@sre-bot
Copy link
Contributor

sre-bot commented Apr 27, 2022

TiDB MergeCI notify

✅ Well Done! New fixed [1] after this pr merged.

CI Name Result Duration Compare with Parent commit
idc-jenkins-ci-tidb/common-test 🔴 failed 2, success 10, total 12 6 min 33 sec Existing failure
idc-jenkins-ci-tidb/integration-compatibility-test ✅ all 1 tests passed 4 min 3 sec Fixed
idc-jenkins-ci-tidb/integration-common-test 🟢 all 11 tests passed 18 min Existing passed
idc-jenkins-ci-tidb/integration-ddl-test 🟢 all 6 tests passed 8 min 44 sec Existing passed
idc-jenkins-ci-tidb/sqllogic-test-1 🟢 all 26 tests passed 6 min 46 sec Existing passed
idc-jenkins-ci-tidb/sqllogic-test-2 🟢 all 28 tests passed 6 min 25 sec Existing passed
idc-jenkins-ci-tidb/tics-test 🟢 all 1 tests passed 6 min 3 sec Existing passed
idc-jenkins-ci-tidb/mybatis-test 🟢 all 1 tests passed 3 min 54 sec Existing passed
idc-jenkins-ci-tidb/plugin-test 🟢 build success, plugin test success 4min Existing passed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. type/bugfix This PR fixes a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DATA RACE in the TestLocatePartition v5.4.0 list partition can't work after restarting tidb
10 participants