feat(database): add support for nested transactions using savepoints

[mkuettel]

It's possible to provide nested transaction semantics when using SQLite3
by using SAVEPOINT statements to manage transactions deeper than 1 level.

The implementation starts a transaction when transBegin() is first called as before.
But then all nested transactions will be managed by creating,
releasing (commiting) or rolling back savepoint.
Only when the outermost transaction is completed a COMMIT or ROLLBACK
will be executed, either committing the transaction and all inner
committed transactions, or rolling everything back.

• SQLite Transaction: https://sqlite.org/lang_transaction.html
• SQLite Savepoints: https://sqlite.org/lang_savepoint.html

Description
Explain what you have changed, and why.

Checklist:

• Securely signed commits
• Component(s) with PHPDoc blocks, only if necessary or adds value
• Unit testing, with >80% coverage
• User guide updated
• Conforms to style guide

[mkuettel]

Maybe this should be targeted at 4.6, but IMHO since it fixes nested transactions for when using phpunit which uses the SQLite3 driver by default, and nested transactions are clearly documented, this may also be considered a fix?

[kenjis]

Thank you for sending this PR.

This is not a bug fix. Read the documentation. The current behavior is clearly documented.

In CodeIgniter, transactions can be nested in a way such that only the outmost or top-level transaction commands are executed.
https://codeigniter4.github.io/CodeIgniter4/database/transactions.html#nested-transactions

The problems:

1. This is an enhancement. All PRs with enhancements should go to the next minor branch (4.6 in this moment).
2. This changes the current behavior, so it might break existing apps.
3. The transaction behavior should be the exactly same in all database drivers. Otherwise, if we change the DB driver, the app would break. This only changes the behavior in SQLite3.
4. Some checks in GitHub Actions fail.

Please fix the problems. If you have any questions, feel free to ask.

[mkuettel]

Ah thanks for your quick and detailed feedback! I clearly overread the important part here with the outermost transaction.
That basically means, "Yes you can nest transactions, but we don't care about the inner ones, we just keep track."

I'll definitly rebase and target 4.6 now.

