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.

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

Clarify LockMode::NONE #4391

Closed
BenMorel opened this issue Oct 29, 2020 · 7 comments · Fixed by #4400
Closed

Clarify LockMode::NONE #4391

BenMorel opened this issue Oct 29, 2020 · 7 comments · Fixed by #4400
Labels
Milestone

Comments

@BenMorel
Copy link
Contributor

BenMorel commented Oct 29, 2020

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
Copy link
Member

morozov commented Oct 30, 2020

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
Copy link
Contributor Author

BenMorel commented Nov 1, 2020

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:

  • In the DBAL, branch 2.12.x: remove the hint for LockMode::NONE in SQLServer2012Platform

Proposed improvement:

  • In the ORM, master branch:
    • stop requiring a transaction for LockMode::NONE

    • set the default lock mode to LockMode::NONE everywhere null is the default.

      For example, EntityManager::find() would change from:

      public function find(string $entityName, $id, ?int $lockMode = null, ?int $lockVersion = null)

      to:

      public function find(string $entityName, $id, int $lockMode = LockMode::NONE, ?int $lockVersion = null)

@BenMorel
Copy link
Contributor Author

BenMorel commented Nov 1, 2020

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 added a commit to BenMorel/dbal that referenced this issue Nov 2, 2020
This fixes the issue detailed in doctrine#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.
BenMorel added a commit to BenMorel/dbal that referenced this issue Nov 12, 2020
This fixes the issue detailed in doctrine#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.
@BenMorel
Copy link
Contributor Author

@morozov
Copy link
Member

morozov commented Nov 13, 2020

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

@morozov morozov linked a pull request Nov 13, 2020 that will close this issue
@morozov morozov added this to the 2.12.1 milestone Nov 13, 2020
@BenMorel
Copy link
Contributor Author

Nope, we can continue the discussion in doctrine/orm#8341 now.

@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants