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

fix(transaction): delete autocommit when transaction begin (#9877) #9921

Merged
merged 2 commits into from Sep 19, 2018

Conversation

@tsasaki609
Copy link
Contributor

@tsasaki609 tsasaki609 commented Sep 13, 2018

Pull Request check-list

Please make sure to review and check all of these items:

  • Does npm run test or npm run test-DIALECT pass with this change (including linting)?
  • Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
  • Have you added new tests to prevent regressions?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
  • Did you follow the commit message conventions explained in CONTRIBUTING.md?

Description of change

fix #9877
Statements within a transaction should either be all successful, or all fail. Autocommit arguments that initiate transactions are misleading and should not be provided. If we want to enable autocommit, we should explicitly call auto-commit. How do you think?

@tsasaki609 tsasaki609 force-pushed the tsasaki609:fix-transaction-control branch from 186b095 to 0115b34 Sep 14, 2018
@sushantdhiman
Copy link
Contributor

@sushantdhiman sushantdhiman commented Sep 17, 2018

MySQL transaction with AUTO COMMIT = 1 does defeat purpose of having transaction i.e. each statement is committed as soon as it is finished without any chance of rollback. This is also default behaviour of MySQL.

Two things can be done here

  1. Set autocommit = false by default, so transactions is predictable
  2. Related to fixing #9877 , we need to reset AUTO COMMIT status when transaction exits. How we will do that is not sure, may be a session variable to store current auto commit value and reset it after transaction
@tsasaki609
Copy link
Contributor Author

@tsasaki609 tsasaki609 commented Sep 17, 2018

Since we are executing START TRANSACTION internally, is not it possible to do SET autocommit = 0 to affect the behavior until the end of the transaction?

If we explicitly use a transaction, it seems that it was behavior that autocommit had no effect until you explicitly commit or roll back.

Sorry if my understanding was wrong.

@sushantdhiman
Copy link
Contributor

@sushantdhiman sushantdhiman commented Sep 18, 2018

Hmm you are right, https://dev.mysql.com/doc/refman/8.0/en/commit.html

By default, MySQL runs with autocommit mode enabled. This means that as soon as you execute a statement that updates (modifies) a table, MySQL stores the update on disk to make it permanent. The change cannot be rolled back.

To disable autocommit mode implicitly for a single series of statements, use the START TRANSACTION statement:

START TRANSACTION;
SELECT @A:=SUM(salary) FROM table1 WHERE type=1;
UPDATE table2 SET summary=@A WHERE type=1;
COMMIT;

With START TRANSACTION, autocommit remains disabled until you end the transaction with COMMIT or ROLLBACK. The autocommit mode then reverts to its previous state.

So If I understood correctly AUTO COMMIT will not affect statement under transactions it only works for statement not under transaction as an implicit transaction

SET AUTOCOMMIT = 0;
#DO STUFF
COMMIT;

None of other database engine use auto commit so in addition to removing this statement from here we need to remove this option and its query generation methods

@tsasaki609
Copy link
Contributor Author

@tsasaki609 tsasaki609 commented Sep 18, 2018

@sushantdhiman
I am glad you understood.

In addition to MySQL, PostgreSQL, SQLite, and MS SQL Server seem to have autocommit statement.
https://www.postgresql.jp/document/9.4/html/ecpg-sql-set-autocommit.html
https://www.sqlite.org/c3ref/get_autocommit.html
https://docs.microsoft.com/en-us/previous-versions/sql/sql-server-2008-r2/ms187878(v=sql.105)

However, the essential meaning of the autocommut statement is that any product is similar,
we don't need to think about separately.

I also deleted options and query generation methods.

Thank you for your review.

@sushantdhiman
Copy link
Contributor

@sushantdhiman sushantdhiman commented Sep 18, 2018

In addition to MySQL, PostgreSQL, SQLite, and MS SQL Server seem to have autocommit statement.
https://www.postgresql.jp/document/9.4/html/ecpg-sql-set-autocommit.html
https://www.sqlite.org/c3ref/get_autocommit.html
https://docs.microsoft.com/en-us/previous-versions/sql/sql-server-2008-r2/ms187878(v=sql.105)

Yeah I meant more at Sequelize level, where we only support/implemented this for MySQL.

@sushantdhiman sushantdhiman merged commit ee9ec57 into sequelize:master Sep 19, 2018
4 checks passed
4 checks passed
codecov/patch Coverage not affected when comparing aa92764...a694ae9
Details
codecov/project 96.09% (+0.05%) compared to aa92764
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@sushantdhiman
Copy link
Contributor

@sushantdhiman sushantdhiman commented Sep 19, 2018

Thanks @tsasaki609

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants