executor: change the evaluation order of columns in `Update` and `Insert` statements #57123

joechenrh · 2024-11-05T06:38:15Z

What problem does this PR solve?

Issue Number: ref #56829

Problem Summary:

In the previous logic, when we use UPDATE or INSERT ON DUPLICATE, the new row will be generated in the following order:

Fill all the explicitly set columns.
Evaluate all auto-generated columns. For UPDATE and INSERT, they are evaluated in composeGeneratedColumns and doDupRowUpdate respectively.
Update on-update-now columns if necessary.

However, auto-generated columns may rely on the on-update-now columns to generate value. For example in #56829 (comment)

CREATE TABLE cache (
  cache_key varchar(512) NOT NULL,
  updated_at datetime NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP,
  expired_at datetime GENERATED ALWAYS AS (if(expires > 0, date_add(updated_at, interval expires second), date_add(updated_at, interval 99 year))) VIRTUAL,
  expires int(11),
  PRIMARY KEY (cache_key) /*T![clustered_index] CLUSTERED */,
  KEY idx_c_on_expired_at (expired_at)
);

expired_at is generated based on the latest timestamp value from updated_at. So we will get wrong expired_at value. Even worse, expired_at is the part of the index idx_c_on_expired_at. So querying data expired_at using index scan and full table scan will get different result, since in full table scan, expired_at is calculated in real-time.

This also explains #56829 (comment) why changing VIRTUAL to STORED will not yield such error, although this value itself is incorrect.

What changed and how does it work?

To address this problem, this PR refactor the logic of INSERT ON DUPLICATE and UPDATE. More specifically:

Move the evalation of auto-generated columns in updateRecord.
ForeignKey Check is also moved into updateRecord.
Extract errorHandler function for UPDATE and INSERT to handle error/warning in updateRecord.

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No need to test
- I checked and no code files have been changed.

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

tiprow · 2024-11-05T06:38:36Z

Hi @joechenrh. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

codecov · 2024-11-05T07:04:53Z

Codecov Report

Attention: Patch coverage is 80.97166% with 47 lines in your changes missing coverage. Please review.

Project coverage is 73.0050%. Comparing base (22c91d0) to head (12a3f93).
Report is 216 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #57123        +/-   ##
================================================
- Coverage   73.2085%   73.0050%   -0.2036%     
================================================
  Files          1679       1698        +19     
  Lines        462531     507757     +45226     
================================================
+ Hits         338612     370688     +32076     
- Misses       103136     115414     +12278     
- Partials      20783      21655       +872

Flag	Coverage Δ
unit	`72.4228% <63.5627%> (+0.0676%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
dumpling	`52.6910% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`40.1288% <ø> (-5.8702%)`	⬇️

joechenrh · 2024-11-06T05:49:17Z

/retest

tiprow · 2024-11-06T05:49:45Z

@joechenrh: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

pkg/executor/insert_test.go

YangKeao · 2024-11-06T08:22:43Z

pkg/executor/insert.go

@@ -430,8 +443,15 @@ func (e *InsertExec) initEvalBuffer4Dup() {
 }

 // doDupRowUpdate updates the duplicate row.
