Transactions

[mmoreram]

Let's chat about how transactions can be implemented in the package.

[bartvanhoutte]

👋

As transactions exist per database session it seems to me that the most obvious way to implement something like this is by adding the ability to lease a connection from the connection pool. When leasing a connection it's taken out of the pool of available connections and is only returned after the transaction has completed, guaranteeing that no other coroutine is adding queries to the transaction by mistake.

The interface could look something like this: ConnectionPool::lease(): PromiseInterface, or perhaps ConnectionPool::transaction(): PromiseInterface. Bikeshedding ensue!

As all connections might be leased, the function should return a Promise and only return a connection when one is available (again). We might need to add some logging to warn for a connection pool without available connections, indicating the need for a bigger pool.

~Leasing in SingleConnection would throw an exception since you only have one database session, effectively unabling transactions in an asynchronous environment.~

Furthermore, we'd obviously also need a function to end the transaction and return the lease.

[bartvanhoutte]

Started working on something, see https://github.com/driftphp/reactphp-dbal/compare/master...bartvanhoutte:feature/transaction?expand=1.

Small snippet demonstrating its behaviour:

    $connection1 = null;
    $connection2 = null;

    Loop::addTimer(10, static function () use (&$connection1, $pool) {
       $pool->commitTransaction($connection1);
       print 'releasing connection 1' . PHP_EOL;
    });
    Loop::addTimer(20, static function () use (&$connection2, $pool) {
        $pool->commitTransaction($connection2);
        print 'releasing connection 2' . PHP_EOL;
    });

    $connection1 = await($pool->startTransaction());
    $connection2 = await($pool->startTransaction());
    $connection3 = await($pool->queryBySQL('SELECT 1=1'));

    print 'done' . PHP_EOL;

A few pointers:

• Haven't thought about nested transactions.
• ~The only BC break so far is Connection::getDriverNamespace(...) which now returns a PromiseInterface.~
• There's a debug statement in ConnectionPool::bestConnectionWorker(...) that would be a perfect candidate to introduce some PSR logging.

[mmoreram]

Why Connection::getDriverNamespace(...) should have to change to an async method? I mean, it can takes the first connection, even if it's busy, and get the driver namespace.

[mmoreram]

Seems good to me, BTW :)

[bartvanhoutte]

Why Connection::getDriverNamespace(...) should have to change to an async method? I mean, it can takes the first connection, even if it's busy, and get the driver namespace.

You're absolutely right. Currently the ConnectionPool takes the best connection so I assumed that was for a reason. I'll make the change.

[bartvanhoutte]

It looks like we'll have to maintain driver-specific syntax to start a transaction. DBAL currently calls beginTransaction directly on the PDO driver, which is not available to us.

I've added the above to my branch, do you want me to create a PR for easier reviewing?

[marinhekman]

It looks like we'll have to maintain different syntax to start a transaction depending on the driver. DBAL currently calls beginTransaction directly on the PDO driver, thus not using the database specific syntax. For example, SQLite uses BEGIN TRANSACTION while MySQL uses START TRANSACTION.

I think the PDO driver takes care of this ? I mean if the PDO constructor is set to use a mysql host , calling beginTransaction() will just do a START TRANSATION for you.

I guess the interface src/Driver/Driver.php should be having a startTransaction() to be implemented by all Drift php database drivers. Which you did !

[marinhekman]

So for my understanding . In the "original" code what would happen if you would get a connection from the pool and do a "start transaction" ? I guess after this query has been executed the connection itself is set to available again ? Then I understand the need for this , to be able to have a connection starting a transaction and being unavailable until that connection is set to stop its transaction. Am I seeing this right ?

What I can do, is testing your branch in our project using react php dbal . It will not use transactions, but at least we also test the existing non-transactions code still works. There should not be any breaking changes right ? I will let you guys know.

[bartvanhoutte]

I think the PDO driver takes care of this ? I mean if the PDO constructor is set to use a mysql host , calling beginTransaction() will just do a START TRANSATION for you.

I thought about this too, but PDO is only used in the child processes created in the driver and thus not available in Connection. Have I got this right @mmoreram?

In the "original" code what would happen if you would get a connection from the pool and do a "start transaction" ? I guess after this query has been executed the connection itself is set to available again ?

So there's a couple of issues here. The first issue is that using transactions in a SingleConnection probably does not do what you want it to do in an asynchronous environment because the transaction will be shared accross multiple coroutines. You can start a transaction in one coroutine/fiber, but you can't be sure no other coroutine will execute queries in the same transaction.

