Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove support for transaction nesting without savepoints #5401

Merged
merged 1 commit into from
May 17, 2022
Merged

Remove support for transaction nesting without savepoints #5401

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions UPGRADE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,25 @@ awareness about deprecated code.

# Upgrade to 4.0

## BC BREAK: remove support for transaction nesting without savepoints

Starting a transaction inside another transaction with
`Doctrine\DBAL\Connection::beginTransaction()` now always results in
savepoints being used.

In case your platform does not support savepoints, you will have to
rework your application logic so as to avoid nested transaction blocks.

## Deprecated: configuration methods related to transaction nesting

Since it is no longer possible to configure whether transaction nesting is
emulated with savepoints or not, configuring that behavior has no effect and it
is deprecated to attempt to change it or to know how it is configured. As a
result, the following methods are deprecated:

- `Connection::setNestTransactionsWithSavepoints()`
- `Connection::getNestTransactionsWithSavepoints()`

## BC BREAK: Auto-increment columns on PostgreSQL are implemented as `IDENTITY`, not `SERIAL`.

Instead of using `SERIAL*` column types for `autoincrement` columns, the DBAL will now use
Expand Down
119 changes: 10 additions & 109 deletions docs/en/reference/transactions.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,48 +67,12 @@ Transaction Nesting
-------------------

Calling ``beginTransaction()`` while already in a transaction will
result in two very different behaviors depending on whether transaction
nesting with savepoints is enabled or not. In both cases though, there
won't be an actual transaction inside a transaction, even if your RDBMS
supports it. There is always only a single, real database transaction.
not result in an actual transaction inside a transaction, even if your
RDBMS supports it. Instead, transaction nesting is emulated by resorting
to SQL savepoints. There is always only a single, real database
transaction.

By default, transaction nesting at the SQL level with savepoints is
disabled. The value for that setting can be set on a per-connection
basis, with
``Doctrine\DBAL\Connection#setNestTransactionsWithSavepoints()``.

Nesting transactions without savepoints is deprecated, but is the
default behavior for backward compatibility reasons.

Dummy mode
~~~~~~~~~~
.. warning::

This behavior is deprecated, avoid it with
``Doctrine\DBAL\Connection#setNestTransactionsWithSavepoints(true)``.

When transaction nesting with savepoints is disabled, what happens is
not so much transaction nesting as propagating transaction control up
the call stack. For that purpose, the ``Connection`` class keeps an
internal counter that represents the nesting level and is
increased/decreased as ``beginTransaction()``, ``commit()`` and
``rollBack()`` are invoked. ``beginTransaction()`` increases the nesting
level whilst ``commit()`` and ``rollBack()`` decrease the nesting level.
The nesting level starts at 0.
Whenever the nesting level transitions from 0 to 1,
``beginTransaction()`` is invoked on the underlying driver connection
and whenever the nesting level transitions from 1 to 0, ``commit()`` or
``rollBack()`` is invoked on the underlying driver, depending on whether
the transition was caused by ``Connection#commit()`` or
``Connection#rollBack()``.

What this means is that transaction control is basically passed to
code higher up in the call stack and the inner transaction block does
not actually result in an SQL transaction. It is not ignored either
though.

To visualize what this means in practice, consider the following
example:
Let's examine what happens with an example

::

Expand All @@ -121,13 +85,13 @@ example:

// nested transaction block, this might be in some other API/library code that is
// unaware of the outer transaction.
$conn->beginTransaction(); // 1 => 2
$conn->beginTransaction(); // 1 => 2, savepoint created
try {
...

$conn->commit(); // 2 => 1
} catch (\Exception $e) {
$conn->rollBack(); // 2 => 1, transaction marked for rollback only
$conn->rollBack(); // 2 => 1, rollback to savepoint
throw $e;
}

Expand All @@ -139,33 +103,9 @@ example:
throw $e;
}

However, **a rollback in a nested transaction block will always mark the
current transaction so that the only possible outcome of the transaction
is to be rolled back**.
That means in the above example, the rollback in the inner
transaction block marks the whole transaction for rollback only.
Even if the nested transaction block would not rethrow the
exception, the transaction is marked for rollback only and the
commit of the outer transaction would trigger an exception, leading
to the final rollback. This also means that you cannot
successfully commit some changes in an outer transaction if an
inner transaction block fails and issues a rollback, even if this
would be the desired behavior (i.e. because the nested operation is
"optional" for the purpose of the outer transaction block). To
achieve that, you need to resort to transaction nesting with savepoint.

All that is guaranteed to the inner transaction is that it still
happens atomically, all or nothing, the transaction just gets a
wider scope and the control is handed to the outer scope.

.. note::

