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

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

Open
wants to merge 16 commits into
base: 4.6
Choose a base branch
from

Conversation

mkuettel
Copy link

@mkuettel mkuettel commented Apr 27, 2024

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.

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 mkuettel force-pushed the sqlite3-nested-transactions branch 2 times, most recently from 4b94ec2 to 753765d Compare April 27, 2024 18:13
@mkuettel mkuettel changed the title feat: SQLite3: add support for nested transactions using savepoints. feat(sqlite3): add support for nested transactions using savepoints. Apr 27, 2024
@mkuettel
Copy link
Author

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?

@mkuettel mkuettel force-pushed the sqlite3-nested-transactions branch 3 times, most recently from 367d5d9 to 47db608 Compare April 27, 2024 21:07
@kenjis kenjis added wrong branch PRs sent to wrong branch enhancement PRs that improve existing functionalities database Issues or pull requests that affect the database layer breaking change Pull requests that may break existing functionalities labels Apr 27, 2024
@kenjis
Copy link
Member

kenjis commented Apr 27, 2024

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
Copy link
Author

mkuettel commented Apr 28, 2024

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
Copy link
Author

mkuettel commented Apr 28, 2024

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 mkuettel marked this pull request as draft April 28, 2024 11:56
@mkuettel mkuettel changed the title feat(sqlite3): add support for nested transactions using savepoints. feat(database): add support for nested transactions using savepoints for SQLite3 + others Apr 28, 2024
@mkuettel mkuettel changed the base branch from develop to 4.6 April 28, 2024 20:02
@mkuettel
Copy link
Author

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
Copy link
Author

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

@mkuettel mkuettel marked this pull request as ready for review April 28, 2024 20:07
@mkuettel mkuettel changed the title feat(database): add support for nested transactions using savepoints for SQLite3 + others feat(database): add support for nested transactions using savepoints Apr 28, 2024
@kenjis kenjis removed wrong branch PRs sent to wrong branch breaking change Pull requests that may break existing functionalities labels Apr 29, 2024
@mkuettel
Copy link
Author

whoops, missed the MockConnection

@mkuettel
Copy link
Author

this should fix most if not all of the linting mistakes

@ddevsr ddevsr added the 4.6 label Apr 29, 2024
app/Config/Database.php Outdated Show resolved Hide resolved
@mkuettel mkuettel force-pushed the sqlite3-nested-transactions branch from 82c06cd to 2f4bd3a Compare April 29, 2024 12:44
@mkuettel mkuettel requested a review from kenjis April 29, 2024 12:44
@mkuettel
Copy link
Author

mkuettel commented Apr 29, 2024

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
Copy link
Author

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.

@mkuettel mkuettel force-pushed the sqlite3-nested-transactions branch from 0e1cdd4 to ed7127b Compare April 30, 2024 21:15
@kenjis
Copy link
Member

kenjis commented May 1, 2024

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
Copy link
Member

kenjis commented May 1, 2024

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.

Test whether we can start two nested transactions ...

1. ... complete them and see the result in the database
2. ... rollback the outer one after completing the inner one,
   and see that the result is _not_ in the database.
3. ... roll back the inner one and see if the result
      of the outer one is in the database after completing.
This option will be false by default, so the previous documented
behaviour of CodeIgniter gets kept & to avoid introducing a breaking
change.

CodeIgniter allows for nesting transactions, but only ever cared about,
actually committing/rolling back the most outer one. This commit keeps
this behaviour by default, but adds the database driver option
'enableNestedTransactions' to the BaseConnection.

When set to true, codeigniter now will call
_transBeginNested()/_transCommitNested()/_transRollbackNested() on the
respective driver implemented, when further transactions are nested into
the outermost transaction.
By using SAVEPOINT SQL statements to manage transactions deeper than 1 level,
it's possible to provide full nested transaction semantics for the
following drivers:

- SQLite3:
  - https://sqlite.org/lang_transaction.html
  - Savepoints: https://sqlite.org/lang_savepoint.html
- MySQLi
  - https://mariadb.com/kb/en/savepoint/
- Postgre
  - https://www.postgresql.org/docs/current/sql-savepoint.html

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.

This feature is currently disabled for SQLSrv & OCI8 and they keep on.
working with just 1 transaction layer & nested ones getting ignored.

Revert "feat(sqlite3): add support for nested transactions using SAVEPOINT"

This reverts commit 47db608.

feat(database): use savepoints for nested transactions for MySQLi, SQLite3 + Postgre driver
@mkuettel mkuettel force-pushed the sqlite3-nested-transactions branch from 1254e83 to 6970c48 Compare May 16, 2024 19:24
@mkuettel
Copy link
Author

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 github-actions bot added the stale Pull requests with conflicts label May 23, 2024
Copy link

👋 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

@kenjis kenjis added the docs needed Pull requests needing documentation write-ups and/or revisions. label Jun 10, 2024
@kenjis
Copy link
Member

kenjis commented Jun 10, 2024

This branch has conflicts. Please resolve.
And can you update the user guide for this feature?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.6 database Issues or pull requests that affect the database layer docs needed Pull requests needing documentation write-ups and/or revisions. enhancement PRs that improve existing functionalities stale Pull requests with conflicts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants