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: add missing update table delta for TxnCtx #20982

Merged
merged 20 commits into from
Nov 19, 2020

Conversation

blacktear23
Copy link
Contributor

@blacktear23 blacktear23 commented Nov 11, 2020

What problem does this PR solve?

Issue Number: close #20975

Problem Summary:

mysql> drop table t2, t1;
Query OK, 0 rows affected (0.04 sec)

mysql> create table t1(a int);
Query OK, 0 rows affected (0.01 sec)

mysql> insert into t1 values (1);
Query OK, 1 row affected (0.00 sec)

mysql> begin pessimistic;
Query OK, 0 rows affected (0.00 sec)

mysql> update t1 set a=a;
Query OK, 0 rows affected (0.00 sec)
Rows matched: 1  Changed: 0  Warnings: 0

/* session 2 */ mysql> create table t2(a int);
Query OK, 0 rows affected (0.00 sec)

mysql> commit;
ERROR 8028 (HY000): Information schema is changed during the execution of the statement(for example, table definition may be updated by other DDL ran in parallel). If you see this error often, try increasing `tidb_max_delta_schema_count`. [try again later]

What is changed and how it works?

What's Changed:

When update statement without changes it missing track table ID for SchemaValidator. So when execute commit statement will cause SchemaValidator check whole Schema without tables and this will cause schema change error.

Related changes

  • No

Check List

Tests

  • Manual test (steps see below)
mysql> drop table t2, t1;
Query OK, 0 rows affected (0.04 sec)

mysql> create table t1(a int);
Query OK, 0 rows affected (0.01 sec)

mysql> insert into t1 values (1);
Query OK, 1 row affected (0.00 sec)

mysql> begin pessimistic;
Query OK, 0 rows affected (0.00 sec)

mysql> update t1 set a=a;
Query OK, 0 rows affected (0.00 sec)
Rows matched: 1  Changed: 0  Warnings: 0

/* session 2 */ mysql> create table t2(a int);
Query OK, 0 rows affected (0.00 sec)

mysql> commit;
Query OK, 0 rows affected (0.00 sec)

Side effects

  • Performance regression
    • Consumes more CPU when DML cause nothing changes
    • Consumes more MEM when DML cause nothing changes
    • Add lock for update TableDeltaMap may cause little slower

Release note

  • No Release Note

@blacktear23 blacktear23 requested a review from a team as a code owner November 11, 2020 06:29
@blacktear23 blacktear23 requested review from lzmhhh123 and removed request for a team November 11, 2020 06:29
@github-actions github-actions bot added the sig/execution SIG execution label Nov 11, 2020
executor/write.go Outdated Show resolved Hide resolved
executor/batch_point_get.go Outdated Show resolved Hide resolved
executor/write.go Outdated Show resolved Hide resolved
@wjhuang2016 wjhuang2016 requested a review from cfzjywxk November 16, 2020 04:13
@blacktear23
Copy link
Contributor Author

@cfzjywxk PTAL

@wjhuang2016 wjhuang2016 requested a review from cfzjywxk November 17, 2020 07:08
@bb7133
Copy link
Member

bb7133 commented Nov 18, 2020

PTAL @cfzjywxk

@cfzjywxk
Copy link
Contributor

LGTM

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

@lysu lysu 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 removed the status/LGT1 Indicates that a PR has LGTM 1. label Nov 18, 2020
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Nov 18, 2020
@lysu
Copy link
Contributor

lysu commented Nov 19, 2020

/merge

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

/run-all-tests

@ti-srebot
Copy link
Contributor

@blacktear23 merge failed.

Copy link
Member

@wjhuang2016 wjhuang2016 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 status/LGT3 The PR has already had 3 LGTM. and removed status/LGT2 Indicates that a PR has LGTM 2. labels Nov 19, 2020
@lysu
Copy link
Contributor

lysu commented Nov 19, 2020

/merge

@ti-srebot
Copy link
Contributor

Your auto merge job has been accepted, waiting for:

  • 21015

@blacktear23
Copy link
Contributor Author

/run-tics-test

@wjhuang2016 wjhuang2016 merged commit e6e894d into pingcap:master Nov 19, 2020
@jebter
Copy link

jebter commented Nov 23, 2020

@wjhuang2016 Maybe cherry-pick to the 4.0 branch?

@wjhuang2016
Copy link
Member

/run-cherry-picker

ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Nov 25, 2020
@ti-srebot
Copy link
Contributor

cherry pick to release-4.0 in PR #21279

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unexpected "Information schema is changed" when commits
8 participants