The transaction nesting described here is a debated
feature that has its critics. Form your own opinion. We recommend
avoiding nesting transaction blocks when possible, and most of the
time, it is possible. Transaction control should mostly be left to
a service layer and not be handled in data access objects or
similar.
Everything is handled at the SQL level: the main transaction is not
marked for rollback only, but the inner emulated transaction is rolled
back to the savepoint.

.. warning::

Expand All @@ -177,45 +117,6 @@ wider scope and the control is handed to the outer scope.
nesting level, causing errors with broken transaction boundaries
that may be hard to debug.

Emulated Transaction Nesting with Savepoints
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Let's now examine what happens when transaction nesting with savepoints
is enabled, with the same example as above

::

<?php
// $conn instanceof Doctrine\DBAL\Connection
$conn->beginTransaction(); // 0 => 1, "real" transaction started
try {

...

// nested transaction block, this might be in some other API/library code that is
// unaware of the outer transaction.
$conn->beginTransaction(); // 1 => 2, savepoint created
try {
...

$conn->commit(); // 2 => 1
} catch (\Exception $e) {
$conn->rollBack(); // 2 => 1, rollback to savepoint
throw $e;
}

...

$conn->commit(); // 1 => 0, "real" transaction committed
} catch (\Exception $e) {
$conn->rollBack(); // 1 => 0, "real" transaction rollback
throw $e;
}

This time, everything is handled at the SQL level: the main transaction
is not marked for rollback only, but the inner emulated transaction is
rolled back to the savepoint.

Auto-commit mode
----------------

Expand Down
70 changes: 29 additions & 41 deletions src/Connection.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
use Doctrine\DBAL\Exception\DriverException;
use Doctrine\DBAL\Exception\EmptyCriteriaNotAllowed;
use Doctrine\DBAL\Exception\InvalidPlatformType;
use Doctrine\DBAL\Exception\MayNotAlterNestedTransactionWithSavepointsInTransaction;
use Doctrine\DBAL\Exception\NoActiveTransaction;
use Doctrine\DBAL\Exception\SavepointsNotSupported;
use Doctrine\DBAL\Platforms\AbstractPlatform;
Expand All @@ -32,6 +31,7 @@
use Doctrine\DBAL\SQL\Parser;
use Doctrine\DBAL\Types\Type;
use Doctrine\Deprecations\Deprecation;
use InvalidArgumentException;
use Throwable;
use Traversable;

Expand All @@ -41,6 +41,7 @@
use function is_int;
use function is_string;
use function key;
use function sprintf;

/**
* A database abstraction-level connection that implements features like events, transaction isolation levels,
Expand Down Expand Up @@ -97,11 +98,6 @@ class Connection implements ServerVersionProvider
*/
private ?int $transactionIsolationLevel = null;

/**
* If nested transactions should use savepoints.
*/
private bool $nestTransactionsWithSavepoints = false;

/**
* The parameters used during creation of the Connection instance.
*
Expand Down Expand Up @@ -1045,39 +1041,44 @@ public function transactional(Closure $func): mixed
/**
* Sets if nested transactions should use savepoints.
*
* @deprecated No replacement planned
*
* @throws Exception
*/
public function setNestTransactionsWithSavepoints(bool $nestTransactionsWithSavepoints): void
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we keep the method, we should throw if false is passed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why deprecate it in the next major given that the dummy mode is already deprecated and is being removed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In #5383, I tell people they should not call setNestTransactionsWithSavepoints(true) since false is the default for backward compatibility. If they do so, it means they will have one more action to perform right after upgrading to v4 before they have a working application. I'll leave you the judge of whether this is fine or not, I don't know if we should have some kind of rule about this. My concern is that if the list of such changes gets too big, the upgrade could become painful.

Copy link
Member

