txn: support multi-table join in nt-dml #39139

ekexium · 2022-11-14T11:04:47Z

Signed-off-by: ekexium eke@fastmail.com

What problem does this PR solve?

Issue Number: ref #33485

Problem Summary:

This PR adds support for multi-join in nontransactional DML.

The remaining problems

Updating the shard column should not be allowed
Using the shard column as join key should not be allowed

What is changed and how it works?

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
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.

None

ti-chi-bot · 2022-11-14T11:04:48Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

sticnarf
you06

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.

Signed-off-by: ekexium <eke@fastmail.com>

you06 · 2022-11-17T12:05:43Z

session/nontransactional.go

@@ -197,11 +205,11 @@ func checkConstraint(stmt *ast.NonTransactionalDMLStmt, se Session) error {
 	return nil
 }

-func checkTableRef(t *ast.TableRefsClause) error {
+func checkTableRef(t *ast.TableRefsClause, allowMultipleTables bool) error {


I see allowMultipleTables is always true now, is this check need to fail with multi tables in some future conditions?

Maybe. I'm not sure about Updates and Deletes. If we find them error-prone in tests we could disable multiple table joins for them.

parser/ast/dml.go

session/nontransactional.go

Signed-off-by: ekexium <eke@fastmail.com>

ekexium · 2022-11-21T06:00:06Z

The remaining problems

Updating the shard column should not be allowed
Using the shard column as join key should not be allowed

Checking these constraints is kinda complex. I'll leave them to a following PR.

sticnarf

Is there any test for selecting sharding key automatically with clustered index?

sticnarf · 2022-11-21T07:51:00Z

session/nontransactional.go

+				// if the clustered index contains multiple columns, we cannot automatically choose a column as the shard column
+				return false, nil, errors.New("Non-transactional DML, the clustered index contains multiple columns. Please specify a shard column")


Should this be a TODO? Multi-column clustered index seems doable.

Do you mean by selecting the first column? That column might be highly skewed. So I'd rather let user manually specify it.

Is there any test for selecting sharding key automatically with clustered index?

It's in TestNonTransactionalWithAutoDetectShardColumn

Single column can be highly skewed too. I don't get why multi-column is special.

I thought you didn't do it just because of implementation...

Ah, actually I don't mean using first column only. To support multiple columns, we need to scan batches of keys containing all columns in the clustered index.

ekexium · 2022-11-21T08:14:17Z

/run-all-tests

sticnarf

LGTM

Signed-off-by: ekexium <eke@fastmail.com>

ekexium · 2022-11-24T06:05:15Z

@sticnarf Could you help merge it?

sticnarf · 2022-11-24T06:06:58Z

/merge

ti-chi-bot · 2022-11-24T06:07:03Z

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

Commit hash: 8340eb0

sre-bot · 2022-11-24T07:01:36Z

TiDB MergeCI notify

🔴 Bad News! [4] CI still failing after this pr merged.
These failed integration tests don't seem to be introduced by the current PR.

CI Name	Result	Duration	Compare with Parent commit
idc-jenkins-ci-tidb/sqllogic-test-2	🔴 failed 4, success 24, total 28	22 min	Existing failure
idc-jenkins-ci/integration-cdc-test	🔴 failed 1, success 38, total 39	20 min	Existing failure
idc-jenkins-ci-tidb/mybatis-test	🔴 failed 1, success 0, total 1	11 min	Existing failure
idc-jenkins-ci-tidb/integration-ddl-test	🔴 failed 1, success 5, total 6	8 min 51 sec	Existing failure
idc-jenkins-ci-tidb/integration-common-test	🟢 all 17 tests passed	15 min	Existing passed
idc-jenkins-ci-tidb/common-test	🟢 all 11 tests passed	14 min	Existing passed
idc-jenkins-ci-tidb/sqllogic-test-1	🟢 all 26 tests passed	6 min 3 sec	Existing passed
idc-jenkins-ci-tidb/tics-test	🟢 all 1 tests passed	5 min 54 sec	Existing passed
idc-jenkins-ci-tidb/integration-compatibility-test	🟢 all 1 tests passed	2 min 44 sec	Existing passed
idc-jenkins-ci-tidb/plugin-test	🟢 build success, plugin test success	4min	Existing passed

ekexium force-pushed the nt-join branch from 00a1caf to 6139443 Compare November 14, 2022 11:05

ti-chi-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 14, 2022

ekexium force-pushed the nt-join branch from 6139443 to ce1ddc8 Compare November 16, 2022 07:04

ti-chi-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 Nov 16, 2022

feat: support multi-table join in nt-dml

8234981

Signed-off-by: ekexium <eke@fastmail.com>

ekexium force-pushed the nt-join branch from ce1ddc8 to 8234981 Compare November 16, 2022 07:06

ekexium marked this pull request as ready for review November 16, 2022 07:08

ti-chi-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 16, 2022

ekexium force-pushed the nt-join branch from 04c12ab to 972507a Compare November 16, 2022 07:25

ekexium requested a review from you06 November 16, 2022 07:39

Merge branch 'master' into nt-join

aab0e76

ekexium force-pushed the nt-join branch from 972507a to aab0e76 Compare November 16, 2022 07:56

you06 approved these changes Nov 17, 2022

View reviewed changes

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

ekexium added 2 commits November 21, 2022 13:48

Merge branch 'master' into nt-join

c52fcb2

refactor: address comments

90d0212

Signed-off-by: ekexium <eke@fastmail.com>

ekexium requested a review from sticnarf November 21, 2022 05:58

sticnarf reviewed Nov 21, 2022

View reviewed changes

sticnarf approved these changes Nov 21, 2022

View reviewed changes

ti-chi-bot removed the status/LGT1 Indicates that a PR has LGTM 1. label Nov 21, 2022

ti-chi-bot added the status/LGT2 Indicates that a PR has LGTM 2. label Nov 21, 2022

ekexium added 3 commits November 23, 2022 17:50

tiny refactor

05566ea

Signed-off-by: ekexium <eke@fastmail.com>

test: fix failure caused by relaxed constraint

35f1b12

Signed-off-by: ekexium <eke@fastmail.com>

Merge branch 'master' into nt-join

8340eb0

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

Merge branch 'master' into nt-join

c53c514

ti-chi-bot merged commit 84fa999 into pingcap:master Nov 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

txn: support multi-table join in nt-dml #39139

txn: support multi-table join in nt-dml #39139

ekexium commented Nov 14, 2022 •

edited

Loading

ti-chi-bot commented Nov 14, 2022 •

edited

Loading

you06 Nov 17, 2022

ekexium Nov 21, 2022 •

edited

Loading

ekexium commented Nov 21, 2022

sticnarf left a comment

sticnarf Nov 21, 2022

ekexium Nov 21, 2022

sticnarf Nov 21, 2022

sticnarf Nov 21, 2022

ekexium commented Nov 21, 2022

sticnarf left a comment

ekexium commented Nov 24, 2022

sticnarf commented Nov 24, 2022

ti-chi-bot commented Nov 24, 2022

sre-bot commented Nov 24, 2022

		// if the clustered index contains multiple columns, we cannot automatically choose a column as the shard column
		return false, nil, errors.New("Non-transactional DML, the clustered index contains multiple columns. Please specify a shard column")

txn: support multi-table join in nt-dml #39139

txn: support multi-table join in nt-dml #39139

Conversation

ekexium commented Nov 14, 2022 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Nov 14, 2022 • edited Loading

you06 Nov 17, 2022

Choose a reason for hiding this comment

ekexium Nov 21, 2022 • edited Loading

Choose a reason for hiding this comment

ekexium commented Nov 21, 2022

sticnarf left a comment

Choose a reason for hiding this comment

sticnarf Nov 21, 2022

Choose a reason for hiding this comment

ekexium Nov 21, 2022

Choose a reason for hiding this comment

sticnarf Nov 21, 2022

Choose a reason for hiding this comment

sticnarf Nov 21, 2022

Choose a reason for hiding this comment

ekexium commented Nov 21, 2022

sticnarf left a comment

Choose a reason for hiding this comment

ekexium commented Nov 24, 2022

sticnarf commented Nov 24, 2022

ti-chi-bot commented Nov 24, 2022

sre-bot commented Nov 24, 2022

TiDB MergeCI notify

ekexium commented Nov 14, 2022 •

edited

Loading

ti-chi-bot commented Nov 14, 2022 •

edited

Loading

ekexium Nov 21, 2022 •

edited

Loading