LockMode::NONE should not set WITH (NOLOCK) by BenMorel · Pull Request #4400 · doctrine/dbal

BenMorel · 2020-11-01T22:25:39Z

Q	A
Type	bug
BC Break	no
Fixed issues	#4391

Summary

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.

greg0ire · 2020-11-02T18:24:13Z

Please improve your commit message according to the contributing guide.

BenMorel · 2020-11-02T22:35:37Z

@greg0ire Done!

SenseException

If this is handled as a bugfix, this looks 👍 after reading a bit more about WITH (NOLOCK) behavior.

BenMorel · 2020-11-08T16:50:41Z

Does this PR need approval from someone else?

morozov · 2020-11-08T17:57:34Z

@BenMorel I believe this change cannot be covered with an integration test but it still requires a reproducer that could be run manually across all supported platforms to confirm that the fix is valid. See the testing guidelines (#4356) and #4279 for an example.

BenMorel · 2020-11-08T19:19:46Z

@morozov Would it be OK if I add a test that uses 2 separate connections (using TestUtil::getConnection()) to ensure that SQL executed by one uncommitted transaction is not visible by another transaction?

morozov · 2020-11-08T19:36:36Z

@BenMorel, absolutely. If the scenario in question can be reproduced using the existing APIs, please add a test for it.

BenMorel · 2020-11-08T23:13:17Z

To ensure that we're realling fixing something that's broken, I created a failing test case in #4415; it fails as expected on SQL Server, but does anyone know why it's also failing on Oracle?

Basically, given 2 transactions in READ COMMITTED isolation, T2 is allowed to see a row that's been inserted by T1 but not yet committed. Bug?

morozov · 2020-11-09T16:44:02Z

Bug?

No idea. You can mark this test incomplete on Oracle since it's out of scope.

As far as I understand the code change, it makes the behavior of null and NONE identical. Which raises the question of whether we should support both and whether the call to "append" with "none" makes any sense.

BenMorel · 2020-11-09T17:04:17Z

You can mark this test incomplete on Oracle since it's out of scope.

OK, and I'll open a bug to keep track of this, to ensure that we're not overlooking something big here.

As far as I understand the code change, it makes the behavior of null and NONE identical. Which raises the question of whether we should support both and whether the call to "append" with "none" makes any sense.

Yep, that's why I'm advocating to remove support for null, and use LockMode::NONE as the new default. The call to "append" with NONE is fine IMO, as long as the method returns the original string untouched.

morozov · 2020-11-09T17:07:41Z

Both points sound good.

BenMorel · 2020-11-11T11:18:58Z

Does anyone have a SQL Server installation at hand?

I'm struggling to create a working test case here. The problem is: by default, in READ COMMITTED isolation, SQL Server blocks all reads on a row that's being updated by another transaction, so my test case blocks indefinitely on the second connection.

I've read here that we can set the following to make SQL Server use row versioning instead of locking:

ALTER DATABASE doctrine SET READ_COMMITTED_SNAPSHOT ON;

This should, in theory, make the query in the second transaction return immediately, but it still blocks. This is contrary to what I can read in the linked article.

I would appreciate a helping hand here!

morozov · 2020-11-11T15:23:46Z

Does anyone have a SQL Server installation at hand?

You can spin up a server from the same Docker image as we use on CI:

dbal/.github/workflows/continuous-integration.yml

Line 429 in 7b5d241

image: "microsoft/mssql-server-linux:2017-latest"

morozov · 2020-11-12T04:50:07Z

@BenMorel please avoid experimentation on CI. Each build job of this PR has locked and timed out after one hour. Given that we run 4 jobs sequentially on AppVeyor, a single build effectively blocked the pipeline for 4 hours.

BenMorel · 2020-11-12T08:01:08Z

@morozov I'm sorry, but I could not repeat the lock locally using the Docker image; at least, not while executing queries manually on the CLI, where it works as expected.

I'll try to do my best to investigate further locally and attempt to not block CI again.

BenMorel · 2020-11-12T13:23:08Z

@morozov, I finally got it to work, with quite a lot of local setup & trial-and-error.

I can't say it's really pretty, but I think it's as pretty as an integration test with 2 connections can be, especially on this quite complex RDBMS.

We now have the same tests failing on SQL server in #4415, and passing here.

Let me know if you need anything else to get this merged! 👍

tests/Doctrine/Tests/DBAL/Functional/LockModeTest.php

lib/Doctrine/DBAL/Platforms/SQLAnywherePlatform.php

tests/Doctrine/Tests/DBAL/Functional/LockModeTest.php

BenMorel · 2020-11-12T18:17:00Z

@morozov All your feedback has been processed. Please review.

morozov · 2020-11-12T18:23:24Z

Looks good! Please squash, and let's get it merged.

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 · 2020-11-12T21:26:24Z

@morozov Squashed, and I opened #4428 to keep track of #4400 (comment).

morozov · 2020-11-12T22:35:39Z

Thanks, @BenMorel!

BenMorel · 2020-11-13T09:33:01Z

Thank you, could you please merge 2.12.x into 3.0.x, so that I can take care of #4428?

morozov · 2020-11-13T16:13:03Z

@BenMorel I already filed #4429.

@morozov

Release [2.12.1](https://github.com/doctrine/dbal/milestone/84) 2.12.1 ====== - Total issues resolved: **2** - Total pull requests resolved: **11** - Total contributors: **7** Documentation,Prepared Statements --------------------------------- - [4424: Mark SQLParserUtils internal](doctrine#4424) thanks to @morozov Packaging --------- - [4416: Update .gitattributes](doctrine#4416) thanks to @bytestream Bug,Cache --------- - [4414: ResultCacheStatement::fetchAllAssociative does not store results in cache](doctrine#4414) thanks to @morozov and @dFayet Deprecation,Prepared Statements ------------------------------- - [4411: Deprecate inappropriate usage of prepared statement parameters](doctrine#4411) thanks to @morozov - [4407: Deprecate colon prefix for prepared statement parameters](doctrine#4407) thanks to @morozov Static Analysis --------------- - [4403: Remove redundant phpstan param from DriverManager::getConnection()](doctrine#4403) thanks to @simPod Bug,Locking,Transactions ------------------------ - [4400: LockMode::NONE should not set WITH (NOLOCK)](doctrine#4400) thanks to @BenMorel Code Style,PHP -------------- - [4398: Update PHP&doctrine#95;CodeSniffer to 3.5.8](doctrine#4398) thanks to @morozov PDO,PHP,Test Suite ------------------ - [4396: Fix php8 mysql mariadb](doctrine#4396) thanks to @greg0ire Documentation ------------- - [4390: Fix headline in the upgrade docs](doctrine#4390) thanks to @jdreesen Documentation,Testing --------------------- - [4356: Testing Guidelines](doctrine#4356) thanks to @morozov # gpg: Signature made Sat Nov 14 21:50:01 2020 # gpg: using DSA key 1BEDEE0A820BC30D858F9F0C2C3A645671828132 # gpg: Can't check signature: No public key # Conflicts: # README.md

BenMorel mentioned this pull request Nov 1, 2020

Clarify LockMode::NONE #4391

Closed

BenMorel force-pushed the with-nolock branch from 787b7fc to d7fb61b Compare November 2, 2020 22:34

SenseException previously approved these changes Nov 3, 2020

View reviewed changes

greg0ire requested a review from morozov November 8, 2020 17:25

BenMorel added a commit to BenMorel/dbal that referenced this pull request Nov 8, 2020

Failing test case for doctrine#4400

a0727d9

BenMorel mentioned this pull request Nov 8, 2020

Failing test case for #4400 #4415

Closed

BenMorel added a commit to BenMorel/dbal that referenced this pull request Nov 8, 2020

Failing test case for doctrine#4400

115e17b

BenMorel added a commit to BenMorel/dbal that referenced this pull request Nov 8, 2020

Failing test case for doctrine#4400

77b960b

BenMorel added a commit to BenMorel/dbal that referenced this pull request Nov 8, 2020

Failing test case for doctrine#4400

d671145

BenMorel dismissed SenseException’s stale review via bc8b0c7 November 10, 2020 13:33

BenMorel force-pushed the with-nolock branch 4 times, most recently from 5f9cbc6 to e9eb3fa Compare November 12, 2020 10:29

BenMorel added a commit to BenMorel/dbal that referenced this pull request Nov 12, 2020

Update failing test case to match doctrine#4400

e80eee2

morozov reviewed Nov 12, 2020

View reviewed changes

morozov added Bug Locking Transactions labels Nov 12, 2020

morozov added this to the 2.12.1 milestone Nov 12, 2020

BenMorel force-pushed the with-nolock branch from c20d7da to 3ed11aa Compare November 12, 2020 20:59

BenMorel mentioned this pull request Nov 12, 2020

Throw exception on invalid LockMode #4428

Closed

morozov approved these changes Nov 12, 2020

View reviewed changes

morozov merged commit 74bc147 into doctrine:2.12.x Nov 12, 2020

BenMorel mentioned this pull request Nov 13, 2020

Use LockMode::NONE as default lock mode doctrine/orm#8341

Closed

morozov linked an issue Nov 13, 2020 that may be closed by this pull request

Clarify LockMode::NONE #4391

Closed

dependabot bot mentioned this pull request Mar 18, 2021

composer(deps): update doctrine/dbal requirement from ^2.12 to ^2.12 || ^3.0 Setono/ClientIdBundle#3

Closed

github-actions bot locked as resolved and limited conversation to collaborators Jul 26, 2022

Uh oh!

Conversation

BenMorel commented Nov 1, 2020

Summary

Uh oh!

greg0ire commented Nov 2, 2020

Uh oh!

BenMorel commented Nov 2, 2020

Uh oh!

SenseException left a comment

Choose a reason for hiding this comment

Uh oh!

BenMorel commented Nov 8, 2020

Uh oh!

morozov commented Nov 8, 2020

Uh oh!

BenMorel commented Nov 8, 2020

Uh oh!

morozov commented Nov 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BenMorel commented Nov 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

morozov commented Nov 9, 2020

Uh oh!

BenMorel commented Nov 9, 2020

Uh oh!

morozov commented Nov 9, 2020

Uh oh!

BenMorel commented Nov 11, 2020

Uh oh!

morozov commented Nov 11, 2020

Uh oh!

morozov commented Nov 12, 2020

Uh oh!

BenMorel commented Nov 12, 2020

Uh oh!

BenMorel commented Nov 12, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

BenMorel commented Nov 12, 2020

Uh oh!

morozov commented Nov 12, 2020

Uh oh!

BenMorel commented Nov 12, 2020

Uh oh!

morozov commented Nov 12, 2020

Uh oh!

BenMorel commented Nov 13, 2020

Uh oh!

morozov commented Nov 13, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

morozov commented Nov 8, 2020 •

edited

Loading

BenMorel commented Nov 8, 2020 •

edited

Loading