[SERVER-44906] Rollback should take global write lock while truncating oplog Created: 02/Dec/19  Updated: 06/Dec/22

Status: Backlog
Project: Core Server
Component/s: Replication
Affects Version/s: None
Fix Version/s: None

Type: Improvement Priority: Major - P3
Reporter: A. Jesse Jiryu Davis Assignee: Backlog - Replication Team
Resolution: Unresolved Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Depends
is depended on by SERVER-45626 Consistent Oplog Locking Rules Closed
Related
is related to SERVER-45525 ReplBatcher should explicitly read at... Closed
Assigned Teams:
Replication
Participants:

 Description   

In ReplicationRecoveryImpl::_truncateOplogTo we lock the oplog's database and collection; however, we have a general principle that we can read from the oplog with a global IS lock, write to it without locking the database and collection.

For example the oplog cap maintainer thread only takes the global IX lock to truncate old entries in OplogCapMaintainerThread::_deleteExcessDocuments.

Let's change ReplicationRecoveryImpl to take the global X or IX lock (I'm not sure which) instead of locking the oplog's db and collection.



 Comments   
Comment by A. Jesse Jiryu Davis [ 30/Jan/20 ]

Perhaps a new RAII type named like "AugutGetOplogForRead" could enforce the locking rules. It may also have a different default read timestamp from AutoGetCollection(ForRead), which could help prevent bugs like SERVER-45525.

Comment by A. Jesse Jiryu Davis [ 03/Dec/19 ]

Same for StorageInterfaceImpl::waitForAllEarlierOplogWritesToBeVisible, which takes an IS lock on the oplog. It includes a comment:

void StorageInterfaceImpl::waitForAllEarlierOplogWritesToBeVisible(OperationContext* opCtx,
                                                                   bool primaryOnly) {
    Lock::GlobalLock lk(opCtx, MODE_IS);
    if (primaryOnly &&
        !repl::ReplicationCoordinator::get(opCtx)->canAcceptWritesForDatabase(opCtx, "admin"))
        return;
    Collection* oplog;
    {
        // We don't want to be holding the collection lock while blocking, to avoid deadlocks.
        // It is safe to store and access the oplog's Collection object with just the global IS
        // lock because of the special concurrency rules for the oplog.
        // TODO(spencer): It should be possible to get the pointer to the oplog Collection object
        // without ever having to take the collection lock.
        AutoGetCollection oplogLock(opCtx, NamespaceString::kRsOplogNamespace, MODE_IS);
        oplog = oplogLock.getCollection();
    }
    uassert(ErrorCodes::NotYetInitialized, "The oplog does not exist", oplog);
    oplog->getRecordStore()->waitForAllEarlierOplogWritesToBeVisible(opCtx);
}

Comment by Eric Milkie [ 02/Dec/19 ]

True. It won't hurt to clean this up then. I would use a Global IX lock for this.

Comment by A. Jesse Jiryu Davis [ 02/Dec/19 ]

For consistency with the locking used in other parts of the code where we touch the oplog. In cases where we might write new code that touches the oplog (I'm working on this in SERVER-43589), it's good to know what locks we'll need.

Comment by Eric Milkie [ 02/Dec/19 ]

For what reason do you want to do this?
We already block all external reads and writes with the node being in ROLLBACK state, outside the locking system.

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