Implement GH-8967: Add PDO_SQLITE_ATTR_TRANSACTION_MODE #19317
+169
−3
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
stancl
commented
Jul 30, 2025
Comment on lines
+260
to
+263
| case 0: | ||
| case PDO_SQLITE_ATTR_TRANSACTION_MODE_DEFERRED: | ||
| begin_statement = "BEGIN DEFERRED TRANSACTION"; | ||
| break; |
There was a problem hiding this comment.
The 0 case is when no transaction mode has been set, so we use the default (deferred).
Is it a safe assumption that driver_data is always zero-initialized? In the code I could find, I saw that driver_data is allocated with pecalloc().
stancl
commented
Jul 30, 2025
ext/pdo_sqlite/sqlite_driver.c
Outdated
Comment on lines
311
to
319
| case PDO_SQLITE_ATTR_TRANSACTION_MODE: | ||
| zend_long mode; | ||
| if (H->transaction_mode == 0) { | ||
| mode = PDO_SQLITE_ATTR_TRANSACTION_MODE_DEFERRED; | ||
| } else { | ||
| mode = H->transaction_mode; | ||
| } | ||
| ZVAL_LONG(return_value, mode); | ||
| break; |
There was a problem hiding this comment.
Same as my previous review https://github.com/php/php-src/pull/19317/files#r2243162264, this depends on driver_data being zero-initialized.
|
@kocsismate @SakiTakamachi Will this make it into 8.5 if merged before the feature freeze? |
|
@stancl perhaps this is worth posting on the PHP Internals mailing list? I would very much like for this feature to land in the next release, and I think this is a marked improvement to PDO. |
|
Someone on the mailing list suggested using an enum (presumably meaning a PHP enum) to represent the values of this attribute. That would solve my issue of figuring out how to represent these values nicely, rather than using additional attributes: namespace Pdo {
enum SqliteTransactionMode {
case Deferred;
case Immediate;
case Exclusive;
}
}One thing I don't love about this approach is that we get an enum directly in the diff --git a/ext/pdo_sqlite/pdo_sqlite.c b/ext/pdo_sqlite/pdo_sqlite.c
index 023e35a2bc3..90b48d3525a 100644
--- a/ext/pdo_sqlite/pdo_sqlite.c
+++ b/ext/pdo_sqlite/pdo_sqlite.c
@@ -27,10 +27,13 @@
#include "php_pdo_sqlite.h"
#include "php_pdo_sqlite_int.h"
#include "zend_exceptions.h"
+#include "zend_enum.h"
#include "pdo_sqlite_arginfo.h"
static zend_class_entry *pdosqlite_ce;
+zend_class_entry *pdosqlite_transaction_mode_ce;
+
/* {{{ pdo_sqlite_deps */
static const zend_module_dep pdo_sqlite_deps[] = {
ZEND_MOD_REQUIRED("pdo")
@@ -421,6 +424,8 @@ PHP_MINIT_FUNCTION(pdo_sqlite)
pdosqlite_ce = register_class_Pdo_Sqlite(pdo_dbh_ce);
pdosqlite_ce->create_object = pdo_dbh_new;
+ pdosqlite_transaction_mode_ce = register_class_Pdo_SqliteTransactionMode();
+
if (php_pdo_register_driver(&pdo_sqlite_driver) == FAILURE) {
return FAILURE;
}
diff --git a/ext/pdo_sqlite/pdo_sqlite.stub.php b/ext/pdo_sqlite/pdo_sqlite.stub.php
index ffb400ac34f..04ec0e327eb 100644
--- a/ext/pdo_sqlite/pdo_sqlite.stub.php
+++ b/ext/pdo_sqlite/pdo_sqlite.stub.php
@@ -42,15 +42,6 @@ class Sqlite extends \PDO
/** @cvalue PDO_SQLITE_ATTR_TRANSACTION_MODE */
public const int ATTR_TRANSACTION_MODE = UNKNOWN;
- /** @cvalue PDO_SQLITE_ATTR_TRANSACTION_MODE_DEFERRED */
- public const int ATTR_TRANSACTION_MODE_DEFERRED = UNKNOWN;
-
- /** @cvalue PDO_SQLITE_ATTR_TRANSACTION_MODE_IMMEDIATE */
- public const int ATTR_TRANSACTION_MODE_IMMEDIATE = UNKNOWN;
-
- /** @cvalue PDO_SQLITE_ATTR_TRANSACTION_MODE_EXCLUSIVE */
- public const int ATTR_TRANSACTION_MODE_EXCLUSIVE = UNKNOWN;
-
#if SQLITE_VERSION_NUMBER >= 3043000
public const int EXPLAIN_MODE_PREPARED = 0;
public const int EXPLAIN_MODE_EXPLAIN = 1;
@@ -100,3 +91,9 @@ public function openBlob(
public function setAuthorizer(?callable $callback): void {}
}
+
+enum SqliteTransactionMode {
+ case Deferred;
+ case Immediate;
+ case Exclusive;
+}
diff --git a/ext/pdo_sqlite/pdo_sqlite_arginfo.h b/ext/pdo_sqlite/pdo_sqlite_arginfo.h
index 6ed5081f2ce..e20fae892fa 100644
Binary files a/ext/pdo_sqlite/pdo_sqlite_arginfo.h and b/ext/pdo_sqlite/pdo_sqlite_arginfo.h differ
diff --git a/ext/pdo_sqlite/php_pdo_sqlite.h b/ext/pdo_sqlite/php_pdo_sqlite.h
index 1bba9e3556a..7e72b667a60 100644
--- a/ext/pdo_sqlite/php_pdo_sqlite.h
+++ b/ext/pdo_sqlite/php_pdo_sqlite.h
@@ -18,6 +18,7 @@
#define PHP_PDO_SQLITE_H
extern zend_module_entry pdo_sqlite_module_entry;
+extern zend_class_entry *pdosqlite_transaction_mode_ce;
#define phpext_pdo_sqlite_ptr &pdo_sqlite_module_entry
#include "php_version.h"
diff --git a/ext/pdo_sqlite/php_pdo_sqlite_int.h b/ext/pdo_sqlite/php_pdo_sqlite_int.h
index cfaa7a85b13..a71f506f576 100644
--- a/ext/pdo_sqlite/php_pdo_sqlite_int.h
+++ b/ext/pdo_sqlite/php_pdo_sqlite_int.h
@@ -78,9 +78,12 @@ enum {
PDO_SQLITE_ATTR_BUSY_STATEMENT,
PDO_SQLITE_ATTR_EXPLAIN_STATEMENT,
PDO_SQLITE_ATTR_TRANSACTION_MODE,
- PDO_SQLITE_ATTR_TRANSACTION_MODE_DEFERRED,
- PDO_SQLITE_ATTR_TRANSACTION_MODE_IMMEDIATE,
- PDO_SQLITE_ATTR_TRANSACTION_MODE_EXCLUSIVE
+};
+
+enum {
+ PDO_SQLITE_TRANSACTION_MODE_DEFERRED = 0,
+ PDO_SQLITE_TRANSACTION_MODE_IMMEDIATE,
+ PDO_SQLITE_TRANSACTION_MODE_EXCLUSIVE,
};
typedef int pdo_sqlite_create_collation_callback(void*, int, const void*, int, const void*);
diff --git a/ext/pdo_sqlite/sqlite_driver.c b/ext/pdo_sqlite/sqlite_driver.c
index 2c6749fc7f4..eb2cf2a1c33 100644
--- a/ext/pdo_sqlite/sqlite_driver.c
+++ b/ext/pdo_sqlite/sqlite_driver.c
@@ -26,6 +26,7 @@
#include "php_pdo_sqlite.h"
#include "php_pdo_sqlite_int.h"
#include "zend_exceptions.h"
+#include "zend_enum.h"
#include "sqlite_driver_arginfo.h"
int _pdo_sqlite_error(pdo_dbh_t *dbh, pdo_stmt_t *stmt, const char *file, int line) /* {{{ */
@@ -257,14 +258,13 @@ static bool sqlite_handle_begin(pdo_dbh_t *dbh)
char *begin_statement;
switch (H->transaction_mode) {
- case 0:
- case PDO_SQLITE_ATTR_TRANSACTION_MODE_DEFERRED:
+ case PDO_SQLITE_TRANSACTION_MODE_DEFERRED:
begin_statement = "BEGIN DEFERRED TRANSACTION";
break;
- case PDO_SQLITE_ATTR_TRANSACTION_MODE_IMMEDIATE:
+ case PDO_SQLITE_TRANSACTION_MODE_IMMEDIATE:
begin_statement = "BEGIN IMMEDIATE TRANSACTION";
break;
- case PDO_SQLITE_ATTR_TRANSACTION_MODE_EXCLUSIVE:
+ case PDO_SQLITE_TRANSACTION_MODE_EXCLUSIVE:
begin_statement = "BEGIN EXCLUSIVE TRANSACTION";
break;
default:
@@ -303,7 +303,7 @@ static bool sqlite_handle_rollback(pdo_dbh_t *dbh)
static int pdo_sqlite_get_attribute(pdo_dbh_t *dbh, zend_long attr, zval *return_value)
{
pdo_sqlite_db_handle *H = (pdo_sqlite_db_handle *)dbh->driver_data;
- zend_long transaction_mode;
+ char *transaction_mode = "Deferred";
switch (attr) {
case PDO_ATTR_CLIENT_VERSION:
@@ -311,12 +311,18 @@ static int pdo_sqlite_get_attribute(pdo_dbh_t *dbh, zend_long attr, zval *return
ZVAL_STRING(return_value, (char *)sqlite3_libversion());
break;
case PDO_SQLITE_ATTR_TRANSACTION_MODE:
- if (H->transaction_mode == 0) {
- transaction_mode = PDO_SQLITE_ATTR_TRANSACTION_MODE_DEFERRED;
- } else {
- transaction_mode = H->transaction_mode;
+ switch (H->transaction_mode) {
+ case PDO_SQLITE_TRANSACTION_MODE_DEFERRED:
+ transaction_mode = "Deferred";
+ break;
+ case PDO_SQLITE_TRANSACTION_MODE_IMMEDIATE:
+ transaction_mode = "Immediate";
+ break;
+ case PDO_SQLITE_TRANSACTION_MODE_EXCLUSIVE:
+ transaction_mode = "Exclusive";
+ break;
}
- ZVAL_LONG(return_value, transaction_mode);
+ ZVAL_OBJ_COPY(return_value, zend_enum_get_case_cstr(pdosqlite_transaction_mode_ce, transaction_mode));
break;
default:
@@ -354,15 +360,22 @@ static bool pdo_sqlite_set_attr(pdo_dbh_t *dbh, zend_long attr, zval *val)
sqlite3_extended_result_codes(H->db, lval);
return true;
case PDO_SQLITE_ATTR_TRANSACTION_MODE:
- if (!pdo_get_long_param(&lval, val)) {
+ if (Z_TYPE_P(val) != IS_OBJECT || Z_OBJCE_P(val) != pdosqlite_transaction_mode_ce) {
return false;
}
- if (lval != PDO_SQLITE_ATTR_TRANSACTION_MODE_DEFERRED &&
- lval != PDO_SQLITE_ATTR_TRANSACTION_MODE_IMMEDIATE &&
- lval != PDO_SQLITE_ATTR_TRANSACTION_MODE_EXCLUSIVE) {
- return false;
+ switch (ZSTR_VAL(Z_STR_P(zend_enum_fetch_case_name(Z_OBJ_P(val))))[0]) {
+ case 'D':
+ H->transaction_mode = PDO_SQLITE_TRANSACTION_MODE_DEFERRED;
+ break;
+ case 'I':
+ H->transaction_mode = PDO_SQLITE_TRANSACTION_MODE_IMMEDIATE;
+ break;
+ case 'E':
+ H->transaction_mode = PDO_SQLITE_TRANSACTION_MODE_EXCLUSIVE;
+ break;
+ default:
+ return false;
}
- H->transaction_mode = lval;
return true;
}
diff --git a/ext/pdo_sqlite/tests/subclasses/pdo_sqlite_transaction_mode.phpt b/ext/pdo_sqlite/tests/subclasses/pdo_sqlite_transaction_mode.phpt
index c17838a67ce..0286a0ac66a 100644
--- a/ext/pdo_sqlite/tests/subclasses/pdo_sqlite_transaction_mode.phpt
+++ b/ext/pdo_sqlite/tests/subclasses/pdo_sqlite_transaction_mode.phpt
@@ -10,32 +10,27 @@
$pdo2 = PDO::connect($dsn);
// Deferred by default before any transaction mode is set
-var_dump($pdo->getAttribute(PDO\Sqlite::ATTR_TRANSACTION_MODE) === PDO\Sqlite::ATTR_TRANSACTION_MODE_DEFERRED);
+var_dump($pdo->getAttribute(PDO\Sqlite::ATTR_TRANSACTION_MODE) === PDO\SqliteTransactionMode::Deferred);
// Both should return true
-var_dump($pdo->setAttribute(PDO\Sqlite::ATTR_TRANSACTION_MODE, PDO\Sqlite::ATTR_TRANSACTION_MODE_DEFERRED));
-var_dump($pdo->getAttribute(PDO\Sqlite::ATTR_TRANSACTION_MODE) === PDO\Sqlite::ATTR_TRANSACTION_MODE_DEFERRED);
+var_dump($pdo->setAttribute(PDO\Sqlite::ATTR_TRANSACTION_MODE, PDO\SqliteTransactionMode::Deferred));
+var_dump($pdo->getAttribute(PDO\Sqlite::ATTR_TRANSACTION_MODE) === PDO\SqliteTransactionMode::Deferred);
// Both should return true
-var_dump($pdo->setAttribute(PDO\Sqlite::ATTR_TRANSACTION_MODE, PDO\Sqlite::ATTR_TRANSACTION_MODE_IMMEDIATE));
-var_dump($pdo->getAttribute(PDO\Sqlite::ATTR_TRANSACTION_MODE) === PDO\Sqlite::ATTR_TRANSACTION_MODE_IMMEDIATE);
+var_dump($pdo->setAttribute(PDO\Sqlite::ATTR_TRANSACTION_MODE, PDO\SqliteTransactionMode::Immediate));
+var_dump($pdo->getAttribute(PDO\Sqlite::ATTR_TRANSACTION_MODE) === PDO\SqliteTransactionMode::Immediate);
// Both should return true
-var_dump($pdo->setAttribute(PDO\Sqlite::ATTR_TRANSACTION_MODE, PDO\Sqlite::ATTR_TRANSACTION_MODE_EXCLUSIVE));
-var_dump($pdo->getAttribute(PDO\Sqlite::ATTR_TRANSACTION_MODE) === PDO\Sqlite::ATTR_TRANSACTION_MODE_EXCLUSIVE);
+var_dump($pdo->setAttribute(PDO\Sqlite::ATTR_TRANSACTION_MODE, PDO\SqliteTransactionMode::Exclusive));
+var_dump($pdo->getAttribute(PDO\Sqlite::ATTR_TRANSACTION_MODE) === PDO\SqliteTransactionMode::Exclusive);
// Cannot set invalid values
var_dump($pdo->setAttribute(PDO\Sqlite::ATTR_TRANSACTION_MODE, 0));
-var_dump($pdo->setAttribute(PDO\Sqlite::ATTR_TRANSACTION_MODE, 1));
-var_dump($pdo->setAttribute(PDO\Sqlite::ATTR_TRANSACTION_MODE, 123));
-
-// Cannot use these as keys, only as values. These should return false
-var_dump($pdo->setAttribute(PDO\Sqlite::ATTR_TRANSACTION_MODE_DEFERRED, true));
-var_dump($pdo->setAttribute(PDO\Sqlite::ATTR_TRANSACTION_MODE_IMMEDIATE, true));
-var_dump($pdo->setAttribute(PDO\Sqlite::ATTR_TRANSACTION_MODE_EXCLUSIVE, true));
+var_dump($pdo->setAttribute(PDO\Sqlite::ATTR_TRANSACTION_MODE, RoundingMode::HalfOdd));
+var_dump($pdo->setAttribute(PDO\Sqlite::ATTR_TRANSACTION_MODE, new class {}));
// Set $pdo to deferred, try to get immediate transaction in $pdo2. There should be no lock contention
-$pdo->setAttribute(PDO\Sqlite::ATTR_TRANSACTION_MODE, PDO\Sqlite::ATTR_TRANSACTION_MODE_DEFERRED);
+$pdo->setAttribute(PDO\Sqlite::ATTR_TRANSACTION_MODE, PDO\SqliteTransactionMode::Deferred);
$pdo->beginTransaction();
try {
$pdo2->exec('begin immediate transaction');
@@ -47,7 +42,7 @@
$pdo->rollBack();
// Set $pdo to immediate, try to get immediate transaction in $pdo2. There SHOULD be lock contention
-$pdo->setAttribute(PDO\Sqlite::ATTR_TRANSACTION_MODE, PDO\Sqlite::ATTR_TRANSACTION_MODE_IMMEDIATE);
+$pdo->setAttribute(PDO\Sqlite::ATTR_TRANSACTION_MODE, PDO\SqliteTransactionMode::Immediate);
$pdo->beginTransaction();
try {
$pdo2->exec('begin immediate transaction');
@@ -58,7 +53,7 @@
$pdo->rollBack();
// Set $pdo to exclusive, try to get immediate transaction in $pdo2. There SHOULD be lock contention
-$pdo->setAttribute(PDO\Sqlite::ATTR_TRANSACTION_MODE, PDO\Sqlite::ATTR_TRANSACTION_MODE_EXCLUSIVE);
+$pdo->setAttribute(PDO\Sqlite::ATTR_TRANSACTION_MODE, PDO\SqliteTransactionMode::Exclusive);
$pdo->beginTransaction();
try {
$pdo2->exec('begin immediate transaction');
@@ -78,9 +73,6 @@
bool(false)
bool(false)
bool(false)
-bool(false)
-bool(false)
-bool(false)
Database is not locked
Database is locked: SQLSTATE[HY000]: General error: 6 database table is locked
Database is locked: SQLSTATE[HY000]: General error: 6 database schema is locked: main |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves #8967
First time contributing to this repo, so please review carefully. Especially the use of zvals and the placement of new constants.
This PR lets the user select which transaction mode
Pdo\Sqliteshould be using. When a transaction mode is not specified, SQLite defaults to DEFERRED transactions which cause issues with locks in concurrent requests. See https://www.sqlite.org/lang_transaction.htmlWhen you start a deferred transaction, the lock is only acquired when you actually interact with the database. If you have a transaction that first reads and only then tries to write, it will start by acquiring a shared lock and only when you try to write does it try to upgrade to an exclusive lock. However, this logic of upgrading locks doesn't really respect the
busy_timeoutpragma, so it just immediately fails withSQLITE_BUSY.On the other hand, when you start an immediate transaction, you don't have to worry about getting
SQLITE_BUSYunless another transaction holds a lock for longer than thebusy_timeout. So the new transaction just waits for a bit rather than immediately failing. This is the behavior you want in production.If you look at the linked issue, I include a simple reproduction using
pcntl_fork()that uses two concurrent processes. If you run the reproduction just asphp test.php, one of the transactions fails withSQLITE_BUSY. If you run it asphp test.php immediate, the code does not use$pdo->beginTransaction()and instead uses$pdo->exec('begin immediate transaction'). With that, the second transaction simply waits for the first one to release the lock.The problem with starting transactions using a custom statement you
exec()is that this is simply not an option in many cases, frameworks prefer calling$pdo->beginTransaction(). I've had to solve this in an application of mine by creating a wrapper around the framework's transaction function that first writes to a dummy table and only then executes the callback. That way the transaction always starts with a write lock and doesn't run into the issue of trying to promote a lock. But this doesn't work in every scenario, for instance when the framework's own logic heavily uses transactions that you have no way of customizing with your own wrappers. Not like the wrappers are a proper solution anyway.PDO attributes are a perfect fit for this because you can simply set the attribute and no other logic has to change. Most frameworks let you set these in a config file as well.
The only thing I'm not sure about is how to structure these attributes. The transaction mode has 3 possible values. We need to represent those somehow. We could take the transaction mode string but then we'd need to deal with managing the lifetime of that string allocation. We could use boolean attributes like
ATTR_TRANSACTION_MODE_DEFERRED => truebut then we'd need to handle invalid states like two of these attributes being set at once. For that reason I went with one attribute as the "key" and 3 attributes as the "values". These are just class constants onPdo\Sqlite.I've included very basic tests (in the
pdo_sqlite/tests/subclassesdirectory which I think is correct since this interacts withPdo\Sqlite). They test the attribute userland API and then thatbeginTransaction()actually respects the transaction mode using a simplified reproduction of the lock contention scenario in my original comment. It uses named in-memory DBs (that way separately created PDO instances can use the same memory region) and infers the transaction mode used in$pdobased on the error message$pdo2gets (or doesn't get) when it tries to start an immediate transaction.