Nested transactions without enable Savepoints.

[ZhukV]

Nested transactions without enable Savepoints.

[derrabus]

Okay, and what would you expect DBAL to do in that case?

Doctrine must have functionality for control logic in nested transactions. Developer MUST configure it himself, otherwise, which may affect the operation of applications.

You did not answer the question.

[javaDeveloperKid]

I still don't understand what's the problem that both middleware and flush() open a "transaction" (quoted as nested transaction creates a savepoint). It doesn't change anything in how application work. For happy path it means nothing. For unhappy path when using DBAL 3 your code already doesn't (shouldn't) do anything after nested transaction fails because root transaction is marked as rollback-only so any call to commit() would throw an exception.

My concerns are only related to small applications, where the presence of save points can cause misunderstanding among the developer himself.

If a developer is misunderstood by savepoint then he should gain knowledge about what it is and how it works.

[derrabus]

MySQL does not support nested transactions. The only way, DBAL can emulate them is through savepoints. In DBAL 4, you cannot disable savepoints anymore.

[ZhukV]

@derrabus , Hm. Maybe I not understand you or ask incorrect question.

MySQL support savepoints (logically - this is nested transactions). In some application we want to use savepoints, in other applications we don't want to use savepoints. If, configure savepoints will been deleted, next code work wrongly:

use Doctrine\DBAL\LockMode;
use Doctrine\ORM\EntityManagerInterface;

/** @var EntityManagerInterface $entityManager */

$entityManager->wrapInTransaction(function () use ($entityManager) {
    $query = $entityManager->createQueryBuilder()
        ->from(Customer::class, 'customer')
        ->select('customer')
        ->andWhere('....')
        ->getQuery();

    $query->setLockMode(LockMode::PESSIMISTIC_WRITE);

    $customer = $query->getOneOrNullResult();

    $customer->withdrawBalance('100');

    $entityManager->flush();
});

Because in this example, we not want to create savepoints on flush mechanism. Explain SQL from this example:

START TRANSACTION;
SELECT * FROM customer WHERE ... FOR UPDATE;
SAVEPOINT savepoint_1;
UPDATE customer SET balance = :new_balance WHERE id = :id;
RELEASE SAVEPOINT savepoint_1;
COMMIT;

Critical: this is a small example, but correct illustrate what we dont want to create savepoints.

Maybe I don't understand all features, and in ORM those will been fixed (not call beginTransaction in UnitOfWork::commit), because this repository only for DBAL?

Thank.

[derrabus]

So you basically don't want the entity manager to create a transaction for the flush operation, for whatever reason? Yeah, you should open an issue on the doctrine/orm repository then. That being said, I don't really see an issue with that savepoint, tbh.

[ZhukV]

@derrabus , I can open issue on doctrine/orm package, but doctrine/orm is high level application, and I'm sure that they won't want to control transaction level on entity manager (unit of work) side.

Ok, please see code below:

<?php

namespace Acme\Controller;

use Doctrine\ORM\EntityManagerInterface;
use Symfony\Component\HttpFoundation\Response;
use Symfony\Component\HttpKernel\Attribute\MapRequestPayload;
use Symfony\Component\Uid\Uuid;

class AddCommentController
{
    public function __construct(private EntityManagerInterface $entiytManager)
    {
    }

    public function addComment(Uuid $postId, #[MapRequestPayload] AddCommentRequest $request): Response
    {
        $post = $this->entiytManager->find(Post::class, $postId);

        $comment = new PostComment($post, $request->comment);
        $this->entiytManager->persist($comment);

        $this->entiytManager->flush();

        return new Response(status: 204);
    }
}

It's very simple application, and work perfectly. But, what if we work with balances, and want to control security (race condition attack)? MySQL support lock, and many developers use it.

Ok, as example we create service with withdraw money from balance.

<?php

namespace Acme;

use Doctrine\ORM\EntityManagerInterface;
use Symfony\Component\Uid\Uuid;

class AccountBalanceManager
{
    public function __construct(private EntityManagerInterface $entityManager)
    {
    }

    public function withdraw(Uuid $accountId, float $withdrawMoney): void
    {
        $account = $this->entityManager->find(Account::class, $accountId);

        $actualBalance = $account->getBalance();
        $finalBalance = $actualBalance - $withdrawMoney;

        if ($finalBalance < 0) {
            throw new InsufficientBalanceException();
        }

        $account->setBalance($finalBalance);
    }
}

But, this application can be attacked by race condition. For safety, developer can use LOCK architecture provided from MySQL (or another database):

public function withdraw(Uuid $accountId, float $withdrawMoney): void
{
    $this->entityManager->wrapInTransaction(function use ($accountId, $withdrawMoney) {
        $account = $this->entityManager->createQueryBuilder()
            ->from(Account::class, 'account')
            ->select('account')
            ->andWhere('account.id = :id')
            ->setParameter('id', $accountId)
            ->getQuery()
            ->setLockMode(LockMode::PESSIMISTIC_WRITE) // <-- HERE WITH USE "SELECT FOR UPDATE"
            ->getOneOrNullResult();

        $actualBalance = $account->getBalance();
        $finalBalance = $actualBalance - $withdrawMoney;

        if ($finalBalance < 0) {
            throw new InsufficientBalanceException();
        }

        $account->setBalance($finalBalance);
    });
}

Very nice, our application is protected for possible race condition attack. And if we want to withdraw money from account, we use this service and guarantee security.

Ok, what next? Let's assume that we are an online store, and we want to control buy product with control possible counts of purchase?

class ProductManager
{
    public function __construct(private EntityManagerInterface $entityManager, private AccountBalanceManager $accountBalanceManager)
    {
    }

    public function buyProduct(Uuid $productId, Uuid $accountId): void
    {
        $product = $this->entityManager->find(Product::class, $productId);

        $availableForPurchase = $product->getAvailableForPurchase();
        $availableForPurchase--;

        if (!$availableForPurchase) {
            throw new ProductNotAllowedForPurchaseException('....');
        }

        $product->setAvailableForPurchase($availableForPurchase);

        $this->accountBalanceManager->withdraw($accountId, $product->getPrice());

        $this->entityManager->flush();
    }
}

Very nice, account can't purchase product if it's out of stock. But, this method not protected for race condition. Ok, change it for protect:

public function buyProduct(Uuid $productId, Uuid $accountId): void
{
    $this->entityManager->wrapInTransaction(function () use ($productId, $accountId) {
        $product = $this->entityManager->createQueryBuilder()
            ->from(Product::class, 'product')
            ->select('product')
            ->andWhere('product.id = :id')
            ->setParameter('id', $productId)
            ->getQuery()
            ->setLockMode(LockMode::PESSIMISTIC_WRITE)
            ->getOneOrNullResult();

        $availableForPurchase = $product->getAvailableForPurchase();
        $availableForPurchase--;

        if (!$availableForPurchase) {
            throw new ProductNotAllowedForPurchaseException('....');
        }

        $product->setAvailableForPurchase($availableForPurchase);

        $this->accountBalanceManager->withdraw($accountId, $product->getPrice());

        $this->entityManager->flush();
    });
}

Our application protected for race condition for products and balance too. Very perfectly!
You can see, that by code we use transaction in transaction: we call withdraw money from opened transaction. But, for our application it's right solution, because we want to full security protected.

Ok, what about SQL (if you remove setting to disable savepoints):

START TRANSACTION; -- ProductManager (wrapInTransaction)
    SELECT * FROM products WHERE id = ... FOR UPDATE;
    SAVEPOINT DOCTRINE2_SAVEPOINT_2; -- BalanceManager (wrapInTransaction)
        SAVEPOINT DOCTRINE2_SAVEPOINT_3; -- UnitOfWork::flush
            SELECT * FROM accounts WHERE id = ... FOR UPDATE;
            UPDATE accounts SET balance = ? WHERE id = ...;
        RELEASE SAVEPOINT DOCTRINE2_SAVEPOINT_3;
    RELEASE SAVEPOINT;
    SAVEPOINT DOCTRINE2_SAVEPOINT_2; -- UnitOfWork::flush
        UPDATE products SET available_for_purchase = ? WHERE id = ...;
    RELEASE SAVEPOINT
COMMIT; -- ProductManager

What? What is this? Why system open savepoints without ask me?
You think what this correct? Force system to open savepoints without any possible control it? Developer MUST HAVE possible for control - want use savepoints or don't want, you can control it.

doctrine/dbal - LOW LEVEL library. It's not problem for doctrine/orm, because doctrine/orm - high level library, and not anybody wants to control this on orm layer.

I develop many application in financial sector, and in it we should full control all transactions/savepoints for correct process financial transaction, rollback any changes and next save error reasons (all operation in one root transaction).
But I developer many simple applications too (blog like, etc...) where I don't any control about transactions. Called only EntityManager::flush.

Disable setting for control use savepoints or not - very, very bad idea, if we see generally doctrine. The developer must decide for himself, use savepoints or not.

Ok, a lot of text above, what you provide for save backward compatibiltiy (save setting for enable/disable)? I think, what you can pass nullable to this setting, and on Connection check, if try to use nested transactions and settings not configured (null value), throw error. By default, setting should not configured (via bundle, etc...), as result for simple application where developer not use transactional layers - all OK. But if developer want to use nested transaction, hi MUST CONFIGURE IT.

How about other problems?

Code described above - simple code, on real applications devleoper make other architectures for control transactions. It's can be middleware layer on message bus, or use AOP patter with custom attributes (annotations), etc... Many ways for create ideal architecture for specific application. As result, in some applications call to begin transaction inside opened transaction - normal flow.

UPD: It's relate only to doctrine/dbal, because I can change this code for use only doctrine/dbal (without orm).

[derrabus]

Thank you for this little lecture on how transactions work. Not sure I needed this, but well. 🤷🏻‍♂️

As result, in some applications call to begin transaction inside opened transaction - normal flow.

Okay, and what would you expect DBAL to do in that case? And please keep your answer brief. 🙏🏻

[ZhukV]

And please keep your answer brief. 🙏🏻

Why brief? If you not understand problem, I MUST full describe problem because you as developer must full understand problems after change code of library.

Okay, and what would you expect DBAL to do in that case?

Doctrine must have functionality for control logic in nested transactions. Developer MUST configure it himself, otherwise, which may affect the operation of applications.

[javaDeveloperKid]

Thank you for this little lecture on how transactions work. Not sure I needed this, but well. 🤷🏻‍♂️

@derrabus This comment was unnecessary. I don't think the guy wanted to give you a lecture on how transactions work. He just wanted to explain his POV (so his understanding of transactions as a consequence) and maybe the misunderstanding can be on his side.

[javaDeveloperKid]

@ZhukV Hi, could you elaborate on how the mandatory savepoints will impact you application where nested transactions occur? Remember that savepoint does not mean something is "saved" to the database.

Let's consider this example:

try {
  $conn->beginTransaction() // sql: begin
  // some logic before
  try {
    $conn->beginTransaction(); // sql: savepoint s1
    // some logic
    $conn->commit(); // sql: release savepoint s1
  } catch ($e) {
    $conn->rollback(); // sql: rollback to savepoint s1
    throw $e; // throw or not
  }
  // some logic after
  $conn->commit(); // sql: commit
} catch ($e) {
  $conn->rollback(); // sql: rollback
  $throw $e;
}

For happy path there's no problem to talk about because it's the same as there was not savepoint related sql command. So let's consider the unhappy path (i.e. exception is thrown in nested try):
DBAL 3 rollback in nested transaction would mark the root transaction rollback-only.
DBAL 4 rollback in nested transaction will not mark transaction rollback-only, but only rollback to savepoint s1.
You can implement a process in your application in 2 ways:

1. you can throw nested exception. It will then bubble up to the root transaction and everything will be rolled back.
2. you can silent nested exception but it doesn't really make sense when using DBAL 3 as you will not be able to commit the root transaction. So after upgrading to DBAL 4 you will still have those nested exceptions thrown. If your application has never thrown Transaction commit failed because the transaction has been marked for rollback only. exception it means you don't have any silenced exceptions and you should be fine.

The switch from marking transaction rollback only to savepoints allows to silent exceptions whereas in DBAL 3 your whole process is rolled back even if you don't care about whether some nested transaction failed. On the other hand, when root transaction is marked rollback only then we don't have to worry we could have missed a throw statement somewhere.
Example: Somewhere deep in the root process there is a class that saves some log to the database. We don't really care if the log was saved because it's not crucial and the main process is much more important to succeed, so we add try..catch block around that part of code, rollback in case of failure and move on.

I'm looking forward to hear from you :)

[ZhukV]

@javaDeveloperKid , you are completely right! And in few our applications we use savepoints. As example - process financial transaction:

public function process(TransactionInterface $transaction): void
{
    $this->entityManager->beginTransaction(); // Root transaction

    try {
        $this->entityManager->beginTransaction(); // savepoint 1
        $this->processTransaction($transaction); // logic for process transaction (complete, decline, etc...)
        $this->entityManager->flush();
        $this->entityManager->commit();
    } catch (\Throwable $error) {
        $this->entityManager->beginTransaction(); // savepoint 1
        $this->handleErrorForTransaction($transaction, $error); // correct write errors
        $this->entityManager->flush();
        $this->entityManager->commit();
    }

    $this->entityManager->commit(); // root transaction
}

We can receive error on process financial transaction, and in next step we must handle error and write data to database, but we should ignore all records writes in process transaction.

In this situation, all OK, system work perfectly!

My concerns are only related to small applications, where the presence of save points can cause misunderstanding among the developer himself. In future, there will be no possibility to customize this (other than redoing it connection class). As example, when we use TransactionMiddleware in MessageBus (Symfony). Middleware open our transaction and call EntityManager::flush inside. EntityManager::flush opens transaction too. As result, after execute only one message (via MessageBus), system opens one savepoint.

It would be great to be able to customize the logic like we did before. I understand the root problem for Doctrine and why developers want to disable nested transactions without savepoints and I support it! But for very simple applications it can be difficult for developers. Only!

@javaDeveloperKid , Thank for understanding me.

[ZhukV]

For apps not any problem. You right, it doesn't change anything in how application work. It's only difficult for developers who not known about savepoints and transactions. And after update they see savepoints in SQL logs and have more questions about it.

Otherwise, I completely agree with:

If a developer is misunderstood by savepoint then he should gain knowledge about what it is and how it works.

@javaDeveloperKid , thanks you for more detailed description and understanding me. Have a nice day!

[github-actions]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.