For example:

$connection = SingleConnection::createConnected(...$args);

Loop::futureTick(async(static function () use ($connection) {
    await($connection->startTransaction());
    await($connection->insert('foo', []));
    await(\React\Promise\Timer\sleep(10));
    await($connection->commitTransaction());
}));

Loop::addTimer(5, async(static function () use ($connection) {
    await($connection->insert('foo', []));
}));

In this case, the timer started in Loop::addTimer(...) will add an insert query to the transaction started in the Loop::futureTick, which is not what we want. Therefore, I disabled starting a transaction in a SingleConnection that wasn't created in a ConnectionPool.

Then I understand the need for this , to be able to have a connection starting a transaction and being unavailable until that connection is set to stop its transaction. Am I seeing this right ?

Correct, the same issue described above can happen in a ConnectionPool. This is where leasing a connection comes in. When a transaction is started in ConnectionPool, the connection is taken from the pool (technically, it's flagged as "leased"), making sure the connection is not reused in other coroutines. Whenever the transaction is committed or rolled back, the connection is added back to the pool.

What I can do, is testing your branch in our project using react php dbal . It will not use transactions, but at least we also test the existing non-transactions code still works. There should not be any breaking changes right ? I will let you guys know.

Correct! Currently I'm testing the branch with transactions in a project of ours, so far so good. I can make a pull request to this project if it makes it easier to test @marinhekman.

[bartvanhoutte]

A couple of things to think about:

• Should there be a configurable maximum limit for leased connections? For example, if you have 4 connections in a ConnectionPool, would you like to reserve one or more connections that cannot have transactions? This could be useful if you don't frequently use transactions in your application and want to make sure there's always at least one connection for SELECT statements and INSERT and UPDATE statements that do not need to be executed in a transaction.

• It would be useful to get some feedback when all connections are leased so you can finetune the amount of connections created in a ConnectionPool. Are we okay with introducing psr/log as a dependency for the project - if yes, which version - and make Connection or ConnectionPool implement LoggerAwareInterface? This way, we can log a notice when all connections are in use.

[marinhekman]

I can make a pull request to this project if it makes it easier to test @marinhekman. @bartvanhoutte Yes please, put it in draft for now I guess until you really want to merge. I do not need the PR to be able to test , I can just change the git source in our composer.json I guess

Thanks for explaining the leasing more in-depth. I do understand it better now, that a connection should not be reused in other coroutines, at least not for transactions. Let me look at the code more in-depth as well, e.g. wondering now if the leasing only takes place for transactions , or in general for instance. That's also "meets" your question above:

would you like to reserve one or more connections that cannot have transactions

@mmoreram I would not mind to have a psr/log dependency introduced for logging, what do you think? For console apps I think it would be nice to have a output of these feedback notices as well (depending on the verbosity set). Therefore I think we should consider using monolog such that we can just write a notice to different locations if we want to https://symfony.com/doc/current/logging.html#handlers-writing-logs-to-different-locations https://symfony.com/doc/current/logging/monolog_console.html

[bartvanhoutte]

Therefore I think we should consider using monolog such that we can just write a notice to different locations if we want to

@marinhekman monolog implements psr/log, so it's not necessary for this library to depend on monolog/monolog; psr/log is sufficient. You can require monolog/monolog in your own application and pass a monolog Logger to this library.

The question is which version of psr/log we'd like to support since you can't support all current versions. Versions >= 2 require PHP 8.0 but the minimum requirement of this library is PHP 7.4.

[marinhekman]

@mmoreram I guess you would like to support php7.4+ incl. php 8. If so, we cannot rely on php8 only, so we have to properly use psr/log version < 2 ? @bartvanhoutte I am on vacation for 3 weeks. On return, I will test this branch

[bartvanhoutte]

Since PHP 7.4 is officially end-of-life in three months I personally would prefer to drop support for PHP <= 8.0. New projects that are using PHP 8+ are presumably going to use psr/log >= 2 making this library incompatible.

[marinhekman]

Still alive here. I am gonna test this branch the next week or the one after. I will give an update after that

[marinhekman]

@bartvanhoutte I tested our project on your branch , and did not encounter any issues. To be clear, we do not use any transactions , so using drift react without it still works . If you test the transaction parts, and me and @mmoreram have reviewed the code, I think we can merge this into master. @mmoreram ? There was still some psr logging to work on ? Let's go to php 8 , we already moved to 8.1 here as well.