doctrine / dbal

Doctrine Database Abstraction Layer
https://www.doctrine-project.org/projects/dbal.html
MIT License
9.43k stars 1.33k forks source link

Why do you begin a transaction in your commit method?! #6258

Open mica16 opened 8 months ago

mica16 commented 8 months ago

Bug Report

Using Doctrine 3.7.2 and Symfony 6.3.

Summary

In the EntityManager.php file, we have the wrapInTransactionmethod:

https://github.com/doctrine/orm/blob/e585a92763612455f591b44cf0482d9852cc5fc0/lib/Doctrine/ORM/EntityManager.php#L267-L284

Inside the code of $this->conn->commit() we can find that at the very end of the method:

https://github.com/doctrine/dbal/blob/6a793fb72948c9dec844de57de8cdbc16ff75bec/src/Connection.php#L1442-L1448

Why to declare a nested transaction at the end without ever closing it?

Current behaviour

I noticed that when I run my integration tests, warning about "Nested Transactions..." I was "forced" to declare "use_savepoints: true" in my doctrine.yaml.

How to reproduce

Don't declare use_savepoints: true in doctrine.yaml and run an integration test using wrapInTransaction.

Expected behaviour

Don't declare a useless nested transaction.

greg0ire commented 8 months ago

Let me guess… wrong repository?

mica16 commented 8 months ago

No, right one

greg0ire commented 8 months ago

Ok, then why did you close? Also, can you post a Github permalink to the file you're mentioning? https://docs.github.com/en/repositories/working-with-files/using-files/getting-permanent-links-to-files

mica16 commented 8 months ago

You're indeed right, actually it was from Doctrine ORM, not Dbal. I will post the issue on the right github repo.

greg0ire commented 8 months ago

Don't worry, I can transfer it.

greg0ire commented 8 months ago

Ah actually, the second code block is in doctrine/dbal, let's transfer the issue back!

greg0ire commented 8 months ago

And to answer your question, this is done in auto-commit mode: https://www.doctrine-project.org/projects/doctrine-dbal/en/3.7/reference/transactions.html#auto-commit-mode

The new transaction is not nested, since we are inside commit(), as explained in the comment: // commits transaction and immediately starts a new one

bassilil commented 7 months ago

cod done?

Guuzen commented 2 months ago

And to answer your question, this is done in auto-commit mode: https://www.doctrine-project.org/projects/doctrine-dbal/en/3.7/reference/transactions.html#auto-commit-mode

The new transaction is not nested, since we are inside commit(), as explained in the comment: // commits transaction and immediately starts a new one

But why do you begin transactions in the commit method? I haven't found an answer about the purpose in the doc. I mean I tried to turn autocommit off and expected to manage transactions manually, but it turned out that dbal behaves differently to what I expected. In this case DBAL tries to handle transactions itself, but only partially because I need to call commit in order to finalize transactions. :thinking:

greg0ire commented 2 months ago

Sorry, I think I meant to wrote "this is done when not in auto-commit mode". When in auto-commit mode, there are no transactions at all => everything is committed to the database as soon as it sent. When not in auto-commit mode, a transaction is automatically started to prevent that, but you have to close is yourself, and Doctrine cannot guess when you want to do that. At least that's my understanding with fresh eyes, and a cursory glance at the docs I mentioned earlier.

Guuzen commented 2 months ago

When not in auto-commit mode, a transaction is automatically started to prevent that, but you have to close is yourself, and Doctrine cannot guess when you want to do that

But it can guess when I need to open a new one :thinking: Well, thank you. I think I understand why it works this way. Because somebody decided that autocommit = off in rdbmses should have two functions:

Current behaviour just mimics autocommit behaviour in databases, but consequently (in the case where autocommit = off) usage of transactional methods becomes pointless (because anyway, we will need to perform an additional commit and also there will probably be unnecessary nested transactions) and transactions demarcation instead of becoming explicit, stays implicit, but in a different way in comparison to autocommit = on :grin: