Skip to content

Conversation

@kesselb
Copy link
Collaborator

@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 ;)

@kesselb kesselb added this to the Nextcloud 19 milestone Dec 4, 2019
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
Collaborator 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
Signed-off-by: Daniel Kesselberg <mail@danielkesselberg.de>
@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