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 deadloop when exec replace with generated column #10357

Closed

Conversation

amyangfei
Copy link
Contributor

@amyangfei amyangfei commented May 5, 2019

What problem does this PR solve?

TiDB will run into dead loop and exhausts large memory very soon if we execute replace into values (...) on a generated column. The bug can be reproduced as following:

mysql> create table t3 (id int, id_gen int as (`id`+1), unique key id_gen(`id_gen`));
Query OK, 0 rows affected (0.04 sec)

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

mysql> replace into t3 (id) values (1);

The root cause of this is we fetched key value from store for duplicated kv checks, but did not apply generated column rule.

What is changed and how it works?

Force remove duplicated key from ReplaceExec.dupKVs.

This pr is not a totally solution to this problem, it solves the deadloop, but a replace into operation having duplicated unique key conflict on a generated column still fail now.

I didn't find a suit method to calculate generated column value without building a logic planner, if anyone points out a workaround way, I will have a try to make this pr better.

Check List

Tests

  • Unit test
  • Integration test

Related changes

  • Need to cherry-pick to the release branch

@shenli shenli added the type/bugfix This PR fixes a bug. label May 5, 2019
@amyangfei amyangfei force-pushed the replace-info-with-gen-col branch 2 times, most recently from 97c4e9a to e6b51e3 Compare May 6, 2019 02:58
@codecov
Copy link

codecov bot commented May 6, 2019

Codecov Report

Merging #10357 into master will decrease coverage by 0.0102%.
The diff coverage is 100%.

@@               Coverage Diff                @@
##             master     #10357        +/-   ##
================================================
- Coverage   77.6897%   77.6794%   -0.0103%     
================================================
  Files           411        411                
  Lines         85436      85446        +10     
================================================
- Hits          66375      66374         -1     
- Misses        14101      14111        +10     
- Partials       4960       4961         +1

@codecov
Copy link

codecov bot commented May 6, 2019

Codecov Report

Merging #10357 into master will decrease coverage by 0.0266%.
The diff coverage is 100%.

@@               Coverage Diff               @@
##             master    #10357        +/-   ##
===============================================
- Coverage   77.6897%   77.663%   -0.0267%     
===============================================
  Files           411       411                
  Lines         85436     85446        +10     
===============================================
- Hits          66375     66360        -15     
- Misses        14101     14119        +18     
- Partials       4960      4967         +7

@amyangfei amyangfei force-pushed the replace-info-with-gen-col branch from e6b51e3 to 4787594 Compare May 6, 2019 03:19
@amyangfei
Copy link
Contributor Author

/run-all-tests

@jackysp
Copy link
Member

jackysp commented May 6, 2019

I filed this PR #10370 to fix it. Not fully tested.

@amyangfei amyangfei closed this May 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/execution SIG execution type/bugfix This PR fixes a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants