session: refactor non-transactional DML to make it ready for more DML types by ekexium · Pull Request #38197 · pingcap/tidb

ekexium · 2022-09-27T08:06:59Z

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

What problem does this PR solve?

Issue Number: ref #33485

Problem Summary:

To support update and insert into select later, first refactor the existing code specific to delete.

What is changed and how it works?

Refactor
A proof-of-concept implementation of non-transactional update to show the refactor 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

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

ti-chi-bot · 2022-09-27T08:07:00Z

[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.

Details

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>

parser/parser.y

sticnarf · 2022-09-28T09:32:53Z

Emm, is it really just a refactoring? I think it supports UPDATE for non-transaction DML, right?

ekexium · 2022-09-28T11:44:38Z

Emm, is it really just a refactoring? I think it supports UPDATE for non-transaction DML, right?

Yes. It's not a full implementation though. I'd prefer merge the part first so that following work on UPDATE and INSERT could be parallelized, hopefully.

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

sticnarf

LGTM. Please fix the conflicts.

ekexium · 2022-10-13T15:59:23Z

/run-build
/run-mysql-test

you06

rest LGTM

you06 · 2022-10-14T08:46:21Z

session/nontransactional_test.go

+			tk.MustQuery(fmt.Sprintf("batch on a limit %d update t set b = b * 2", c.batchSize)).Check(testkit.Rows(fmt.Sprintf("%d all succeeded", (c.tableSize+c.batchSize-1)/c.batchSize)))
+			tk.MustQuery("select coalesce(sum(b), 0) from t").Check(testkit.Rows(fmt.Sprintf("%d", (c.tableSize-1)*c.tableSize*2)))
+			tk.MustQuery(fmt.Sprintf("batch on a limit %d delete from t", c.batchSize)).Check(testkit.Rows(fmt.Sprintf("%d all succeeded", (c.tableSize+c.batchSize-1)/c.batchSize)))
+			tk.MustQuery("select count(*) from t").Check(testkit.Rows("0"))


Make a line break here.

Sorry I didn't get it. What is a line break for?

The lines here are too long(not fully displayed on my screen).

OK that makes sense. I used to turn on the auto hard wrap. But it always changes existing code that I'd rather not touch in a PR. Sadly we don't have a hard standard for this :(

session/nontransactional.go

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

sticnarf · 2022-10-19T06:13:48Z

/merge

ti-chi-bot · 2022-10-19T06:13:51Z

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

Details

Commit hash: 1e1c440

sticnarf · 2022-10-19T07:47:41Z

/merge

ti-chi-bot · 2022-10-19T07:47:45Z

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

Details

Commit hash: 438fa3d

sticnarf · 2022-10-19T07:48:04Z

/run-check_dev_2

sre-bot · 2022-10-19T10:11:55Z

TiDB MergeCI notify

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

CI Name	Result	Duration	Compare with Parent commit
idc-jenkins-ci/integration-cdc-test	🔴 failed 2, success 36, total 38	31 min	Existing failure
idc-jenkins-ci-tidb/integration-ddl-test	✅ all 6 tests passed	27 min	Fixed
idc-jenkins-ci-tidb/integration-common-test	🟢 all 17 tests passed	12 min	Existing passed
idc-jenkins-ci-tidb/common-test	🟢 all 11 tests passed	9 min 36 sec	Existing passed
idc-jenkins-ci-tidb/tics-test	🟢 all 1 tests passed	6 min 56 sec	Existing passed
idc-jenkins-ci-tidb/sqllogic-test-2	🟢 all 28 tests passed	5 min 4 sec	Existing passed
idc-jenkins-ci-tidb/sqllogic-test-1	🟢 all 26 tests passed	4 min 5 sec	Existing passed
idc-jenkins-ci-tidb/mybatis-test	🟢 all 1 tests passed	3 min 5 sec	Existing passed
idc-jenkins-ci-tidb/integration-compatibility-test	🟢 all 1 tests passed	2 min 46 sec	Existing passed
idc-jenkins-ci-tidb/plugin-test	🟢 build success, plugin test success	4min	Existing passed

refactor: remove delete-specific names, messages and types

9286d07

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

ti-chi-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 Sep 27, 2022

ekexium changed the title ~~session: refactor non-transactional DML to make it ready for more DML types~~ [WIP] session: refactor non-transactional DML to make it ready for more DML types Sep 27, 2022

ti-chi-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 27, 2022

ekexium force-pushed the refactor-nt branch 2 times, most recently from 68cf5a2 to 06713d6 Compare September 27, 2022 08:49

ekexium changed the title ~~[WIP] session: refactor non-transactional DML to make it ready for more DML types~~ session: refactor non-transactional DML to make it ready for more DML types Sep 27, 2022

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 Sep 27, 2022

ekexium requested a review from you06 September 28, 2022 04:39

feat: proof-of-concept nt-update

1e35121

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

ekexium force-pushed the refactor-nt branch from 06713d6 to 1e35121 Compare September 28, 2022 04:41

ekexium requested a review from sticnarf September 28, 2022 04:43

test: some more tests for nt-update

b4bed53

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

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 28, 2022

Merge branch 'master' of github.com:pingcap/tidb into refactor-nt

554eeb3

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

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 28, 2022

sticnarf reviewed Sep 28, 2022

View reviewed changes

parser/parser.y Outdated Show resolved Hide resolved

fix: typo

27a5a07

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

sticnarf reviewed Oct 13, 2022

View reviewed changes

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 13, 2022

Merge branch 'master' of github.com:pingcap/tidb into refactor-nt

6ecc1e9

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 13, 2022

you06 reviewed Oct 18, 2022

View reviewed changes

refactor: type switch

393a335

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

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 18, 2022

Merge branch 'master' of github.com:pingcap/tidb into refactor-nt

396d193

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 18, 2022

refactor: wrap long lines in test

1e1c440

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

ekexium force-pushed the refactor-nt branch from 945fb9c to 1e1c440 Compare October 19, 2022 03:35

you06 approved these changes Oct 19, 2022

View reviewed changes

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

sticnarf approved these changes Oct 19, 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 Oct 19, 2022

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

ti-chi-bot and others added 2 commits October 19, 2022 14:13

Merge branch 'master' into refactor-nt

930a03c

Merge branch 'master' into refactor-nt

8019221

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 19, 2022

Merge branch 'master' of github.com:pingcap/tidb into refactor-nt

438fa3d

ti-chi-bot removed status/can-merge Indicates a PR has been approved by a committer. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 19, 2022

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

ti-chi-bot added 3 commits October 19, 2022 15:49

Merge branch 'master' into refactor-nt

4539885

Merge branch 'master' into refactor-nt

df95924

Merge branch 'master' into refactor-nt

24a4130

ti-chi-bot merged commit e48f357 into pingcap:master Oct 19, 2022

Conversation

ekexium commented Sep 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

Uh oh!

ti-chi-bot commented Sep 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

sticnarf commented Sep 28, 2022

Uh oh!

ekexium commented Sep 28, 2022

Uh oh!

sticnarf left a comment

Choose a reason for hiding this comment

Uh oh!

ekexium commented Oct 13, 2022

Uh oh!

you06 left a comment

Choose a reason for hiding this comment

Uh oh!

you06 Oct 14, 2022

Choose a reason for hiding this comment

Uh oh!

ekexium Oct 18, 2022

Choose a reason for hiding this comment

Uh oh!

you06 Oct 19, 2022

Choose a reason for hiding this comment

Uh oh!

ekexium Oct 19, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sticnarf commented Oct 19, 2022

Uh oh!

ti-chi-bot commented Oct 19, 2022

Uh oh!

sticnarf commented Oct 19, 2022

Uh oh!

ti-chi-bot commented Oct 19, 2022

Uh oh!

sticnarf commented Oct 19, 2022

Uh oh!

sre-bot commented Oct 19, 2022

TiDB MergeCI notify

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ekexium commented Sep 27, 2022 •

edited

Loading

ti-chi-bot commented Sep 27, 2022 •

edited

Loading