[SERVER-80463] MigrationChunkClonerSourceOpObserver::onInserts() written to look like it skips checking some documents for whether their chunk has moved Created: 28/Aug/23  Updated: 29/Oct/23  Resolved: 28/Aug/23

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

Type: Bug Priority: Minor - P4
Reporter: Max Hirschhorn Assignee: Max Hirschhorn
Resolution: Fixed Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Backports
Problem/Incident
is caused by SERVER-70437 Lost writes to unsharded collection d... Closed
Backwards Compatibility: Fully Compatible
Operating System: ALL
Backport Requested:
v7.0
Sprint: Sharding NYC 2023-09-04
Participants:

 Description   

The changes from 0323cbd as part of SERVER-70437 changed the interface of shardObserveInsertOp() to become the plural shardObserveInsertsOp(). However the changes mistakenly copied over the early return inside the for-loop. This turns out not to be a transaction consistency problem in practice because in every version where cross-shard transactions are supported (since MongoDB 4.2), the changes from SERVER-38583 had already made it so onInserts() is only called with a single element at a time.

We should change the early return to a continue statement to reflect the intended control flow though.

134
for (auto it = first; it != last; it++, index++) {
135
    auto opTime = opTimeList.empty() ? repl::OpTime() : opTimeList[index];
136
 
137
    if (inMultiDocumentTransaction) {
138
        const auto atClusterTime = repl::ReadConcernArgs::get(opCtx).getArgsAtClusterTime();
139
 
140
        if (atClusterTime) {
141
            const auto shardKey =
142
                metadata->getShardKeyPattern().extractShardKeyFromDocThrows(it->doc);
143
            assertIntersectingChunkHasNotMoved(opCtx, *metadata, shardKey, *atClusterTime);
144
        }
145
 
146
        return;
147
    }
148
 
149
    auto cloner = MigrationSourceManager::getCurrentCloner(*csr);
150
    if (cloner) {
151
        cloner->onInsertOp(opCtx, it->doc, opTime);
152
    }
153
}



 Comments   
Comment by Githook User [ 29/Aug/23 ]

Author:

{'name': 'Max Hirschhorn', 'email': 'max.hirschhorn@mongodb.com', 'username': 'visemet'}

Message: SERVER-80463 Match intended control flow for snapshot txn inserts.

The early return wasn't a problem in practice because onInserts() is
called with a batch of size 1 when the operation is running in a
multi-statement transaction.

(cherry picked from commit 725e331ee2042a11f78e6fb5a52ede42fddcf52e)
Branch: v7.0
https://github.com/mongodb/mongo/commit/03c0b142d55963e2cf2b8a304ee6a989dad650e9

Comment by Githook User [ 28/Aug/23 ]

Author:

{'name': 'Max Hirschhorn', 'email': 'max.hirschhorn@mongodb.com', 'username': 'visemet'}

Message: SERVER-80463 Match intended control flow for snapshot txn inserts.

The early return wasn't a problem in practice because onInserts() is
called with a batch of size 1 when the operation is running in a
multi-statement transaction.
Branch: master
https://github.com/mongodb/mongo/commit/725e331ee2042a11f78e6fb5a52ede42fddcf52e

Generated at Thu Feb 08 06:43:37 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.