I also tested the MySQLi driver with mariadb on debian bookworm with the same SAVEPOINT (https://mariadb.com/kb/en/savepoint/) Queries as above, and they seem to be compatible. I was thinking about creating a trait like SavepointsForNestedTransactions, to avoid duplication.
Or do you like the redudancy in this case, because of weird side cases that might appear?

In order not to introduce a breaking change and be backwards-compatible, should I add a configuration option like $nestedTransactions = [DISABLED|SAVEPOINT|OTHER....] to the Database Config?
Or do prefer not cluttering the config? Or do you have other suggestions to mitigate the problem of breaking existing apps with this?

Or do you prefer not cluttering?

Thanks for your help!

[mkuettel]

I might be able to implement this as well for PostgreSQL which seems to suppor the same savepoint syntax, but I have no access or experience to the other database systems.

https://www.postgresql.org/docs/current/sql-savepoint.html

[mkuettel]

I've now implemented most of this on the BaseConnection directly, while also providing a configuration option with the very verbose name enableNestedTransactions.

Nested transactions should now be supported by mysqli, sqlite3 as well as the postgre driver

[mkuettel]

I'll now wait for some feedback, before cleaning up everything to hopefully make it ready to merge.

[mkuettel]

whoops, missed the MockConnection

[mkuettel]

this should fix most if not all of the linting mistakes

[kenjis]

I'm not sure this config key is good for this change. I would like to hear others' opinions. > @codeigniter4/core-team @codeigniter4/database-team

Because we have already the transException() method to change CI4's transaction behavior.
https://codeigniter4.github.io/CodeIgniter4/database/transactions.html#throwing-exceptions

[michalsn]

Such a change is indeed problematic. I don't know how to reconcile exception handling with savepoints.

Anyway, maybe we should use something like enableSavepointTransactions? Also, can we refer users to the same section in the user guide describing the differences between nested transactions and save point transactions?

Last thing. I haven't checked from which versions, but SQLSRV and OCI8 also support savepoints - possibly with different syntax, but that doesn't change the fact that support should be provided.

[mkuettel]

Yes that one came to mind too. In the same vein I personally don't like the current behaviour of BaseConnection::query, which rerolls ALL (outer) transactions when a single nested query fails, when DBDebug = true + transException is set:
https://github.com/codeigniter4/CodeIgniter4/blob/develop/system/Database/BaseConnection.php#L658-L670

I don't like it as when the exception gets passed on, it might still be caught by code calling query() and get rerolled there.

If the point is to make sure we don't leave transactions in limbo, we should do this on a cleanup step (__destruct() or something).

I'm currently using a bad hack + another implementation of this PR & seperate driver classes with savepoint support for MySQLi + SQLite
in my project https://github.com/mkuettel/codeigniter4-services/blob/main/src/TransactionService.php#L55

There i set transDepth to 0 using reflection to after starting a transaction to avoid the complete rollback behaviour in the while loop with DBDebug = true + transException = true.

[mkuettel]

Addionally, I actually like having DBDebug on, so more exceptions are thrown generally, so database errors not just get ignored, or the database even gets in a weird unexpected state.

[mkuettel]

@michalsn What am I missing regarding the differences between nested transactions and savepoints, https://sqlite.org/lang_savepoint.html, look for "There are several ways of thinking about the RELEASE command", the first one sounds like a nested transaction, the second one is the way i like to think about it. Also they seem to be equivalent in some way?

Regarding SQLsrv + OCI8, I don't have access to those systems (except here in CI, when you guys run it), so this will be very labourious and spammy in here.

[kenjis]

We have docker-compose.yaml for all supported databases:
https://github.com/codeigniter4/devkit/tree/develop/src/docker

[mkuettel]

I've now added some more tests and the transNested function (better names are welcome), which sets enableNestedTransactions. I'd like to make it private, but I want to be able to write to this flag from the driver classes to disable it.

Although i removed the setting, because the BaseConnection just checks whether a property exists, its still settable via a BaseConnection option as before. If this is not desired, how should I avoid this?

Additionally I deliberately only talk about transaction nesting instead of savepoints in the Context of BaseConnection as of now, as it technically doesn't need to know how nesting is implemented. Is this useful or just nonsense?

Some of the tests may not check for the correct behaviour, regarding error handling and exception handling in the context of transactions.

What should the interactions between DBDebug, transException and transNest look like?

Addtionally there are probably some more details with each driver automatically
rolling back automatically when an error occurs. For example in SQLite3 we should check autocommit after an error to know if our current transDepth should be reset to 0.

[mkuettel]

I've added some fixes for the tests and I'm getting a clearer picture now.

But I've been thinking about another thing: strict mode. Strict mode is turned on by default, which causes everything to be rolled back when it is enabled as soon as a single transaction (even nested at a deep level) causes all future ones to be rolled back.

Is the solution to apply strict to a transaction layer, meaning that future transactions in the same or lower level automatically fail, when it is set?

We'd have to keep track of transStatus (+ maybe transFailure) as well as the transStrict setting on a per "transaction layer" basis.

[kenjis]

Although i removed the setting, because the BaseConnection just checks whether a property exists, its still settable via a BaseConnection option as before. If this is not desired, how should I avoid this?

I think we don't need to avoid it.
The option enableNestedTransactions is (will be) not documented. It is not an official config item.
If a dev deliberately adds it in their DB config, they know what they do.

[kenjis]

What should the interactions between DBDebug, transException and transNest look like?

It is recommended to always set DBDebug to true.

During transactions, by default CI4 does not throw exceptions.
But if transException is true, it throws exceptions.
Enabling transException is not recommended.

[mkuettel]

Thanks a lot for your clarifications, this helps a lot.

I've added Savepoints to the missing drivers OCI8 and SQLSRV as well.
Then I've renamed some stuff and rebased upon current 4.6.

I'd be glad if you could run the workflows again, to know what to fix.

[github-actions]

👋 Hi, @mkuettel!

We detected conflicts in your PR against the base branch 🙊
You may want to sync 🔄 your branch with upstream!

Ref: Syncing Your Branch