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

[5.2] Handle InnoDB Deadlocks By Re-Attempting Transactions #12151

Merged
merged 1 commit into from
Feb 3, 2016
Merged

[5.2] Handle InnoDB Deadlocks By Re-Attempting Transactions #12151

merged 1 commit into from
Feb 3, 2016

Conversation

agustinprod
Copy link
Contributor

On MySQL galera cluster, deadlocks happens very often. Fixes #5380

taylorotwell added a commit that referenced this pull request Feb 3, 2016
Handle InnoDB Deadlocks By Re-Attempting Transactions
@taylorotwell taylorotwell merged commit d47e210 into laravel:5.2 Feb 3, 2016
@GrahamCampbell GrahamCampbell changed the title Handle InnoDB Deadlocks By Re-Attempting Transactions [5.2] Handle InnoDB Deadlocks By Re-Attempting Transactions Feb 3, 2016
@GrahamCampbell
Copy link
Member

This needs to go to 5.1.

@agustinprod
Copy link
Contributor Author

Should I do another pull request at 5.1 branch?

@carcinocron
Copy link

Correct me if I'm wrong, but just to clarify, I'm pretty sure this retries the MySQL connection, not the actual transaction in deadlock.

@davidsivocha
Copy link

davidsivocha commented Jun 16, 2016

@InstanceOfMichael is right! This doesn't solve the problem at all, it just tries to call reconnect, which still makes deadlocks fail because you can't reconnect when there is an open transaction in MySQL waiting to be finished. In no way does this fix #5380 at all. If anything it just compounds the problem!

@agustinprod
Copy link
Contributor Author

I cannot actually test the patch, the deadlocks happen on a 4.2 app. But correct me if i'm wrong, this error happens even if you are not using transactions (atleast on Galera cluster) and in that case, this should work.

There is any way to trigger this deadlock on a single instance mariaDB? I'll be happy to provide a full patch to fix this issue.

@davidsivocha
Copy link

Yes, it solves the issue in a single use case. But that's not the cause in the issue it pupports to fix (#5380).

When a deadlock happens in normal use cases, this added line will cause your DB to reconnect and to re-try to query that was deadlocked, which is fine!

The problem is that if you have started a DB::transaction() this method causes your transaction to fail because if you deadlock, it tries to reconnect, which you cannot do while mid transaction which triggers another PDO error, instead of handling the deadlock correctly (either initiating a rollback, or attempting to retry the transaction).

DB::transaction() DB::commit() and DB::rollback() cannot ever let you reconnect your PDO instance.

@carcinocron
Copy link

this error happens even if you are not using transactions (atleast on Galera cluster) and in that case, this should work.

Yes, the pull request is definitely important and a bugfix, but the description of the bugfix is misleading (or blatantly wrong).

If anyone actually wants automatic retries, they'll need to write their own fancy DB::transaction(function(){ ... }) replacement checking things like DB::transactionLevel()

@davidsivocha
Copy link

davidsivocha commented Jun 16, 2016

For me, automatic retries aren't even a problem. The problem lies in the fact that the framework tries to force a reconnect regardless of the nature of the query being run. If a DB::transaction() has started, PDO should never attempt a reconnect.

Edit: Until after DB::commit() has been called anyway

@carcinocron
Copy link

DB::transaction() definitely needs an upgrade, I've thought about some fluent builder things like:

Transaction::make(function(){
    [...]
})
->retries(3)
->exec();

But I've never written anything.

@davidsivocha
Copy link

The other issue at play here is that you are also treating a Deadlock as a Disconnect, which is also not the case. A deadlock and a disconnect are two completely separate things.

In a nutshell, a Deadlock needs to either retry or discard (and rollback if you are in a transaction), not force a reconnect because a Deadlock is not a disconnect

@InstanceOfMichael That would actually make a lot of sense. A decent transaction builder would be lovely to have in here, especially if you can specify a retry count before discard and rollback.

@taylorotwell
Copy link
Member

taylorotwell commented Jun 16, 2016

Should this be reverted? Could someone with some more knowledge make a PR?

@GrahamCampbell
Copy link
Member

NB If it is reverted, or simply repaired, this will need to happen on 5.1.

@carcinocron
Copy link

carcinocron commented Jun 22, 2016

Should this be reverted? Could someone with some more knowledge make a PR?

Probably, I have an exception where I attempt to DB::rollback() an unexpected open transaction from a previous queue job (so DB::transactionLevel() returns 1) and PDOException: There is no active transaction is thrown. I'm worried its possible that one or more queue workers can get stuck in limbo over this. One or more of these need to happen:

  • Illuminate\Database\Connection needs to set ->transactions = 0 on reconnect.
  • Illuminate\Database\Connection needs to be deconstructed and fresh re-instanciated (not sure when, maybe on reconnect).
  • need a way to tell a single queue worker (not all of them) to restart.
  • SerializesModels throws RuntimeException: Can't swap PDO instance while within transaction. I'm not able to run any safety cleanup code before this in a clean way.

sbine added a commit to sbine/framework that referenced this pull request Aug 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants