Skip to content

Commit

Permalink
LockMode::NONE should not set WITH (NOLOCK)
Browse files Browse the repository at this point in the history
This fixes the issue detailed in #4391, with SQL Server and SQL Anywhere setting WITH (NOLOCK) for LockMode::NONE, which effectively means using a READ UNCOMMITTED isolation level at table level, which is not the contract of LockMode::NONE.
  • Loading branch information
BenMorel committed Nov 12, 2020
1 parent cad295a commit 3ed11aa
Show file tree
Hide file tree
Showing 7 changed files with 109 additions and 8 deletions.
2 changes: 1 addition & 1 deletion lib/Doctrine/DBAL/Platforms/SQLAnywherePlatform.php
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ public function appendLockHint($fromClause, $lockMode)
{
switch (true) {
case $lockMode === LockMode::NONE:
return $fromClause . ' WITH (NOLOCK)';
return $fromClause;

case $lockMode === LockMode::PESSIMISTIC_READ:
return $fromClause . ' WITH (UPDLOCK)';
Expand Down
2 changes: 1 addition & 1 deletion lib/Doctrine/DBAL/Platforms/SQLServerPlatform.php
Original file line number Diff line number Diff line change
Expand Up @@ -1574,7 +1574,7 @@ public function appendLockHint($fromClause, $lockMode)
{
switch (true) {
case $lockMode === LockMode::NONE:
return $fromClause . ' WITH (NOLOCK)';
return $fromClause;

case $lockMode === LockMode::PESSIMISTIC_READ:
return $fromClause . ' WITH (HOLDLOCK, ROWLOCK)';
Expand Down
101 changes: 101 additions & 0 deletions tests/Doctrine/Tests/DBAL/Functional/LockMode/NoneTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
<?php

declare(strict_types=1);

namespace Doctrine\Tests\DBAL\Functional\LockMode;

use Doctrine\DBAL\Connection;
use Doctrine\DBAL\Driver\OCI8;
use Doctrine\DBAL\Exception;
use Doctrine\DBAL\LockMode;
use Doctrine\DBAL\Platforms\SqlitePlatform;
use Doctrine\DBAL\Platforms\SQLServerPlatform;
use Doctrine\DBAL\Schema\Table;
use Doctrine\DBAL\TransactionIsolationLevel;
use Doctrine\Tests\DbalFunctionalTestCase;
use Doctrine\Tests\TestUtil;

class NoneTest extends DbalFunctionalTestCase
{
/** @var Connection */
private $connection2;

public function setUp(): void
{
parent::setUp();

if ($this->connection->getDriver() instanceof OCI8\Driver) {
// https://github.com/doctrine/dbal/issues/4417
self::markTestSkipped('This test fails on OCI8 for a currently unknown reason');
}

if ($this->connection->getDatabasePlatform() instanceof SQLServerPlatform) {
// Use row versioning instead of locking on SQL Server (if we don't, the second connection will block when
// attempting to read the row created by the first connection, instead of reading the previous version);
// for some reason we cannot set READ_COMMITTED_SNAPSHOT ON when not running this test in isolation,
// there may be another connection active at this point; temporarily forcing to SINGLE_USER does the trick.
$db = $this->connection->getDatabase();
$this->connection->executeStatement('ALTER DATABASE ' . $db . ' SET SINGLE_USER WITH ROLLBACK IMMEDIATE');
$this->connection->executeStatement('ALTER DATABASE ' . $db . ' SET READ_COMMITTED_SNAPSHOT ON');
$this->connection->executeStatement('ALTER DATABASE ' . $db . ' SET MULTI_USER');
}

$table = new Table('users');
$table->addColumn('id', 'integer');
$table->setPrimaryKey(['id']);

$this->connection->getSchemaManager()->dropAndCreateTable($table);

$this->connection2 = TestUtil::getConnection();

if ($this->connection2->getSchemaManager()->tablesExist('users')) {
return;
}

if ($this->connection2->getDatabasePlatform() instanceof SqlitePlatform) {
self::markTestSkipped('This test cannot run on SQLite using an in-memory database');
}

self::fail('Separate connections do not seem to talk to the same database');
}

public function tearDown(): void
{
parent::tearDown();

if ($this->connection2->isTransactionActive()) {
$this->connection2->rollBack();
}

$this->connection2->close();

$this->connection->getSchemaManager()->dropTable('users');

if (! $this->connection->getDatabasePlatform() instanceof SQLServerPlatform) {
return;
}

$db = $this->connection->getDatabase();
$this->connection->executeStatement('ALTER DATABASE ' . $db . ' SET READ_COMMITTED_SNAPSHOT OFF');
}

public function testLockModeNoneDoesNotBreakTransactionIsolation(): void
{
try {
$this->connection->setTransactionIsolation(TransactionIsolationLevel::READ_COMMITTED);
$this->connection2->setTransactionIsolation(TransactionIsolationLevel::READ_COMMITTED);
} catch (Exception $e) {
self::markTestSkipped('This test must be able to set a transaction isolation level');
}

$this->connection->beginTransaction();
$this->connection2->beginTransaction();

$this->connection->insert('users', ['id' => 1]);

$query = 'SELECT id FROM users';
$query = $this->connection2->getDatabasePlatform()->appendLockHint($query, LockMode::NONE);

self::assertFalse($this->connection2->fetchOne($query));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -374,14 +374,14 @@ public function testModifyLimitQueryWithOrderByClause(): void
}

$sql = 'SELECT m0_.NOMBRE AS NOMBRE0, m0_.FECHAINICIO AS FECHAINICIO1, m0_.FECHAFIN AS FECHAFIN2'
. ' FROM MEDICION m0_ WITH (NOLOCK)'
. ' FROM MEDICION m0_'
. ' INNER JOIN ESTUDIO e1_ ON m0_.ESTUDIO_ID = e1_.ID'
. ' INNER JOIN CLIENTE c2_ ON e1_.CLIENTE_ID = c2_.ID'
. ' INNER JOIN USUARIO u3_ ON c2_.ID = u3_.CLIENTE_ID'
. ' WHERE u3_.ID = ? ORDER BY m0_.FECHAINICIO DESC';

$alteredSql = 'SELECT TOP 15 m0_.NOMBRE AS NOMBRE0, m0_.FECHAINICIO AS FECHAINICIO1, m0_.FECHAFIN AS FECHAFIN2'
. ' FROM MEDICION m0_ WITH (NOLOCK)'
. ' FROM MEDICION m0_'
. ' INNER JOIN ESTUDIO e1_ ON m0_.ESTUDIO_ID = e1_.ID'
. ' INNER JOIN CLIENTE c2_ ON e1_.CLIENTE_ID = c2_.ID'
. ' INNER JOIN USUARIO u3_ ON c2_.ID = u3_.CLIENTE_ID'
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -266,7 +266,7 @@ public static function getLockHints(): iterable
[null, ''],
[false, ''],
[true, ''],
[LockMode::NONE, ' WITH (NOLOCK)'],
[LockMode::NONE, ''],
[LockMode::OPTIMISTIC, ''],
[LockMode::PESSIMISTIC_READ, ' WITH (UPDLOCK)'],
[LockMode::PESSIMISTIC_WRITE, ' WITH (XLOCK)'],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -224,14 +224,14 @@ public function testModifyLimitQueryWithExtraLongQuery(): void
public function testModifyLimitQueryWithOrderByClause(): void
{
$sql = 'SELECT m0_.NOMBRE AS NOMBRE0, m0_.FECHAINICIO AS FECHAINICIO1, m0_.FECHAFIN AS FECHAFIN2'
. ' FROM MEDICION m0_ WITH (NOLOCK)'
. ' FROM MEDICION m0_'
. ' INNER JOIN ESTUDIO e1_ ON m0_.ESTUDIO_ID = e1_.ID'
. ' INNER JOIN CLIENTE c2_ ON e1_.CLIENTE_ID = c2_.ID'
. ' INNER JOIN USUARIO u3_ ON c2_.ID = u3_.CLIENTE_ID'
. ' WHERE u3_.ID = ? ORDER BY m0_.FECHAINICIO DESC';

$expected = 'SELECT m0_.NOMBRE AS NOMBRE0, m0_.FECHAINICIO AS FECHAINICIO1, m0_.FECHAFIN AS FECHAFIN2'
. ' FROM MEDICION m0_ WITH (NOLOCK)'
. ' FROM MEDICION m0_'
. ' INNER JOIN ESTUDIO e1_ ON m0_.ESTUDIO_ID = e1_.ID'
. ' INNER JOIN CLIENTE c2_ ON e1_.CLIENTE_ID = c2_.ID'
. ' INNER JOIN USUARIO u3_ ON c2_.ID = u3_.CLIENTE_ID'
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ public static function getLockHints(): iterable
{
return [
[null, ''],
[LockMode::NONE, ' WITH (NOLOCK)'],
[LockMode::NONE, ''],
[LockMode::OPTIMISTIC, ''],
[LockMode::PESSIMISTIC_READ, ' WITH (HOLDLOCK, ROWLOCK)'],
[LockMode::PESSIMISTIC_WRITE, ' WITH (UPDLOCK, ROWLOCK)'],
Expand Down

0 comments on commit 3ed11aa

Please sign in to comment.