[SERVER-53817] Remove check for shouldConflictWithSecondaryBatchApplication() in isCollectionLockedForMode() Created: 14/Jan/21  Updated: 29/Oct/23  Resolved: 28/Jul/23

Status: Closed
Project: Core Server
Component/s: None
Affects Version/s: None
Fix Version/s: 7.1.0-rc0

Type: Task Priority: Major - P3
Reporter: Henrik Edin Assignee: Gregory Noma
Resolution: Fixed Votes: 0
Labels: auto-reverted, techdebt
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Depends
depends on SERVER-53846 Clarify locking requirements in creat... Closed
depends on SERVER-79319 Modify collection lock assertions on ... Closed
depends on SERVER-79320 Modify collection lock assertions for... Closed
depends on SERVER-79322 Modify collection lock assertion when... Closed
depends on SERVER-79323 Modify collection lock check in SBE p... Closed
depends on SERVER-79343 Modify system.views locking assertion... Closed
depends on SERVER-79344 Modify collection lock assertion for ... Closed
depends on SERVER-79345 Modify collection lock assertions in ... Closed
depends on SERVER-79346 Modify collection lock assertion in c... Closed
depends on SERVER-79347 Modify collection lock assertion in r... Closed
depends on SERVER-79348 Modify collection lock assertion when... Closed
is depended on by SERVER-79399 Remove ShouldNotConflictWithSecondary... Closed
Duplicate
is duplicated by SERVER-77474 LockerImpl::isCollectionLockedForMode... Closed
Problem/Incident
Assigned Teams:
Storage Execution
Backwards Compatibility: Fully Compatible
Sprint: Execution NAMR Team 2023-08-07
Participants:
Linked BF Score: 169

 Description   

isCollectionLockedForMode() has a special case where it always returns true if shouldConflictWithSecondaryBatchApplication returns false:
https://github.com/mongodb/mongo/blob/1522b29e65bffd4c438756e5202aef062410864d/src/mongo/db/concurrency/lock_state.cpp#L630-L631

While we might not need to hold locks in this special mode it causes seemingly confusing behavior where the locker reports that locks are held.

We have invariants throughout the codebase that checks if Collections are locked in certain modes. Here are a few that would start firing in certain conditions if the shouldConflictWithSecondaryBatchApplication check is removed from isCollectionLockedForMode

https://github.com/mongodb/mongo/blob/3a4dda6f0dc7f32e91310b9256cd3b499f7715d2/src/mongo/db/pipeline/pipeline_d.cpp#L110-L112

https://github.com/mongodb/mongo/blob/bc41e7992d3c4e84a532e3833623b8e237a281e4/src/mongo/db/storage/wiredtiger/wiredtiger_record_store.cpp#L2458-L2460

https://github.com/mongodb/mongo/blob/bc41e7992d3c4e84a532e3833623b8e237a281e4/src/mongo/db/s/collection_sharding_state.cpp#L120-L121

https://github.com/mongodb/mongo/blob/bc41e7992d3c4e84a532e3833623b8e237a281e4/src/mongo/db/catalog/database_impl.cpp#L529-L530



 Comments   
Comment by Githook User [ 28/Jul/23 ]

Author:

{'name': 'Gregory Noma', 'email': 'gregory.noma@gmail.com', 'username': 'gregorynoma'}

Message: SERVER-53817 Remove confusing condition when checking collection lock
Branch: master
https://github.com/mongodb/mongo/commit/49ef575375b966620f26bfbac71edb768427a73c

Comment by xgen-buildbaron-user [ 28/Jul/23 ]

Ticket re-opened due to revert. change_streams_multitenant_passthrough began a consistent failure of jstests/change_streams/pipeline_style_updates.js

Comment by Githook User [ 28/Jul/23 ]

Author:

{'name': 'auto-revert-processor', 'email': 'dev-prod-dag@mongodb.com', 'username': ''}

Message: Revert "SERVER-53817 Remove confusing condition when checking collection lock"

This reverts commit 7361e4e5bf9758705fb46c2cc022defa22c4042f.
Branch: master
https://github.com/mongodb/mongo/commit/ef8504eefef4c9fe2062b8424e85053b3e1ad8d2

Comment by Githook User [ 27/Jul/23 ]

Author:

{'name': 'Gregory Noma', 'email': 'gregory.noma@gmail.com', 'username': 'gregorynoma'}

Message: SERVER-53817 Remove confusing condition when checking collection lock
Branch: master
https://github.com/mongodb/mongo/commit/7361e4e5bf9758705fb46c2cc022defa22c4042f

Comment by Jordi Olivares Provencio [ 25/Jul/23 ]

We set the flag for shouldConflictWithSecondaryBatchApplication to false on multi-document transactions as well. I didn't know this ticket existed and filed SERVER-77474 for this.

Comment by Dianna Hohensee (Inactive) [ 05/Feb/21 ]

Copying this here, as it is enlightening about why this lock bypass exists:

"Apparently, for secondary oplog application, the writer threads expect the main applier thread to hold locks for them. The main applier thread runs X lock operations serially, so no other writers are active in the system. Therefore, isCollectionLockedForMode returns true if shouldConflictWithSecondaryBatchApplication==false as a signal that is what is happening."

Generated at Thu Feb 08 05:31:56 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.