doctrine / dbal

Doctrine Database Abstraction Layer
https://www.doctrine-project.org/projects/dbal.html
MIT License
9.44k stars 1.33k forks source link

Clarify LockMode::NONE #4391

Closed BenMorel closed 3 years ago

BenMorel commented 3 years ago

I feel like there is some confusion with LockMode::NONE.

In all platforms but SQLServer and SQLAnywhere, LockMode::NONE is equivalent to not giving a lock hint.

I these 2 platforms however, LockMode::NONE adds a WITH (NOLOCK) hint, which as I understand it, allows dirty reads, effectively changing the isolation level to READ UNCOMMITTED for this table.

Is this really what was intended?

In the ORM for example, using null as a lock mode in EntityManager::find() is different from using LockMode::NONE , but only on these 2 platforms:

Lock mode SQLServer/SQLAnywhere Other platforms
null no hint no hint
LockMode::NONE WITH(NOLOCK) no hint

And the read semantics are therefore different across platforms for LockMode::NONE, which I believe is a bug.

morozov commented 3 years ago

Is this really what was intended?

I believe this is one of those DBAL features that were implemented w/o integration testing and as a result in a non-portable way, so I cannot answer your question.

You are welcome to do the research and propose the changes that will make this API clearer and/or better portable between the DB platforms.

BenMorel commented 3 years ago

I came across this issue when I explicitly used LockMode::NONE in the ORM, and was greeted with an exception telling me that a transaction was required. Digging a bit made me find this.

Now that SQLAnywhere is gone, we're only left with the special case of SQLServer. SQLServer2012Platform is the only platform left that implements appendLockHint(), and therefore has an opportunity to set a hint for LockMode::NONE:

public function appendLockHint($fromClause, $lockMode)
{
    switch (true) {
        case $lockMode === LockMode::NONE:
            return $fromClause . ' WITH (NOLOCK)';

        case $lockMode === LockMode::PESSIMISTIC_READ:
            return $fromClause . ' WITH (HOLDLOCK, ROWLOCK)';

        case $lockMode === LockMode::PESSIMISTIC_WRITE:
            return $fromClause . ' WITH (UPDLOCK, ROWLOCK)';

        default:
            return $fromClause;
    }
}

All the other platforms only get a chance to set a hint via getReadLockSQL() and getWriteLockSQL(), which directly map to LockMode::PESSIMISTIC_READ and LockMode::PESSIMISTIC_WRITE.

Here is a detailed comparison of the behaviour in the ORM:

Lock mode SQL Server MySQL / MariaDB PostgreSQL DB2 SQLite Oracle
null no hint no hint no hint no hint no hint no hint
LockMode::NONE WITH (NOLOCK) no hint no hint no hint no hint no hint
LockMode::PESSIMISTIC_READ WITH (HOLDLOCK, ROWLOCK) LOCK IN SHARE MODE FOR SHARE WITH RR USE AND KEEP UPDATE LOCKS no hint FOR UPDATE
LockMode::PESSIMISTIC_WRITE WITH (UPDLOCK, ROWLOCK) FOR UPDATE FOR UPDATE WITH RR USE AND KEEP UPDATE LOCKS no hint FOR UPDATE

So, SQL Server is the only platform to make the distinction between no lock mode (null) vs using LockMode::NONE explicitly.

Because WITH (NOLOCK) is the equivalent of using READ UNCOMMITED as a transaction isolation level, I believe this is a bug.

Proposed resolution:

Proposed improvement:

BenMorel commented 3 years ago

Created #4400 which fixes the issue on 2.12.x; I fixed SQL Anywhere as well of course, because it's still there in 2.x.

Doc for SQLAnywhere: http://dcx.sap.com/1200/fr/dbreference/from-statement.html

NOLOCK | Sets the behavior to be equivalent to isolation level 0. This table hint is synonymous with READUNCOMMITTED.

Which is consistent with the behaviour on SQL Server.

BenMorel commented 3 years ago
morozov commented 3 years ago

@BenMorel do you expect any more work done on this issue? Otherwise, it can be closed.

BenMorel commented 3 years ago

Nope, we can continue the discussion in https://github.com/doctrine/orm/pull/8341 now.

github-actions[bot] commented 2 years ago

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.