Leaving the savepoint in the transaction stack upon rollback is incorrect

Adding an additional test case, repeated rollback.

I'm a little confused by the idea that the changes in 🐛 [PP-1] Rolling back to a savepoint should leave the savepoint in the transaction stack Postponed were "partially incorrect". Doesn't this issue completely revert the nature of the changes made there?

I think this fix is correct but I think that we should have a re-read of the docs that 🐛 [PP-1] Rolling back to a savepoint should leave the savepoint in the transaction stack Postponed was based on

The ROLLBACK TO SAVEPOINT statement rolls back a transaction to the named savepoint without terminating the transaction. Modifications that the current transaction made to rows after the savepoint was set are undone in the rollback, but InnoDB does not release the row locks that were stored in memory after the savepoint. (For a new inserted row, the lock information is carried by the transaction ID stored in the row; the lock is not separately stored in memory. In this case, the row lock is released in the undo.) Savepoints that were set at a later time than the named savepoint are deleted.

We specifically prevent Savepoints that were set at a later time than the named savepoint are deleted. from happening and we throw out of order errors. Are we sure the behaviour we've implemented here is correct. I'm happy to leave this unresolved here as I think this issue is returning us to the behaviour before all the recent transaction changes.

Removed 'partially' from the IS.

What we do here is revert the behaviour that the savepoint rolled back to was left alive, both on the DB and as an item on the TransactionManager stack. After this patch, we RELEASE SAVEPOINT the rolled back savepoint on the DB, and void the item on the TransactionManager so it cannot be committed or rolled back again.

As far as I can remember, I was quite confused by Drupal logic - you could commit any savepoint/root at discretion, but you only could rollback LIFO, i.e. without breaking the order. However, I made efforts to keep this logic in the TransactionManager, too.

On this basis,

We specifically prevent Savepoints that were set at a later time than the named savepoint are deleted. from happening and we throw out of order errors. Are we sure the behaviour we've implemented here is correct.

I think (hope) it's correct. It's implicit in the LIFO behavior. It's now covered by DriverSpecificTransactionTestBase::testRollbackSavepointWithLaterSavepoint.

Happy to loosen the LIFO behaviour if I was mistaken, and allow rolling back to savepoint/root coming earlier than the last active, if that's the correct expected behavior. It was a grey zone for me, but I remember I found barriers in the previous testing that were indicating to me strict LIFO.

Let's do this fix as it is really important. I think we can think about loosening the LIFO behaviour in a future issue.

Thanks. Filed 📌 Loosen TransactionManager LIFO behaviour on rollback Active as a follow-up.

Committed/pushed to 11.x and cherry-picked to 10.2.x, thanks!

mondrake → closed merge request !5743

Automatically closed - issue fixed for 2 weeks with no activity.