-func (e *InsertExec) doDupRowUpdate(ctx context.Context, handle kv.Handle, oldRow []types.Datum, newRow []types.Datum,
-	extraCols []types.Datum, cols []*expression.Assignment, idxInBatch int, dupKeyMode table.DupKeyCheckMode, autoColIdx int) error {
+func (e *InsertExec) doDupRowUpdate(


It only fixes the INSERT .. ON DUPLICATE UPDATE .. case. As I have tested, the UPDATE path also has the same bug:

CREATE TABLE cache ( cache_key varchar(512) NOT NULL, updated_at datetime NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP, expired_at datetime GENERATED ALWAYS AS (if(expires > 0, date_add(updated_at, interval expires second), date_add(updated_at, interval 99 year))) VIRTUAL, expires int(11), PRIMARY KEY (cache_key) /*T![clustered_index] CLUSTERED */, KEY idx_c_on_expired_at (expired_at) ); INSERT INTO cache(cache_key, expires) VALUES ('2001-01-01 11:11:11', 60) ON DUPLICATE KEY UPDATE expires = expires + 1; update cache set expires = expires + 1 where cache_key = '2001-01-01 11:11:11';

Then the following two queries will have different result:

select /*+ force_index(test.cache, idx_c_on_expired_at) */ cache_key, expired_at from cache order by cache_key; select /*+ ignore_index(test.cache, idx_c_on_expired_at) */ cache_key, expired_at from cache order by cache_key;

YangKeao · 2024-11-06T08:30:40Z

pkg/executor/insert.go

-	if err != nil {
-		return err
+
+	if _, err := updateRecord(


Is it possible to move the logic of handling generated column into the updateRecord? It seems that the function updateRecord also handles the ON UPDATE columns (and as we specified all ON UPDATE column in this functioin, these logic are meaningless).

Another possible solution is to remove the codes related to ON UPDATE columns in updateRecord function. However, as the same logic will be used multiple times (in INSERT .. ON DUPLICATE UPDATE and a normal UPDATE statement), I prefer to write the codes related to generated column in updateRecord to avoid repeating the codes.

pkg/executor/insert.go

joechenrh · 2024-12-11T05:44:28Z

/ok-to-test

joechenrh · 2024-12-11T06:34:32Z

/retest

ti-chi-bot · 2024-12-16T09:35:03Z

[LGTM Timeline notifier]

Timeline:

2024-12-12 05:53:51.808632211 +0000 UTC m=+504221.897434752: ☑️ agreed by YangKeao.
2024-12-16 09:35:02.66825884 +0000 UTC m=+863092.757061377: ☑️ agreed by wjhuang2016.

joechenrh · 2024-12-17T02:08:31Z

/hold
schrddl found another problem

joechenrh · 2024-12-18T02:41:21Z

/unhold
The problem is related to plan builder, not executor. I will fix it in another PR.

joechenrh · 2024-12-18T02:43:48Z

/retest

joechenrh · 2024-12-18T02:47:21Z

/retest-all

joechenrh · 2024-12-18T02:47:48Z

/retest-required

joechenrh · 2024-12-24T02:07:13Z

/cherry-pick release-7.5

Signed-off-by: ti-chi-bot <[email protected]>

ti-chi-bot · 2024-12-24T02:07:56Z

@joechenrh: new pull request created to branch release-7.5: #58494.

In response to this:

/cherry-pick release-7.5

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

joechenrh · 2024-12-25T06:02:47Z

/cherry-pick release-7.1

Signed-off-by: ti-chi-bot <[email protected]>

ti-chi-bot · 2024-12-25T06:03:32Z

@joechenrh: new pull request created to branch release-7.1: #58524.

In response to this:

/cherry-pick release-7.1

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

joechenrh · 2025-01-06T07:58:02Z

/cherry-pick release-8.5

ti-chi-bot · 2025-01-06T07:58:44Z

@joechenrh: new pull request created to branch release-8.5: #58708.

In response to this:

/cherry-pick release-8.5

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

…ert` statements (#57123) (#58708) ref #56829

ti-chi-bot · 2025-01-21T03:38:21Z

In response to a cherrypick label: new pull request could not be created: failed to create pull request against pingcap/tidb#release-6.5 from head ti-chi-bot:cherry-pick-57123-to-release-6.5: the GitHub API request returns a 403 error: {"message":"You have exceeded a secondary rate limit and have been temporarily blocked from content creation. Please retry your request again later. If you reach out to GitHub Support for help, please include the request ID B9C8:1BF88E:FA678C:1F2CF15:678F16AC and timestamp 2025-01-21 03:38:21 UTC.","documentation_url":"https://docs.github.com/rest/overview/rate-limits-for-the-rest-api#about-secondary-rate-limits","status":"403"}

Signed-off-by: ti-chi-bot <[email protected]>

…ert` statements (#57123) (#59273) ref #56829

…ert` statements (#57123) (#58494) ref #56829

Change order of column evaluation

9381a49

ti-chi-bot bot added do-not-merge/needs-triage-completed 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 Nov 5, 2024

Fix

befa301

YangKeao reviewed Nov 6, 2024

View reviewed changes

joechenrh added 2 commits November 19, 2024 14:23

Implement

80aab91

Fix update statement

8f7117d

ti-chi-bot 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 20, 2024

joechenrh added 4 commits December 10, 2024 16:47

Refine code

cbe3925

Fix build

3d8a2b4

Fix

484291b

fix multi update

f655f8f

ti-chi-bot bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Dec 10, 2024

joechenrh added 5 commits December 11, 2024 09:22

fix multi update

69a7c1b

fix multi update

ac898c6

fix integrationtest

af6b478

Add test and fix comments

93c6d69

simplify code

0ba873c

YangKeao self-requested a review December 11, 2024 05:29

ti-chi-bot bot added the ok-to-test Indicates a PR is ready to be tested. label Dec 11, 2024

joechenrh changed the title ~~executor: change the order of column value evaluation in doDupRowUpdate~~ executor: change the evaluation order of columns in Update and Insert statements Dec 11, 2024

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

Merge branch 'master' into fix-generated-column

12a3f93

ti-chi-bot bot removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. do-not-merge/needs-triage-completed labels Dec 18, 2024

ti-chi-bot bot merged commit 5ac0b2e into pingcap:master Dec 18, 2024
24 checks passed

joechenrh mentioned this pull request Dec 23, 2024

The index data and table data of the TTL table are inconsistent #56829

Closed

ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request Dec 24, 2024

This is an automated cherry-pick of pingcap#57123

3dc5a95

Signed-off-by: ti-chi-bot <[email protected]>

ti-chi-bot mentioned this pull request Dec 24, 2024

executor: change the evaluation order of columns in Update and Insert statements (#57123) #58494

Merged

13 tasks

ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request Dec 25, 2024

This is an automated cherry-pick of pingcap#57123

9f6e6c7

Signed-off-by: ti-chi-bot <[email protected]>

ti-chi-bot mentioned this pull request Dec 25, 2024

executor: change the evaluation order of columns in Update and Insert statements (#57123) #58524

Open

13 tasks

ti-chi-bot mentioned this pull request Jan 6, 2025

executor: change the evaluation order of columns in Update and Insert statements (#57123) #58708

Merged

13 tasks

ti-chi-bot bot pushed a commit that referenced this pull request Jan 9, 2025

executor: change the evaluation order of columns in Update and `Ins…

587a810

…ert` statements (#57123) (#58708) ref #56829

ti-chi-bot bot added the needs-cherry-pick-release-6.5 Should cherry pick this PR to release-6.5 branch. label Jan 21, 2025

ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request Jan 21, 2025

This is an automated cherry-pick of pingcap#57123

b8641a2

Signed-off-by: ti-chi-bot <[email protected]>

joechenrh mentioned this pull request Feb 6, 2025

executor: change the evaluation order of columns in Update and Insert statements (#57123) #59273

Merged

13 tasks

ti-chi-bot bot pushed a commit that referenced this pull request Feb 11, 2025

executor: change the evaluation order of columns in Update and `Ins…

d8dd55b

…ert` statements (#57123) (#59273) ref #56829

ti-chi-bot bot pushed a commit that referenced this pull request Feb 27, 2025

executor: change the evaluation order of columns in Update and `Ins…

e34e953

…ert` statements (#57123) (#58494) ref #56829

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

executor: change the evaluation order of columns in `Update` and `Insert` statements #57123

executor: change the evaluation order of columns in `Update` and `Insert` statements #57123

joechenrh commented Nov 5, 2024 •

edited

Loading

tiprow bot commented Nov 5, 2024

codecov bot commented Nov 5, 2024 •

edited

Loading

joechenrh commented Nov 6, 2024

tiprow bot commented Nov 6, 2024

YangKeao Nov 6, 2024

YangKeao Nov 6, 2024

joechenrh commented Dec 11, 2024

joechenrh commented Dec 11, 2024

ti-chi-bot bot commented Dec 16, 2024

joechenrh commented Dec 17, 2024

joechenrh commented Dec 18, 2024

joechenrh commented Dec 18, 2024

joechenrh commented Dec 18, 2024

joechenrh commented Dec 18, 2024

joechenrh commented Dec 24, 2024

ti-chi-bot commented Dec 24, 2024

joechenrh commented Dec 25, 2024

ti-chi-bot commented Dec 25, 2024

joechenrh commented Jan 6, 2025

ti-chi-bot commented Jan 6, 2025

ti-chi-bot commented Jan 21, 2025

executor: change the evaluation order of columns in Update and Insert statements #57123

executor: change the evaluation order of columns in Update and Insert statements #57123

Conversation

joechenrh commented Nov 5, 2024 • edited Loading

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

tiprow bot commented Nov 5, 2024

codecov bot commented Nov 5, 2024 • edited Loading

Codecov Report

joechenrh commented Nov 6, 2024

tiprow bot commented Nov 6, 2024

YangKeao Nov 6, 2024

Choose a reason for hiding this comment

YangKeao Nov 6, 2024

Choose a reason for hiding this comment

joechenrh commented Dec 11, 2024

joechenrh commented Dec 11, 2024

ti-chi-bot bot commented Dec 16, 2024

[LGTM Timeline notifier]

joechenrh commented Dec 17, 2024

joechenrh commented Dec 18, 2024

joechenrh commented Dec 18, 2024

joechenrh commented Dec 18, 2024

joechenrh commented Dec 18, 2024

joechenrh commented Dec 24, 2024

ti-chi-bot commented Dec 24, 2024

joechenrh commented Dec 25, 2024

ti-chi-bot commented Dec 25, 2024

joechenrh commented Jan 6, 2025

ti-chi-bot commented Jan 6, 2025

ti-chi-bot commented Jan 21, 2025

executor: change the evaluation order of columns in `Update` and `Insert` statements #57123

executor: change the evaluation order of columns in `Update` and `Insert` statements #57123

joechenrh commented Nov 5, 2024 •

edited

Loading

codecov bot commented Nov 5, 2024 •

edited

Loading