Implement GH-8967: Add PDO_SQLITE_ATTR_TRANSACTION_MODE

[stancl]

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\Sqlite should 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.html

When 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_timeout pragma, so it just immediately fails with SQLITE_BUSY.

On the other hand, when you start an immediate transaction, you don't have to worry about getting SQLITE_BUSY unless another transaction holds a lock for longer than the busy_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 as php test.php, one of the transactions fails with SQLITE_BUSY. If you run it as php 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 => true but 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 on Pdo\Sqlite.

I've included very basic tests (in the pdo_sqlite/tests/subclasses directory which I think is correct since this interacts with Pdo\Sqlite). They test the attribute userland API and then that beginTransaction() 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 $pdo based on the error message $pdo2 gets (or doesn't get) when it tries to start an immediate transaction.

[stancl]

@kocsismate @SakiTakamachi Will this make it into 8.5 if merged before the feature freeze?

[panda-madness]

@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.

[stancl]

https://news-web.php.net/php.internals/128446

[panda-madness]

Tangentially related to this, I've opened a PR for Laravel to work around this, if you'd like to chime in @stancl .

[AhmedAlaa4611]

What is the problem with using:

$this->getPdo()->exec("BEGIN IMMEDIATE TRANSACTION");

[stancl]

Most frameworks use $pdo->beginTransaction(). It's also more portable. See the PR linked right above your comment. Frameworks shouldn't have to add hacks like that, this is exactly what PDO attributes are for.

[AhmedAlaa4611]

I’ve reviewed the PRs along with nearly all of your comments and discussions and wanted to share a few thoughts:

Point 1

I don’t think we can get anything simpler or clearer than:

$this->getPdo()->exec("BEGIN IMMEDIATE TRANSACTION");

Compared to what this PR suggests:

$pdo->setAttribute($pdo::ATTR_TRANSACTION_MODE, $pdo::ATTR_TRANSACTION_MODE_IMMEDIATE);

Point 2

Laravel already has a dedicated class for SQLite src/Illuminate/Database/SQLiteConnection.php and it is the natural place for handling this kind of logic.

Point 3

If IMMEDIATE should be the default mode for SQLite, then this seems more like a SQLite-level concern rather than something to solve by introducing new PDO attributes.

Conclusion

The existing Laravel PR already covers this behavior in a cleaner and more appropriate way, so this PR seems unnecessary and can be closed.

[stancl]

Sorry but you're completely missing the point here to be honest.

[AhmedAlaa4611]

Can you clarify how the changes introduced in the Laravel PR would look if this PR is merged?

[stancl]

This is a PR to php-src. You're hyperfocused on Laravel and don't seem to know how PDO attributes are used. Please keep the discussion on topic. To answer your points briefly:

1. That is not how PDO attributes are used. This is an example of how PDO attributes are used. You set PDO\Sqlite::ATTR_TRANSACTION_MODE => PDO\Sqlite::ATTR_TRANSACTION_MODE_IMMEDIATE. Pretty much any framework supports this. This is not a question of $this->getPdo()->exec("BEGIN IMMEDIATE TRANSACTION"); vs $pdo->setAttribute($pdo::ATTR_TRANSACTION_MODE, $pdo::ATTR_TRANSACTION_MODE_IMMEDIATE);. Most frameworks don't let you do the former. They do let you specify PDO attributes with a single line of configuration though.
2. Irrelevant, this is a PR to php-src so that any framework can support this without making ANY code changes to the framework code.
3. That's not how SQLite works. If DEFERRED is the default transaction mode, it will always be the default transaction mode. They do not make breaking changes.
4. "The existing Laravel PR already covers this behavior in a cleaner and more appropriate way" it is neither cleaner nor more appropriate. Your suggestion is that every single framework should add special logic just for SQLite with custom framework-specific configuration for SQLite transaction modes. PDO attributes work in any framework that lets users specify PDO attributes (which is pretty much every framework).

This PR allows anyone, using any framework, to change the SQLite transaction mode. It is a matter of adding a single line to a config file in any framework. The alternative — your suggestion — is that every PHP framework on earth should have to make the change Laravel made in the PR linked above. That change is a hack which shouldn't be needed. There's no reason why frameworks should be using beginTransaction() for every PDO driver except SQLite, where they have to manually handle user configuration to run the right exec("BEGIN $mode TRANSACTION") statement.

[iluuu1994]

@SakiTakamachi Can you have a look? Also @php/release-managers-85, since OP asked whether this may still target 8.5. https://externals.io/message/128446

[TimWolla]

Not a PDO expert and not an SQLite expert either. This seems to be mostly right, I didn't see any obvious conceptual problems.

[mbeccati]

LGTM, however... 10 years ago I would have probably pushed something similar to pdo_pgsql without worries (before feature freeze)... but now I'd be much more reluctant without a proper RFC process.

Then again, it's up to the RMs

[TimWolla]

Some optional suggestions, but LGTM.

[edorian]

RM wise: have too little expertise to judge the impact but I'm leaning towards being ok with having this in 8.5.

But I'd be grateful for your take @SakiTakamachi

[SakiTakamachi]

I'll look it tomorrow :)

[SakiTakamachi]

Looks goot to me, thanks!

[DanielEScherzer]

RM approval, technical review not performed

[TimWolla]

Given the approvals, I've merged master and added NEWS/UPGRADING. Will merge when CI passes.

[stancl]

@TimWolla Do you want me to squash and rebase (in place of the master merge) again so the commit is merged with the original message? Or you can just copy it when squash-merging I guess.

[TimWolla]

I'll handle the rest during merging, thank you!

[stancl]

Thanks a lot!