LockMode::NONE should not set WITH (NOLOCK) #4400
Conversation
|
Please improve your commit message according to the contributing guide. |
|
@greg0ire Done! |
|
Does this PR need approval from someone else? |
|
@morozov Would it be OK if I add a test that uses 2 separate connections (using |
|
@BenMorel, absolutely. If the scenario in question can be reproduced using the existing APIs, please add a test for it. |
|
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 |
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 |
OK, and I'll open a bug to keep track of this, to ensure that we're not overlooking something big here.
Yep, that's why I'm advocating to remove support for |
|
Both points sound good. |
|
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 I've read here that we can set the following to make SQL Server use row versioning instead of locking:
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! |
You can spin up a server from the same Docker image as we use on CI: |
|
@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. |
|
@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
force-pushed
the
with-nolock
branch
4 times, most recently
from
5f9cbc6
to
e9eb3fa
Compare
|
@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! 👍 |
|
@morozov All your feedback has been processed. Please review. |
|
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
force-pushed
the
with-nolock
branch
from
c20d7da
to
3ed11aa
Compare
|
@morozov Squashed, and I opened #4428 to keep track of #4400 (comment). |
|
Thanks, @BenMorel! |
|
Thank you, could you please merge 2.12.x into 3.0.x, so that I can take care of #4428? |
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
Summary
This fixes the issue detailed in #4391, with SQL Server and SQL Anywhere setting
WITH (NOLOCK)forLockMode::NONE, which effectively means using aREAD UNCOMMITTEDisolation level at table level, which is not the contract ofLockMode::NONE.