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

Add event subscriber for transaction isolation level #18227

Merged
merged 1 commit into from
Feb 21, 2020

Conversation

kesselb
Copy link
Contributor

@kesselb kesselb commented Dec 4, 2019

Fix #17947

Tricky one. Take a long running task (e.g. uploading a file). Connection is lost from sql server. ReconnectWrapper will handle the disconnect and reconnect. TransactionLevel is correct for the first connection but not for the reconnected one because setTransactionLevel works per connection.

This change is probably danger. setTransactionIsolation(parent::TRANSACTION_READ_COMMITTED); is the same as calling $this->connect() from the constructor.

Master: A new DB\Connection instance will connect immediately to the sql server (triggered by setTransactionIsolation.

This PR: Connection to sql server is established on demand. postConnect will run before the requested query.

Don't have a reliable way to reproduce it yet. Keeping a process open with xdebug and kill the sql connection works sometimes ;)

Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

🐘

@kesselb
Copy link
Contributor Author

kesselb commented Jan 9, 2020

Master is 19 now. Please merge this early.

@kesselb kesselb force-pushed the bug/17947/set-transaction-isolation-connect branch from de64574 to ac6171e Compare January 9, 2020 10:36
@kesselb kesselb force-pushed the bug/17947/set-transaction-isolation-connect branch from ac6171e to 9e699a8 Compare February 21, 2020 08:59
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Code makes sense!

@ChristophWurst ChristophWurst added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Feb 21, 2020
@ChristophWurst ChristophWurst merged commit daf1d74 into master Feb 21, 2020
@ChristophWurst ChristophWurst deleted the bug/17947/set-transaction-isolation-connect branch February 21, 2020 16:48
@wriver4
Copy link

wriver4 commented Aug 20, 2021

The documentation https://docs.nextcloud.com/server/latest/admin_manual/installation/system_requirements.html still says to set the installation to read-committed. Please change. Thanks

@nickvergessen
Copy link
Member

Which is still what we do 🤷‍♂️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish bug technical debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Automatically Set MySQL/MariaDB Session Transaction Isolation to "Read Committed" for Upon Reconnection
5 participants