@morozov morozov May 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, so there are two breaking changes necessary:

  1. Change the default mode from false to true in 4.0 (we're letting users opt in to the true mode in 3.x).
  2. Remove the API for changing the mode in 5.0 (it will exist in 4.0 only as a stub for backward compatibility with 3.x).

Makes sense.

{
if (! $nestTransactionsWithSavepoints) {
Deprecation::trigger(
'doctrine/dbal',
'https://github.com/doctrine/dbal/pull/5383',
<<<'DEPRECATION'
Nesting transactions without enabling savepoints is deprecated.
Call %s::setNestTransactionsWithSavepoints(true) to enable savepoints.
DEPRECATION,
self::class
);
}

if ($this->transactionNestingLevel > 0) {
throw MayNotAlterNestedTransactionWithSavepointsInTransaction::new();
throw new InvalidArgumentException(sprintf(
'Calling %s with false to enable nesting transactions without savepoints is no longer supported.',
__METHOD__
));
}

if (! $this->getDatabasePlatform()->supportsSavepoints()) {
throw SavepointsNotSupported::new();
}

$this->nestTransactionsWithSavepoints = $nestTransactionsWithSavepoints;
Deprecation::trigger(
'doctrine/dbal',
'https://github.com/doctrine/dbal/pull/5383',
'%s is deprecated and will be removed in 5.0',
__METHOD__
);
}

/**
* Gets if nested transactions should use savepoints.
*
* @deprecated No replacement planned
*
* @return true
*/
public function getNestTransactionsWithSavepoints(): bool
{
return $this->nestTransactionsWithSavepoints;
Deprecation::trigger(
'doctrine/dbal',
'https://github.com/doctrine/dbal/pull/5383',
'%s is deprecated and will be removed in 5.0',
__METHOD__
);

return true;
}

/**
Expand All @@ -1099,18 +1100,8 @@ public function beginTransaction(): void

if ($this->transactionNestingLevel === 1) {
$connection->beginTransaction();
} elseif ($this->nestTransactionsWithSavepoints) {
$this->createSavepoint($this->_getNestedTransactionSavePointName());
} else {
Deprecation::trigger(
'doctrine/dbal',
'https://github.com/doctrine/dbal/pull/5383',
<<<'DEPRECATION'
Nesting transactions without enabling savepoints is deprecated.
Call %s::setNestTransactionsWithSavepoints(true) to enable savepoints.
DEPRECATION,
self::class
);
$this->createSavepoint($this->_getNestedTransactionSavePointName());
}

$this->getEventManager()->dispatchEvent(Events::onTransactionBegin, new TransactionBeginEventArgs($this));
Expand All @@ -1137,7 +1128,7 @@ public function commit(): void
} catch (Driver\Exception $e) {
throw $this->convertException($e);
}
} elseif ($this->nestTransactionsWithSavepoints) {
} else {
$this->releaseSavepoint($this->_getNestedTransactionSavePointName());
}

Expand Down Expand Up @@ -1197,11 +1188,8 @@ public function rollBack(): void
$this->beginTransaction();
}
}
} elseif ($this->nestTransactionsWithSavepoints) {
$this->rollbackSavepoint($this->_getNestedTransactionSavePointName());
--$this->transactionNestingLevel;
} else {
$this->isRollbackOnly = true;
$this->rollbackSavepoint($this->_getNestedTransactionSavePointName());
--$this->transactionNestingLevel;
}

Expand Down
18 changes: 0 additions & 18 deletions src/Exception/MayNotAlterNestedTransactionWithSavepointsInTransaction.php
View file
Open in desktop

This file was deleted.

25 changes: 21 additions & 4 deletions tests/ConnectionTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,11 +167,20 @@ static function (TransactionCommitEventArgs $eventArgs) use ($conn): bool {
$conn->commit();
}

public function testTransactionCommitEventNotCalledAfterRollBack(): void
public function testTransactionCommitEventCalledAfterRollBack(): void
{
$eventManager = new EventManager();
$driverMock = $this->createMock(Driver::class);
$conn = new Connection([], $driverMock, new Configuration(), $eventManager);
$platform = $this->createStub(AbstractPlatform::class);
$platform
->method('supportsSavepoints')
->willReturn(true);

$driverMock = $this->createMock(Driver::class);
$driverMock
->method('getDatabasePlatform')
->willReturn($platform);

$conn = new Connection([], $driverMock, new Configuration(), $eventManager);

$rollBackListenerMock = $this->createMock(TransactionRollBackDispatchEventListener::class);
$rollBackListenerMock
Expand All @@ -187,7 +196,7 @@ static function (TransactionRollBackEventArgs $eventArgs) use ($conn): bool {
$eventManager->addEventListener([Events::onTransactionRollBack], $rollBackListenerMock);

$commitListenerMock = $this->createMock(TransactionCommitDispatchEventListener::class);
$commitListenerMock->expects(self::never())->method('onTransactionCommit');
$commitListenerMock->expects(self::exactly(1))->method('onTransactionCommit');
$eventManager->addEventListener([Events::onTransactionCommit], $commitListenerMock);

$conn->beginTransaction();
Expand Down Expand Up @@ -345,7 +354,15 @@ public function testRollBackStartsTransactionInNoAutoCommitMode(): void

public function testSwitchingAutoCommitModeCommitsAllCurrentTransactions(): void
{
$platform = $this->createStub(AbstractPlatform::class);
$platform
->method('supportsSavepoints')
->willReturn(true);

$driverMock = $this->createMock(Driver::class);
$driverMock
->method('getDatabasePlatform')
->willReturn($platform);

$conn = new Connection([], $driverMock);

Expand Down
Loading