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

executor: fix value swapping and multi-updates in UPDATE statement #20493

Merged
merged 40 commits into from
Dec 10, 2020
Merged

executor: fix value swapping and multi-updates in UPDATE statement #20493

merged 40 commits into from
Dec 10, 2020

Conversation

dyzsr
Copy link
Contributor

@dyzsr dyzsr commented Oct 16, 2020

What problem does this PR solve?

Issue Number: close #11911, close #19137, amendment to #20603 (#20594)

Problem Summary: In UPDATE statement:

What is changed and how it works?

If we have a table:

create table t(a int primary key, b int, c int);
insert into t values (1, 2, 3);

For #19137 (changes in UpdateExec.composeNewRow):

  • Evaluate expressions using old row data rather than new values.
update t set a=b, b=a; # b ← 2, a ← 1

For #11911 :

  • Modify updateRowKeys to allow the same row be updated once for each table alias (instead of once for each table).
# Originally
update t m, t n set m.c=5, m.b=10, n.b=20; # only updates from m will be written into t;

# Now
update t m, t n set m.c=5, m.b=10, n.b=20; # both updates from m and n will be written into t;
  • Use mergedRowData to merge modifications to a row from each table alias.
update t m, t n set m.c=5, m.b=10, n.b=20;
# Before merge
# | as name |        m        |        n        |
# | --------|-----------------|-----------------|
# | column  |  a  |  b  |  c  |  a  |  b  |  c  |
# | old row |  1  |  2  |  3  |  1  |  2  |  3  |
# | new row |  1  |  10 |  5  |  1  |  20 |  3  |
# | flag    |     |  *  |  *  |     |  *  |     |
# 
# Previous updates to the same row of the same table will be merged into current row.
# For example, updates from m will be merged into n,
# | as name |        m        |        n        |
# | --------|-----------------|-----------------|
# | column  |  a  |  b  |  c  |  a  |  b  |  c  |
# | old row |  1  |  2  |  3  |  1  |  10 |  5  |
# | new row |  1  |  10 |  5  |  1  |  20 |  5  |
# | flag    |     |  *  |  *  |     |  *  |     |
  • Change the computations order in UpdateExec.updateRows:
    1. Prepare handles... (UpdateExec.prepare)
    2. Compose new row for non-generated columns (composeFunc)
    3. Merge modifications for non-generated columns (UpdateExec.merge)
    4. Compose new row for generated columns using newly evaluated values (composeGeneratedColumns)
    5. Merge modifications for generated columns (UpdateExec.merge)
    6. Write back to table (UpdateExec.exec)

For #20603 :

  • Reject the update to a table's primary keys if these exists actual multi-updates to that table (In checkUpdateLists). For example:
update t m, t n set m.a=10, n.b=20; # reject
update t m, t n set m.a=10;         # ok

Related changes

  • Need to cherry-pick to the release branch 4.0

Check List

Tests

  • Unit test
  • Integration test

Side effects

  • Performance regression
    • Consumes more CPU
    • Consumes more MEM
  • Breaking backward compatibility

Release note

  • Support value swapping in UPDATE, e.g. update t set a=b, b=a.
    • This may break the compatibility if you have queries like update t set b=a, c=b, as the assignments will use values from the original row instead of the newly evaluated row.
  • Bug fixes for multi-update on the same table, e.g. update t m, t n set m.a=1, n.b=2.
    • Previously, updates from exactly one alias of the same table (m.a or n.b) can be written to storage.
    • Now this has been corrected, both updates can take effects.

@dyzsr dyzsr requested a review from a team as a code owner October 16, 2020 09:12
@dyzsr dyzsr requested review from lzmhhh123 and removed request for a team October 16, 2020 09:12
@github-actions github-actions bot added the sig/execution SIG execution label Oct 16, 2020
@dyzsr dyzsr requested a review from a team as a code owner October 16, 2020 12:03
@dyzsr
Copy link
Contributor Author

dyzsr commented Oct 16, 2020

PTAL @SunRunAway

@dyzsr dyzsr changed the title executor: fix UPDATE same table with multiple aliases executor: fix value swapping and multiple tables modification in UPDATE Oct 18, 2020
@@ -1802,15 +1802,15 @@ func (s *testSuiteP1) TestMultiUpdate(c *C) {
// Test UPDATE ... set_lists.
tk.MustExec(`UPDATE test_mu SET b = 0, c = b WHERE a = 4`)
result = tk.MustQuery(`SELECT * FROM test_mu ORDER BY a`)
result.Check(testkit.Rows(`1 7 2`, `4 0 0`, `7 8 9`))
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the result consistent with MySQL?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. It could be more consistent with PG and SQLite as discussed in #19137.

Copy link
Contributor

@lzmhhh123 lzmhhh123 left a comment

Choose a reason for hiding this comment

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

LGTM

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Oct 20, 2020
@lzmhhh123
Copy link
Contributor

/cc @lysu

@ti-srebot ti-srebot requested a review from lysu October 20, 2020 03:04
@lysu lysu added compatibility-breaker Violation of forwards/backwards compatibility in a design-time piece. require-LGT3 Indicates that the PR requires three LGTM. labels Oct 20, 2020
@lysu
Copy link
Contributor

lysu commented Oct 20, 2020

/bench

@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Dec 9, 2020
@XuHuaiyu
Copy link
Contributor

XuHuaiyu commented Dec 9, 2020

It seems not to be a compatibility-breaker, the behavior is correct for now.

@XuHuaiyu XuHuaiyu removed the compatibility-breaker Violation of forwards/backwards compatibility in a design-time piece. label Dec 9, 2020
@lysu
Copy link
Contributor

lysu commented Dec 9, 2020

LGTM

Copy link
Contributor Author

@dyzsr dyzsr left a comment

Choose a reason for hiding this comment

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

.

@dyzsr
Copy link
Contributor Author

dyzsr commented Dec 9, 2020

/run-all-tests

@dyzsr
Copy link
Contributor Author

dyzsr commented Dec 9, 2020

/run-common-test tidb-test=pr/1111
/run-integration-common-test tidb-test=pr/1111

@dyzsr
Copy link
Contributor Author

dyzsr commented Dec 9, 2020

/run-unit-test

@dyzsr
Copy link
Contributor Author

dyzsr commented Dec 9, 2020

PTAL @lysu @cfzjywxk

@lysu
Copy link
Contributor

lysu commented Dec 9, 2020

LGTM

@ti-srebot ti-srebot removed the status/LGT2 Indicates that a PR has LGTM 2. label Dec 9, 2020
@ti-srebot ti-srebot added the status/LGT3 The PR has already had 3 LGTM. label Dec 9, 2020
@XuHuaiyu XuHuaiyu added the type/bugfix This PR fixes a bug. label Dec 9, 2020
@lzmhhh123
Copy link
Contributor

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Dec 10, 2020
@ti-srebot
Copy link
Contributor

Your auto merge job has been accepted, waiting for:

  • 21633
  • 20924
  • 21179

@ti-srebot
Copy link
Contributor

/run-all-tests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
require-LGT3 Indicates that the PR requires three LGTM. sig/execution SIG execution status/can-merge Indicates a PR has been approved by a committer. status/LGT3 The PR has already had 3 LGTM. type/bugfix This PR fixes a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can not swap values by a multi-tables update Wrong result when update ... set ... on same table
10 participants