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

session: add a batch commit session variable for the large transaction #8293

Merged
merged 16 commits into from
Dec 10, 2018

Conversation

jackysp
Copy link
Member

@jackysp jackysp commented Nov 13, 2018

What problem does this PR solve?

tidb_batch_insert / tidb_batch_delete only split the statement into several batches. If the transaction contains many statements, TiDB could not commit it in batch.

What is changed and how it works?

Add a session level variable to control if the transaction should return an error or start a new transaction when it exceeds the statements limit of the transaction. It will be useful when importing data.

Check List

Tests

  • Unit test
  • Manual test (add detailed scripts or steps below)
    TPC-C data importing test works fine on it.

Code changes

  • Has exported function/method change

Related changes

  • Need to update the documentation

PTAL @tiancaiamao @lysu


This change is Reviewable

@jackysp jackysp added type/enhancement The issue or PR belongs to an enhancement. component/server labels Nov 13, 2018
@tiancaiamao
Copy link
Contributor

I'm afraid if a transaction use too many statements, TiDB OOM quickly and this config can't help.

@tiancaiamao
Copy link
Contributor

tiancaiamao commented Nov 13, 2018

I remember that the memory amplification is terrible, and count = 5000 is big enough for most cases

session/session_test.go Outdated Show resolved Hide resolved
session/tidb.go Outdated Show resolved Hide resolved
// The last history could not be "commit" statement.
// It means it is impossible to start a new transaction at the end of the transaction (the "commit" statement).
// Because after the server executed "commit" statement, the session is out of the transaction.
se.sessionVars.SetStatusFlag(mysql.ServerStatusInTrans, true)
Copy link
Contributor

Choose a reason for hiding this comment

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

What will happen if the last statement is a rollback, will there be a corner case that this line put the session into a new transaction while it should be rollbacked ?

Copy link
Member Author

Choose a reason for hiding this comment

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

rollback is the same as commit.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will update the comment and add a rollback test case.

Copy link
Contributor

Choose a reason for hiding this comment

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

s1:
set @@autocommit = 0;
insert into t value (1);
rollback;

s2:
insert into t values (1);
insert into t values (2);

s1:
select  * from t;    // what it see ?

@jackysp

Copy link
Member Author

@jackysp jackysp Nov 16, 2018

Choose a reason for hiding this comment

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

So the statement count limit is 1 for this case?

config/config.toml.example Outdated Show resolved Hide resolved
@tiancaiamao tiancaiamao added the require-LGT3 Indicates that the PR requires three LGTM. label Nov 14, 2018
@tiancaiamao
Copy link
Contributor

I've tag a requre LGTM3 to this PR.
Changes involved in transaction should be review carefully. @jackysp

@tiancaiamao
Copy link
Contributor

LGTM

@tiancaiamao tiancaiamao added the status/LGT1 Indicates that a PR has LGTM 1. label Nov 16, 2018
@jackysp jackysp removed the require-LGT3 Indicates that the PR requires three LGTM. label Nov 27, 2018
@jackysp
Copy link
Member Author

jackysp commented Nov 27, 2018

PTAL @lysu @winkyao

@jackysp
Copy link
Member Author

jackysp commented Nov 28, 2018

PTAL @lysu @winkyao. This PR is useful for the benchmarkSQL test.

1 similar comment
@jackysp
Copy link
Member Author

jackysp commented Nov 28, 2018

PTAL @lysu @winkyao. This PR is useful for the benchmarkSQL test.

@lysu
Copy link
Contributor

lysu commented Dec 3, 2018

IMHO, it's better let BatchCommit be a session varibale instead of server config?

Just as description above, this config only useful in prepare data situation and only can be "safe" use in prepare data situation.

make variable in server level will be problem, just imagine: we have a tidb running under heavy online transaction processing, now we want add a table and using a "import program" to import some data in new table. we just want to import fast in import program's sql, the sql from online processing should never affect by batchCommit change..

So, we should better use session variable, and set it in program's database dsn

 jdbc:mysql://localhost:4000/db?characterEncoding=UTF-8&sessionVariables=batch_commit=100
root@tcp(localhost:4001)/db?strict=true&batch_commit=100

@jackysp
Copy link
Member Author

jackysp commented Dec 3, 2018

PTAL @lysu

lysu
lysu previously approved these changes Dec 4, 2018
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

@lysu
Copy link
Contributor

lysu commented Dec 4, 2018

/run-all-tests

@lysu lysu added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Dec 4, 2018
Copy link
Contributor

@tiancaiamao tiancaiamao left a comment

Choose a reason for hiding this comment

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

I think this config option is risky enough and need another LGTM @shenli

@jackysp
Copy link
Member Author

jackysp commented Dec 4, 2018

/run-common-test
/run-integration-common-test

@jackysp jackysp changed the title config, session: add a batch commit config for the large transaction session: add a batch commit session variable for the large transaction Dec 4, 2018
@jackysp
Copy link
Member Author

jackysp commented Dec 5, 2018

PTAL @winkyao @zz-jason

1 similar comment
@jackysp
Copy link
Member Author

jackysp commented Dec 7, 2018

PTAL @winkyao @zz-jason

winkyao
winkyao previously approved these changes Dec 10, 2018
Copy link
Contributor

@winkyao winkyao left a comment

Choose a reason for hiding this comment

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

LGTM

@zz-jason
Copy link
Member

@jackysp CI failed.

@jackysp jackysp dismissed stale reviews from winkyao and lysu via 7ef4275 December 10, 2018 09:33
@jackysp
Copy link
Member Author

jackysp commented Dec 10, 2018

/run-all-tests

@tiancaiamao tiancaiamao added status/LGT3 The PR has already had 3 LGTM. and removed status/LGT2 Indicates that a PR has LGTM 2. labels Dec 10, 2018
@jackysp jackysp merged commit e3f3ac2 into pingcap:master Dec 10, 2018
@jackysp jackysp deleted the batch_commit branch February 11, 2019 07:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/server status/LGT3 The PR has already had 3 LGTM. type/enhancement The issue or PR belongs to an enhancement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants