Add ability to specify a transaction mode for SQLite connection #56681
Conversation
|
Thanks for submitting a PR! Note that draft PR's are not reviewed. If you would like a review, please mark your pull request as ready for review in the GitHub user interface. Pull requests that are abandoned in draft may be closed due to inactivity. |
ccae166 to
ba8643d
Compare
|
It seems that my naive approach only works in PHP 8.4, likely due to the introduction of database-specific PDO classes. I suspect that, prior to 8.4, PDO's The above behavior breaks Laravel's transaction management logic if we substitute I don't really see a clean way to work around this. I'll this PR up for a bit in case anyone has any ideas on how this could be achieved. |
|
I have decided to restrict these changes to PHP 8.4 and above, so at least people running the latest version can benefit from this feature. |
There was a problem hiding this comment.
Personally I'd probably hold off on this until we see if php/php-src#19317 makes it into PHP 8.5. If it does, then (at least in PHP 8.5) this feature would be redundant.
If we do want this option (which IMO is quite important) in PHP < 8.5, this PR could be merged.
My article linked in the PR desc shows how this can be worked around at the moment. It is a pretty ugly override:
class SQLiteConnectionOverride extends SQLiteConnection
{
protected function createTransaction()
{
if ($this->transactions == 0) {
$this->reconnectIfMissingConnection();
try {
// Override here
$this->getPdo()->exec('begin immediate transaction');
// Instead of:
// $this->getPdo()->beginTransaction();
} catch (Throwable $e) {
$this->handleBeginTransactionException($e);
}
} elseif ($this->transactions >= 1 && $this->queryGrammar->supportsSavepoints()) {
$this->createSavepoint();
}
}
}Perhaps a good first change here would be to simply extract executeBeginTransactionStatement() to make both user overrides, and default overrides like you're adding here, easy to do.
That would be a reasonable thing to merge now regardless of when my PR lands in php-src.
| if (version_compare(PHP_VERSION, '8.4.0') >= 0) { | ||
| $mode = $this->getConfig('transaction_mode') ?? 'DEFERRED'; | ||
|
|
||
| $this->getPdo()->exec("BEGIN $mode TRANSACTION"); |
There was a problem hiding this comment.
Just for some context, as far as I can tell, there is no issue whatsoever with using a manual exec("BEGIN ...") instead of beginTransaction() with Pdo\Sqlite. They are equivalent.
PDO has some extra logic for tracking whether a connection is in a transaction, but it looks like this:
static bool pdo_is_in_transaction(pdo_dbh_t *dbh) {
if (dbh->methods->in_transaction) {
return dbh->methods->in_transaction(dbh);
}
return dbh->in_txn;
}Bypassing beginTransaction() bypasses setting in_txn = true. However, the SQLite driver does implement in_transaction, so that serves as the source of truth for whether the connection is in a transaction.
Simple test:
$ cat /tmp/intxn.php
<?php
$pdo = PDO::connect('sqlite::memory:');
$pdo->beginTransaction();
var_dump($pdo->inTransaction());
$pdo->rollBack();
var_dump($pdo->inTransaction());
$pdo->exec('begin transaction');
var_dump($pdo->inTransaction());
$ php /tmp/intxn.php
bool(true)
bool(false)
bool(true)
There was a problem hiding this comment.
I believe if you run this same script in PHP 8.3 and below you will get
bool(true)
bool(false)
bool(false)
There was a problem hiding this comment.
Nice catch. Seems this behavior was fixed here php/php-src#14268, so it's PHP 8.4 only yeah.
There was a problem hiding this comment.
Right I see your comment above now, missed that before sorry.
I created this PR partly due to the fact that by this point the linked fix for PHP 8.5 seems like it won't land in the next release, since the release managers have already started publishing beta builds. |
|
From my understanding if the chosen implementation doesn't require an RFC it can be added late in the release cycle. Either way, older PHP versions will remain widely used for years, so having a workaround for this in Laravel might make sense depending on what Taylor thinks about this. |
|
Tbh with the restriction to PHP 8.4 I'm less sure if we want this in the current form (assuming my PR makes it into PHP 8.5). Then we'd have the changes here usable just for PHP 8.4, and redundant in the very next release. So simply extracting the method for beginning a transaction, with people using custom overrides, may really be the way to go. Once the PDO attribute lands in PHP there'd be two ways of configuring transaction modes in |
This PR adds the ability to specify which mode SQLite should use for running transactions. This is done via an additional config option in
database.php.I've opted for the current implementation because PDO currently lacks the ability specify a transaction mode when running
$pdo->beginTransaction(). We have to work around this by manually running aBEGIN $MODE TRANSACTIONstatement.Justification:
By default, SQLite transactions run in DEFERRED mode, which causes problems (namely, a bunch of SQLITE_BUSY errors) when running in WAL mode. The recommended production setup for SQLite is WAL mode paired with IMMEDIATE transactions. This article has a good breakdown of the problem specifically with Laravel. In short, currently running an SQLite database in production with database driven queues and cache will result in many logged
Database is lockederrors in your logs and some failed jobs. One could reproduce this by, e.g. starting a few queue workers in multiple terminals and dispatching 100 jobs to the queue.I wanted to include tests as well, but I'm not sure how to test which statement runs when beginning transactions. Initially I planned to use the built in query logging, but discovered that transaction statements are not included in the query log, which makes sense.