Fix incorrect transactional() handling when DB auto-rolled back the transaction

[simPod]

Q	A
Type	bug
Fixed issues	#3423

Summary

#6543 partially solved what we tried to achieve in #4846. With that There's no active transaction exception contains the actual exception in $previous.

Now, let's get rid of There's no active transaction exception which occurs e.g. when using deferred constraints so the violation is checked at the end of the transaction and not during it. Database then rolls back the transaction automatically.

The current faulty flow is following:

1. on commit(), Constrain violation occurs
2. transaction is implicitly aborted and rolled back in database
3. DBAL calls rollback()
4. Since there's no active transaction, There's no active transaction exception is thrown:

This PR adjusts the flow so:

1. on commit(), Constrain violation occurs
2. transaction is implicitly aborted and rolled back in database
3. DBAL DOES NOT call rollback() 🥳

---

For DBAL v4 it looks like this #6546 (draft, does not contain review changes)

[greg0ire]

What if commit throws for some other reason, before anything is transmitted to the database?

[simPod]

Can you describe some scenario you have in mind so I can cover it with test?

[greg0ire]

Here, there are throws statements:

dbal/src/Connection.php

Lines 1013 to 1026 in 220aec4

	throw NoActiveTransaction::new();
	}
	if ($this->isRollbackOnly) {
	throw CommitFailedRollbackOnly::new();
	}
	$connection = $this->connect();
	if ($this->transactionNestingLevel === 1) {
	try {
	$connection->commit();
	} catch (Driver\Exception $e) {
	throw $this->convertException($e);

I don't know if there is a realistic scenario in which they can throw when commit is called from the method you are modifying, before the transaction is actually committed, but that's what I was thinking about.

[simPod]

I see. Those should not happen but I have adjusted the flow and covered with unit test should it happen.

72af9b8

[greg0ire]

Why not do something similar to what I did in doctrine/orm@b6137c8 and use isTransactionActive()?

[simPod]

I forgot about that API, should work.

[greg0ire]

What should happen in this scenario?

$connection->beginTransaction();
$connection->transactional(some function call here);
$connection->commit();

I'm not familiar with nested transactions, but if it's possible to nest them then this will return true because of the outer transaction, although the outer transaction will have been property closed. Also, why not reuse the previous try / finally block just like I did in ORM?

[simPod]

Nested transactions should be covered by 4 tests.

I merged try blocks, it makes sense and is nice with the isTransactionActive API.

[greg0ire]

Now the code looks like it is going to wrongly revert the outer transaction after successfully committing the inner one, just because the outer one is active (as it should)

[simPod]

Umm I'm confused now. Will take a break.

[simPod]

I can add a check of nesting level but the green test suite somehow implies it won't happen at all anyway.

[simPod]

It indeed tried to rollback the transaction. I have reverted to a previous version and added a test to cover the case.

[greg0ire]

I am confused now. Are you saying that the bug exists, and that you reproduced it? I don't see any check on the transaction nesting level, which does sound exactly like what would be needed here, so why is the tests suite mostly green (it's red on Oracle, do you know why?)

[simPod]

it's red on Oracle, do you know why

oracle and db2 require select... to have also from clause. Fixed.

Are you saying that the bug exists, and that you reproduced it

Yes, the bug would have been introduced with 88879b3. I covered it with testNestedTransactionWalkthrough() so we know it passes.

When doing commit(), we need to rollback the transaction only if something failed outside the database #6545 (comment).

Since we need to have rollback() in finally, we need to use boolean flag to know whether to trigger it.
We can't use isTransactionActive() since it only checks the nesting level. isTransactionActive() can actually be renamed to something isTransactionNested().

[greg0ire]

It seems there are CI jobs failing. Please take a look at this guide for more on how to handle those.

[simPod]

Thanks, just did not notice that basic stuff, u were faster 🙃 fixed.

[greg0ire]

If the above lines are successful but something goes wrong here, then $shouldRollback will be true (right?) although no rollback should happen.

That's why I was expecting some code that takes a look at the transaction nesting level before and after the commit. If the transaction level decreased all by itself.

Looking at the code though, I don't see how it could fail, so I guess this is OK.

[greg0ire]

It's unclear to me why you are calling this function inside a finally block. If the commit fails, why should the transaction nesting level decrease?

[simPod]

If the commit fails, why should the transaction nesting level decrease?

We need to cleanup the transaction state. Otherwise isTransactionActive() will still report true to other functions (e.g. in disconnect()) which is not true actually. It is covered by UniqueConstraintViolationsTest.

[greg0ire]

Why should it no longer be considered active? If COMMIT fails, then the transaction will be "aborted", but it won't be auto-rolled-back, right? So to me, I'd say the transaction is still going on, but it is aborted and unusable, and to regain control, you have to use ROLLBACK TO or ROLLBACK.

That's what I understand after reading the last few sentences of https://www.postgresql.org/docs/current/tutorial-transactions.html anyway.

[simPod]

You're right, good point.

I came up with the solution where we can check the exception type and if it is a known case where the transaction is auto rolled back, we won't call ROLLBACK.

36e23d1

[simPod]

yes, of course. I meant "how the wrong rollback" can happen in this code flow.

Let's say the network error happens and the opening of the transaction fails. An exception will bubble up, preventing $successful from switching to true, and there will be an attempt to rollback a transaction that was never happened in the first place. Right?

I think I covered the case where

transactional() is used here

https://github.com/doctrine/dbal/pull/6545/files#diff-a3bde44a40c3047e1a8c316dd4e5dbb1c8f8d169fa9558e01faabbb2a794d58cR400-R440

and where transactional() is not used here

https://github.com/doctrine/dbal/pull/6545/files#diff-a3bde44a40c3047e1a8c316dd4e5dbb1c8f8d169fa9558e01faabbb2a794d58cR459-R486

---

When autocommit=false, then calling transactional() increases nesting level to 2 and this line is never called

therefore, there does not appear to be any attempt to rollback 🤔

[greg0ire]

When autocommit=false, then calling transactional() increases nesting level to 2 and this line is never called
therefore, there does not appear to be any attempt to rollback 🤔

Makes sense to me. What about when autocommit=true? Sorry if this should be obvious to me, but I don't have all the details loaded in mind.

[simPod]

When autocommit=false, then calling transactional() increases nesting level to 2 and this line is never called
therefore, there does not appear to be any attempt to rollback 🤔

Makes sense to me. What about when autocommit=true? Sorry if this should be obvious to me, but I don't have all the details loaded in mind.

Absolutely understand.

I have covered calling transactional() with and without autocommit in those tests.

What about when autocommit=true

When it's true it does not satisfy the left side of the condition in order to rollback

https://github.com/simPod/dbal/blob/e6cf572874bf74f816b94f034be9440aed1c8d76/src/Connection.php#L1482

If it's false it does not satisfy the right side because

When autocommit=false, then calling transactional() increases nesting level to 2 and this line is never called.

[simPod]

I think I have addressed @/derrabus' (not tagging explicitly to avoid noise) comments from previous attempt related to OCI and PDO_OCI #4846 (review)

[greg0ire]

Please use an interactive rebase to make your commits atomic (all commits should be "revertable" / pass the build). Also, if you can split this PR, that would be great.

[simPod]

Yup, though for the review process I wanted the commits to represent addressing the comments / explain the issue.

Will consolidate + continue extracting.