Batch lookup primary table for secondary index scan#404
Batch lookup primary table for secondary index scan#404githubzilla merged 41 commits intoeloqdata:mainfrom
Conversation
WalkthroughThis PR introduces lazy prefetch infrastructure for Eloq index cursors by adding prefetch-state management, batch sorting with result restoration, server parameters for batch size control, and accessor methods to expose internal cursor state for downstream operations. Changes
Sequence Diagram(s)sequenceDiagram
participant CursorOp as Cursor Operation
participant Cursor as EloqIndexCursor
participant PrefetchMgr as Prefetch Manager
participant BatchFetcher as Batch Fetcher
participant KVStore as KV Store
CursorOp->>Cursor: _updatePosition()
activate Cursor
Cursor->>PrefetchMgr: _ensureRecordsFetched()
activate PrefetchMgr
PrefetchMgr->>PrefetchMgr: Check if current position<br/>outside prefetch range
alt Prefetch needed
PrefetchMgr->>BatchFetcher: _fetchRecordsForRange(startIdx, batchSize)
activate BatchFetcher
BatchFetcher->>BatchFetcher: Construct RecordIds<br/>from scan batch
BatchFetcher->>KVStore: batchGetKV(sorted records)
activate KVStore
KVStore-->>BatchFetcher: records
deactivate KVStore
BatchFetcher->>BatchFetcher: Restore original order
BatchFetcher->>PrefetchMgr: Populate _prefetchedRecords
deactivate BatchFetcher
end
deactivate PrefetchMgr
Cursor->>Cursor: _updateRecordPtr()<br/>using prefetched records
deactivate Cursor
Cursor-->>CursorOp: Updated cursor state
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/mongo/db/modules/eloq/src/eloq_index.cpp (2)
529-590: Consider extracting duplicated prefetch lookup logic.The prefetched record lookup logic in
IndexCursorType::UNIQUE(lines 534-558) andIndexCursorType::STANDARD(lines 570-589) cases is nearly identical. Extracting this to a private helper method would reduce duplication and improve maintainability.🔎 Suggested helper method
// Add as private method const Eloq::MongoRecord* _lookupPrefetchedRecord() { if (!_cursor) { return nullptr; } size_t scanBatchIdx = _cursor->getScanBatchIdx(); if (scanBatchIdx >= _prefetchedBatchStartIdx) { size_t offset = scanBatchIdx - _prefetchedBatchStartIdx; if (offset < _prefetchedRecords.size() && _prefetchedRecords[offset] != nullptr) { return _prefetchedRecords[offset].get(); } MONGO_LOG(1) << "RecordId " << _id.toString() << " not found in prefetched records at offset " << offset; } else { MONGO_LOG(1) << "Current scan index " << scanBatchIdx << " is before prefetched range starting at " << _prefetchedBatchStartIdx; } return nullptr; }
446-454: Unreachable return statement afteruassertStatusOK.The
return;on line 453 is unreachable becauseuassertStatusOKthrows an exception when the error code is not NO_ERROR. This is dead code.🔎 Remove unreachable return
if (err != txservice::TxErrorCode::NO_ERROR) { MONGO_LOG(1) << "Batch fetch failed for table " << tableName.StringView() << ", range [" << startIdx << "-" << endIdx << "), error: " << err; // Clear prefetched records on error and return error _prefetchedRecords.clear(); _prefetchedBatchStartIdx = 0; uassertStatusOK(TxErrorCodeToMongoStatus(err)); - return; }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/mongo/db/modules/eloq/src/eloq_cursor.hsrc/mongo/db/modules/eloq/src/eloq_index.cppsrc/mongo/db/query/query_knobs.cppsrc/mongo/db/query/query_knobs.h
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-28T10:55:21.020Z
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 279
File: src/mongo/db/modules/eloq/CMakeLists.txt:54-55
Timestamp: 2025-11-28T10:55:21.020Z
Learning: In src/mongo/db/modules/eloq/CMakeLists.txt, the CMake options EXT_TX_PROC_ENABLED, FORK_HM_PROCESS, and STATISTICS are used by the data_substrate submodule build system and should be retained as configuration interface options.
Applied to files:
src/mongo/db/modules/eloq/src/eloq_index.cppsrc/mongo/db/query/query_knobs.cpp
📚 Learning: 2025-11-24T03:11:24.955Z
Learnt from: lzxddz
Repo: eloqdata/eloqdoc PR: 325
File: src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp:1362-1384
Timestamp: 2025-11-24T03:11:24.955Z
Learning: The EloqDoc codebase uses an older version of glog that supports the two-parameter signature `google::InitGoogleLogging(const char* argv0, PrefixCallback)` for custom log prefix formatting, as seen in src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp line 1383.
Applied to files:
src/mongo/db/modules/eloq/src/eloq_index.cpp
🔇 Additional comments (7)
src/mongo/db/query/query_knobs.cpp (1)
84-85: LGTM!The new server parameter follows the established pattern for other query knobs in this file. The default value of 100 is a reasonable starting point for batch prefetching.
src/mongo/db/query/query_knobs.h (1)
126-128: LGTM!The extern declaration is consistent with the implementation in
query_knobs.cppand follows the existing pattern for atomic server parameters.src/mongo/db/modules/eloq/src/eloq_cursor.h (1)
51-57: LGTM!The accessor methods are const-correct and efficiently expose the internal scan state needed for batch prefetching. Returning a const reference for
getCurrentBatchVector()avoids unnecessary copies.src/mongo/db/modules/eloq/src/eloq_index.cpp (4)
694-698: LGTM!The bounds checking (1 to 1000) is reasonable. It prevents zero batch sizes which would break the prefetching logic, and the upper limit prevents excessive memory consumption.
109-111: LGTM!Properly clearing prefetched records during cursor reset ensures stale data from previous scans doesn't persist.
27-27: LGTM!The include for
query_knobs.his necessary to accessinternalEloqIndexBatchFetchSizeand is appropriately placed.
318-321: LGTM!The lazy prefetching approach is well-placed - it only triggers after confirming a valid scan tuple exists, avoiding unnecessary fetches.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/mongo/db/modules/eloq/src/eloq_index.cpp (1)
852-885: Vector reallocation may invalidate pointers stored in indexBatchTuples.The
mongoRecordsvector does not reserve capacity before the loop at lines 861-885. At line 884, pointers tomongoRecordselements (&mongoRecords.back()) are stored inindexBatchTuples. IfmongoRecords.emplace_back()(line 880) triggers vector reallocation, these stored pointers become invalid, causing use-after-free whenbatchGetKVis called at line 891.Add defensive
reserve()calls to prevent reallocation:Suggested fix
// Use a set to track keys within this batch to detect duplicates within the batch BSONObjSet batchKeys = SimpleBSONObjComparator::kInstance.makeBSONObjSet(); + + // Reserve capacity to prevent reallocation during loop + mongoRecords.reserve(keys.size()); + indexBatchTuples.reserve(keys.size()); // For each key to be checked for (const BSONObj& key : keys) {
♻️ Duplicate comments (2)
src/mongo/db/modules/eloq/src/eloq_index.cpp (2)
361-367: Critical issue from previous review remains unaddressed.As flagged in the previous review, batch detection based solely on size comparison (
batchVector.size() != _lastBatchVectorSize) can miss batch transitions when consecutive batches have identical sizes, leading to stale prefetched records being returned.
404-419: Critical issue from previous review remains unaddressed.As flagged in the previous review, the RecordId extraction at lines 414-416 uses
KeyString::decodeRecordIdStrAtEnd()for all index types. However, for UNIQUE indexes, the RecordId is stored in the record blob (_scanTupleRecord->ToRecordId()as seen at line 535), not appended to the key. This will cause incorrect primary table lookups for UNIQUE indexes.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/mongo/db/modules/eloq/src/eloq_index.cpp
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-24T03:11:24.955Z
Learnt from: lzxddz
Repo: eloqdata/eloqdoc PR: 325
File: src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp:1362-1384
Timestamp: 2025-11-24T03:11:24.955Z
Learning: The EloqDoc codebase uses an older version of glog that supports the two-parameter signature `google::InitGoogleLogging(const char* argv0, PrefixCallback)` for custom log prefix formatting, as seen in src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp line 1383.
Applied to files:
src/mongo/db/modules/eloq/src/eloq_index.cpp
📚 Learning: 2025-11-28T10:55:21.020Z
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 279
File: src/mongo/db/modules/eloq/CMakeLists.txt:54-55
Timestamp: 2025-11-28T10:55:21.020Z
Learning: In src/mongo/db/modules/eloq/CMakeLists.txt, the CMake options EXT_TX_PROC_ENABLED, FORK_HM_PROCESS, and STATISTICS are used by the data_substrate submodule build system and should be retained as configuration interface options.
Applied to files:
src/mongo/db/modules/eloq/src/eloq_index.cpp
🧬 Code graph analysis (1)
src/mongo/db/modules/eloq/src/eloq_index.cpp (3)
src/mongo/db/modules/eloq/src/eloq_record_store.cpp (4)
id(128-130)id(128-128)id(485-517)id(485-485)src/mongo/db/modules/eloq/src/base/eloq_key.h (7)
TxKey(270-272)TxKey(284-310)TxKey(285-287)TxKey(289-291)TxKey(293-309)TxKey(294-296)TxKey(298-300)src/mongo/db/modules/eloq/src/base/eloq_util.cpp (2)
TxErrorCodeToMongoStatus(11-75)TxErrorCodeToMongoStatus(11-11)
🔇 Additional comments (6)
src/mongo/db/modules/eloq/src/eloq_index.cpp (6)
27-27: LGTM - Import needed for batch size configuration.The import is required to access
internalEloqIndexBatchFetchSizeused at line 700.
109-111: LGTM - Proper cleanup of prefetch state on reset.Correctly clears prefetched records when the cursor is reset to prevent using stale data.
319-326: LGTM - Lazy record fetching correctly placed.The call to
_ensureRecordsFetched()is appropriately placed after cursor advancement and correctly limited to STANDARD and UNIQUE indexes.
539-594: LGTM - Prefetched record lookup correctly implemented.The logic for looking up prefetched records in both UNIQUE and STANDARD index cases correctly:
- Calculates the offset from the prefetch range start
- Performs bounds checking
- Handles missing records with appropriate logging
687-703: LGTM - Private members and helpers well-designed.The prefetch storage uses
unique_ptrfor proper memory management, and_getBatchFetchSize()correctly bounds the configuration value to a reasonable range [1, 1000].
1052-1075: Minor formatting changes.The changes in this section are primarily formatting adjustments to the
setKVcall parameters, with no functional impact.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/mongo/db/modules/eloq/src/eloq_index.cpp (1)
405-420: RecordId extraction is incorrect for UNIQUE indexes.The code uses
KeyString::decodeRecordIdStrAtEnd()to extract RecordId from the key for all index types. However, for UNIQUE indexes, the RecordId is stored in the record's encoded blob (tuple.record_), not appended to the key.This will cause incorrect primary table lookups for UNIQUE indexes, as the extracted RecordId will be garbage data.
🔎 Suggested fix to handle index types differently
for (size_t i = startIdx; i < endIdx; ++i) { const auto& tuple = batchVector[i]; // Skip invalid entries - they will remain as nullptr holes in _prefetchedRecords if (tuple.status_ != txservice::RecordStatus::Normal) { continue; // Keep hole at this position } const Eloq::MongoKey* key = tuple.key_.GetKey<Eloq::MongoKey>(); if (key == nullptr) { continue; // Keep hole at this position } - KeyString ks(_idx->keyStringVersion()); - ks.resetFromBuffer(key->Data(), key->Size()); - RecordId id = KeyString::decodeRecordIdStrAtEnd(ks.getBuffer(), ks.getSize()); + RecordId id; + if (_indexType == IndexCursorType::STANDARD) { + // STANDARD: RecordId is appended to the key + KeyString ks(_idx->keyStringVersion()); + ks.resetFromBuffer(key->Data(), key->Size()); + id = KeyString::decodeRecordIdStrAtEnd(ks.getBuffer(), ks.getSize()); + } else { + // UNIQUE: RecordId is stored in the record's encoded blob + const Eloq::MongoRecord* record = static_cast<const Eloq::MongoRecord*>(tuple.record_); + if (record == nullptr) { + continue; + } + id = record->ToRecordId(false); + } recordIds.push_back(id); validIndices.push_back(i); // Track the batchVector index for this valid entry }
🧹 Nitpick comments (1)
src/mongo/db/modules/eloq/src/eloq_index.cpp (1)
541-596: Duplicate code for prefetched record lookup.The prefetched record lookup logic (lines 542-565 and 573-596) is identical for both UNIQUE and STANDARD index types. Consider extracting this into a helper method to improve maintainability.
🔎 Suggested refactor to eliminate duplication
// Add as a private helper method const Eloq::MongoRecord* _lookupPrefetchedRecord() { if (!_cursor) { return nullptr; } size_t scanBatchIdx = _cursor->getScanBatchIdx(); if (scanBatchIdx >= _prefetchedBatchStartIdx) { size_t offset = scanBatchIdx - _prefetchedBatchStartIdx; if (offset < _prefetchedRecords.size() && _prefetchedRecords[offset] != nullptr) { return _prefetchedRecords[offset].get(); } MONGO_LOG(1) << "RecordId " << _id.toString() << " not found in prefetched records at offset " << offset << " (scan index " << scanBatchIdx << ") for index " << _indexName->StringView(); } else { MONGO_LOG(1) << "Current scan index " << scanBatchIdx << " is before prefetched range starting at " << _prefetchedBatchStartIdx; } return nullptr; }Then use it in both cases:
case IndexCursorType::UNIQUE: case IndexCursorType::STANDARD: // ... existing RecordId and TypeBits logic ... _recordPtr = _lookupPrefetchedRecord(); break;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.gitignoresrc/mongo/db/modules/eloq/src/eloq_cursor.hsrc/mongo/db/modules/eloq/src/eloq_index.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/mongo/db/modules/eloq/src/eloq_cursor.h
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-24T03:11:24.955Z
Learnt from: lzxddz
Repo: eloqdata/eloqdoc PR: 325
File: src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp:1362-1384
Timestamp: 2025-11-24T03:11:24.955Z
Learning: The EloqDoc codebase uses an older version of glog that supports the two-parameter signature `google::InitGoogleLogging(const char* argv0, PrefixCallback)` for custom log prefix formatting, as seen in src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp line 1383.
Applied to files:
src/mongo/db/modules/eloq/src/eloq_index.cpp
📚 Learning: 2025-11-28T10:55:21.020Z
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 279
File: src/mongo/db/modules/eloq/CMakeLists.txt:54-55
Timestamp: 2025-11-28T10:55:21.020Z
Learning: In src/mongo/db/modules/eloq/CMakeLists.txt, the CMake options EXT_TX_PROC_ENABLED, FORK_HM_PROCESS, and STATISTICS are used by the data_substrate submodule build system and should be retained as configuration interface options.
Applied to files:
src/mongo/db/modules/eloq/src/eloq_index.cpp
🔇 Additional comments (5)
src/mongo/db/modules/eloq/src/eloq_index.cpp (5)
109-111: LGTM!Properly clears the prefetched records state during cursor reset to prevent stale data from being used after repositioning.
318-326: LGTM!The lazy fetch trigger is correctly placed after the scan tuple is populated and appropriately restricted to STANDARD and UNIQUE indexes.
361-368: Batch detection using batch count is reliable.Using
_cursor->getScanBatchCnt()to detect new batches is more reliable than comparing sizes, as it correctly handles the case where consecutive batches happen to have the same number of elements.
688-699: LGTM!The prefetched records storage uses appropriate ownership semantics with
std::unique_ptr, and the helper method provides a clean way to reset state.
701-705: LGTM!The batch size is properly bounded to prevent misconfiguration from causing issues (too small or excessively large batches).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/mongo/db/modules/eloq/src/eloq_index.cpp (2)
556-610: Consider extracting duplicated prefetch lookup logic.The prefetch record lookup logic is duplicated between the UNIQUE (lines 556-579) and STANDARD (lines 587-610) cases. Both sections:
- Get scanBatchIdx from cursor
- Calculate offset from _prefetchedBatchStartIdx
- Perform bounds checking
- Set _recordPtr to prefetched record or nullptr
- Log when record not found
Consider extracting this into a helper method like
_lookupPrefetchedRecord()to reduce duplication and improve maintainability.🔎 Suggested refactor
Add a helper method:
const Eloq::MongoRecord* _lookupPrefetchedRecord() { if (!_cursor) { return nullptr; } size_t scanBatchIdx = _cursor->getScanBatchIdx(); if (scanBatchIdx >= _prefetchedBatchStartIdx) { size_t offset = scanBatchIdx - _prefetchedBatchStartIdx; if (offset < _prefetchedRecords.size() && _prefetchedRecords[offset] != nullptr) { return _prefetchedRecords[offset].get(); } else { MONGO_LOG(1) << "RecordId " << _id.toString() << " not found in prefetched records at offset " << offset << " (scan index " << scanBatchIdx << ") for index " << _indexName->StringView(); return nullptr; } } else { MONGO_LOG(1) << "Current scan index " << scanBatchIdx << " is before prefetched range starting at " << _prefetchedBatchStartIdx; return nullptr; } }Then use it in both cases:
case IndexCursorType::UNIQUE: { _id = _scanTupleRecord->ToRecordId(false); BufReader br{_scanTupleRecord->UnpackInfoData(), static_cast<unsigned int>(_scanTupleRecord->UnpackInfoSize())}; _typeBits.resetFromBuffer(&br); - - size_t scanBatchIdx = _cursor->getScanBatchIdx(); - - if (scanBatchIdx >= _prefetchedBatchStartIdx) { - size_t offset = scanBatchIdx - _prefetchedBatchStartIdx; - if (offset < _prefetchedRecords.size() && - _prefetchedRecords[offset] != nullptr) { - _recordPtr = _prefetchedRecords[offset].get(); - } else { - _recordPtr = nullptr; - MONGO_LOG(1) << "RecordId " << _id.toString() - << " not found in prefetched records at offset " << offset - << " (scan index " << scanBatchIdx << ") for index " - << _indexName->StringView(); - } - } else { - _recordPtr = nullptr; - MONGO_LOG(1) << "Current scan index " << scanBatchIdx - << " is before prefetched range starting at " - << _prefetchedBatchStartIdx; - } + _recordPtr = _lookupPrefetchedRecord(); } break;Apply similar change for STANDARD case.
715-719: Batch size bounds are reasonable.The helper correctly loads the batch size from the server knob and clamps it to [1, 1000]. These bounds prevent degenerate cases (size=0) and excessive memory usage.
Optionally, consider adding a comment explaining the rationale for the 1000 upper bound to help future maintainers understand the constraint.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.gitignoresrc/mongo/db/modules/eloq/src/eloq_index.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- .gitignore
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-28T10:55:21.020Z
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 279
File: src/mongo/db/modules/eloq/CMakeLists.txt:54-55
Timestamp: 2025-11-28T10:55:21.020Z
Learning: In src/mongo/db/modules/eloq/CMakeLists.txt, the CMake options EXT_TX_PROC_ENABLED, FORK_HM_PROCESS, and STATISTICS are used by the data_substrate submodule build system and should be retained as configuration interface options.
Applied to files:
src/mongo/db/modules/eloq/src/eloq_index.cpp
🔇 Additional comments (7)
src/mongo/db/modules/eloq/src/eloq_index.cpp (7)
27-27: LGTM: Include added for batch size knob.The query_knobs.h include is necessary to access
internalEloqIndexBatchFetchSizeused in the batch fetch size helper.
109-111: LGTM: Prefetched records properly cleared on reset.Clearing prefetched records during cursor reset prevents stale data from being used after repositioning.
319-326: LGTM: Lazy prefetch correctly triggered for secondary indexes.The call to
_ensureRecordsFetched()is properly placed after obtaining the scan tuple and correctly filtered to STANDARD/UNIQUE indexes only, as ID indexes already have records in the scan result.
345-382: LGTM: Batch detection and range checking correctly implemented.The use of
getScanBatchCnt()(line 362) to detect new batches is more reliable than size-based comparison, addressing the previous concern about missing batch transitions. The prefetched range check and lazy fetch trigger logic are sound.
405-434: LGTM: RecordId extraction correctly handles index type differences.The code properly distinguishes between UNIQUE indexes (RecordId stored in record data, lines 413-420) and STANDARD indexes (RecordId appended to key, lines 422-429). This addresses the previous concern about incorrect RecordId extraction.
498-506: Hard error on missing records still present despite previous review.A previous review comment (marked as addressed) suggested treating missing records as holes rather than failing hard, to handle concurrent deletions gracefully. However, the current code (lines 503-505) still throws an
InternalErrorwhen a record cannot be fetched, which could cause unnecessary query failures in concurrent workloads.If the decision was made to keep the hard error despite the previous suggestion, please confirm this is intentional. Otherwise, consider treating missing records as nullptr holes (continuing the loop instead of throwing) to improve resilience to concurrent modifications.
850-973: Duplicate key checking changes appear unrelated to prefetching.The changes to
_checkDuplicateKeysInternal(addingcurrentRecordIdparameter, lines 851-855) andcheckDuplicateKeysForUpdate(line 964-965) don't appear directly related to the batch prefetching feature described in the PR objectives. The signature changes and variable declarations (lines 868-893) seem consistent but are a separate concern.If these changes are intentional scope creep for this PR, please confirm. Otherwise, consider splitting these into a separate PR for clarity.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
src/mongo/db/modules/eloq/src/eloq_index.cpp (4)
345-354: Address past review feedback: redundant checks.Per previous review:
- Lines 348-350: The index type check is redundant since
_ensureRecordsFetched()is only called after the caller already checks_indexType == IndexCursorType::STANDARD || _indexType == IndexCursorType::UNIQUE(lines 323-324).- Lines 352-354: It's unclear when
_cursorwould be nullptr at this point, since_ensureRecordsFetched()is only called after_cursor->currentBatchTuple()succeeds.Consider removing these defensive checks if they are truly unreachable, or add comments explaining the edge cases they guard against.
405-438: Address past review feedback: remove unnecessary checks.Per previous review comments:
- Lines 408-410: "Scan won't return a non-normal record" - the
tuple.status_ != txservice::RecordStatus::Normalcheck is unnecessary- Lines 424-426 (STANDARD branch): "The key cannot be nullptr" - the null key check is unnecessary
- Line 436-438: "Remove the detection" - the empty recordIds check may be unnecessary if the above checks are removed
If the scan guarantees Normal status and non-null keys, these defensive checks add unnecessary overhead. However, if you prefer to keep them for defensive coding, consider adding a comment explaining they guard against potential future changes or edge cases.
508-514: Address past review feedback: move the record instead of copying.Per previous review: "Try to move the record."
Line 511 creates a copy of the record. Since
docRecordsis a local array that won't be used after this loop, you could potentially move ownership rather than copy.However, since
tuple.record_points intodocRecords[i]and we need the data to persist in_prefetchedRecords, moving may require restructuring how records are allocated. Consider whetherdocRecordscan be converted tounique_ptrownership that transfers directly to_prefetchedRecords.
458-464: Address past review feedback: avoid prefetch for upserts.Per previous review: "Please avoid prefetch base table for upserts, which might acquire write-intents."
Using
_opCtx->isUpsert()asisForWritefor the prefetch batchGetKV could acquire write-intents on all prefetched records, not just the one being updated. This may cause unnecessary lock contention and conflicts.Consider either:
- Skipping prefetch entirely when
_opCtx->isUpsert()is true, or- Using
isForWrite=falsefor the prefetch call since these are read-ahead operations🔎 Suggested fix - skip prefetch for upserts
Add an early return in
_ensureRecordsFetched():void _ensureRecordsFetched() { + // Skip prefetch for upserts to avoid acquiring unnecessary write-intents + if (_opCtx->isUpsert()) { + return; + } + // Only fetch for STANDARD and UNIQUE indexes (not ID)
🧹 Nitpick comments (2)
src/mongo/db/modules/eloq/src/eloq_cursor.h (1)
51-55: Consider usinginvariant()instead ofassert()for runtime validation.
assert()may be compiled out in release builds, leaving the caller vulnerable to undefined behavior if_scanBatchIdxis invalid. MongoDB'sinvariant()macro provides consistent runtime checking regardless of build type.🔎 Suggested fix
// Return the index of the current batch tuple in the batch vector. size_t getCurrentBatchTupleIdx() const { - assert(_scanBatchIdx != UINT64_MAX && _scanBatchIdx > 0); + invariant(_scanBatchIdx != UINT64_MAX && _scanBatchIdx > 0); return _scanBatchIdx - 1; }src/mongo/db/modules/eloq/src/eloq_index.cpp (1)
555-610: Consider extracting duplicated prefetch lookup logic.The prefetch record lookup code in UNIQUE (lines 560-579) and STANDARD (lines 591-610) branches is nearly identical. This duplication can be reduced by extracting a helper method.
🔎 Suggested refactor
// Add private helper method const Eloq::MongoRecord* _getPrefetchedRecord(size_t scanBatchIdx) const { if (scanBatchIdx >= _prefetchedBatchStartIdx) { size_t offset = scanBatchIdx - _prefetchedBatchStartIdx; if (offset < _prefetchedRecords.size() && _prefetchedRecords[offset] != nullptr) { return _prefetchedRecords[offset].get(); } MONGO_LOG(1) << "RecordId not found in prefetched records at offset " << offset << " (scan index " << scanBatchIdx << ") for index " << _indexName->StringView(); } else { MONGO_LOG(1) << "Current scan index " << scanBatchIdx << " is before prefetched range starting at " << _prefetchedBatchStartIdx; } return nullptr; } // Then in both UNIQUE and STANDARD cases: size_t scanBatchIdx = _cursor->getCurrentBatchTupleIdx(); _recordPtr = _getPrefetchedRecord(scanBatchIdx);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/mongo/db/modules/eloq/src/eloq_cursor.hsrc/mongo/db/modules/eloq/src/eloq_index.cpp
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-28T10:55:21.020Z
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 279
File: src/mongo/db/modules/eloq/CMakeLists.txt:54-55
Timestamp: 2025-11-28T10:55:21.020Z
Learning: In src/mongo/db/modules/eloq/CMakeLists.txt, the CMake options EXT_TX_PROC_ENABLED, FORK_HM_PROCESS, and STATISTICS are used by the data_substrate submodule build system and should be retained as configuration interface options.
Applied to files:
src/mongo/db/modules/eloq/src/eloq_index.cpp
📚 Learning: 2025-11-24T03:11:24.955Z
Learnt from: lzxddz
Repo: eloqdata/eloqdoc PR: 325
File: src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp:1362-1384
Timestamp: 2025-11-24T03:11:24.955Z
Learning: The EloqDoc codebase uses an older version of glog that supports the two-parameter signature `google::InitGoogleLogging(const char* argv0, PrefixCallback)` for custom log prefix formatting, as seen in src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp line 1383.
Applied to files:
src/mongo/db/modules/eloq/src/eloq_index.cpp
🧬 Code graph analysis (1)
src/mongo/db/modules/eloq/src/eloq_index.cpp (1)
src/mongo/db/modules/eloq/src/base/eloq_util.cpp (2)
TxErrorCodeToMongoStatus(11-75)TxErrorCodeToMongoStatus(11-11)
🔇 Additional comments (5)
src/mongo/db/modules/eloq/src/eloq_cursor.h (1)
57-63: LGTM!These simple const accessors correctly expose the internal batch state without modification, enabling the lazy-prefetch logic in
EloqIndexCursor.src/mongo/db/modules/eloq/src/eloq_index.cpp (4)
109-112: LGTM!Correctly clears prefetched records on cursor reset to prevent stale state.
318-327: LGTM!The lazy fetch is correctly triggered after obtaining the scan tuple, and the index type guard is appropriate at this call site.
361-368: Good fix for batch transition detection.Using
getScanBatchCnt()is a reliable approach since the batch count is monotonically increasing, properly addressing the previous concern about detecting batch transitions when sizes are equal.
702-719: LGTM!The member variables and helper methods are well-designed:
unique_ptrensures proper ownership and cleanup of prefetched records_clearPrefetchedRecords()provides consistent state reset_getBatchFetchSize()properly clamps values to a reasonable range (1-1000)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/mongo/db/modules/eloq/src/eloq_index.cpp (1)
453-453: Skip prefetching for upsert operations to avoid write-intent contention.Prefetching the base table for upserts can acquire write-intents on records, potentially causing lock contention or deadlocks. The prefetch optimization should be disabled when
_opCtx->isUpsert()is true.🔎 Proposed fix to skip prefetch for upserts
Add an early return in
_ensureRecordsFetched():void _ensureRecordsFetched() { assert(_cursor); + + // Skip prefetch for upsert operations to avoid acquiring write-intents + if (_opCtx->isUpsert()) { + return; + } const auto& batchVector = _cursor->getCurrentBatchVector();And ensure
_updateRecordPtr()handles the non-prefetched case for STANDARD/UNIQUE:case IndexCursorType::UNIQUE: case IndexCursorType::STANDARD: { + // If prefetch was skipped (e.g., for upserts), fall back to scan record + if (_prefetchedRecords.empty()) { + _recordPtr = _scanTupleRecord; + break; + } + // Look up prefetched record by indexBased on past review comments from xiexiaoy.
🧹 Nitpick comments (1)
src/mongo/db/modules/eloq/src/eloq_index.cpp (1)
503-503: Consider moving the record instead of copying.The code currently copies the
MongoRecordobject, which may be inefficient for large records. If the source record fromfetchTuples[i]is not reused after this point, consider moving it instead.🔎 Proposed change to use move semantics
// Store the verified record Eloq::MongoRecord* record = static_cast<Eloq::MongoRecord*>(tuple.record_); // Make a copy and store at the correct offset - auto recordCopy = std::make_unique<Eloq::MongoRecord>(*record); + auto recordCopy = std::make_unique<Eloq::MongoRecord>(std::move(*record)); size_t offset = batchVectorIdx - startIdx;Note: Verify that the source record in
fetchTuplesis not accessed after this point.Based on past review comments from xiexiaoy.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/mongo/db/modules/eloq/src/eloq_index.cpp
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-28T10:55:21.020Z
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 279
File: src/mongo/db/modules/eloq/CMakeLists.txt:54-55
Timestamp: 2025-11-28T10:55:21.020Z
Learning: In src/mongo/db/modules/eloq/CMakeLists.txt, the CMake options EXT_TX_PROC_ENABLED, FORK_HM_PROCESS, and STATISTICS are used by the data_substrate submodule build system and should be retained as configuration interface options.
Applied to files:
src/mongo/db/modules/eloq/src/eloq_index.cpp
🔇 Additional comments (5)
src/mongo/db/modules/eloq/src/eloq_index.cpp (5)
27-27: LGTM!The include for query knobs and the prefetch state cleanup on reset are both correct and necessary.
Also applies to: 110-111
318-329: LGTM!The lazy fetch trigger correctly limits prefetching to STANDARD and UNIQUE indexes, where it's needed, and maintains proper sequencing of fetch-then-update operations.
348-376: Verify the necessity of assertions at lines 349 and 352.Based on past review comments, there are questions about when
_cursorwould be nullptr and whetherbatchVectorcould be empty at these points. If these conditions are guaranteed by the calling context (which appears to be the case since_updatePosition()checks_scanTupleKeybefore this), consider documenting the invariants or removing the asserts.The batch detection logic using
getScanBatchCnt()is good and addresses the previous concern about size-based detection.Based on past review comments from xiexiaoy.
524-560: LGTM!The
_updateRecordPtr()method correctly handles all three index types with appropriate fallback logic and error logging for edge cases.
675-691: LGTM!The private members and helper methods are well-designed with appropriate bounds checking for the batch fetch size and clear ownership semantics using
unique_ptr.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/mongo/db/modules/eloq/src/eloq_index.cpp (3)
397-426: Simplify by removing redundant defensive checks.Per past review feedback from xiexiaoy:
- "Scan won't return a non-normal record" (line 401)
- "The key cannot be nullptr" (line 418)
The checks at lines 400-402, 409-411, and 416-418 add unnecessary complexity since the scan guarantees valid records and keys. Consider removing them per the suggestions in the past coderabbitai review.
498-504: Consider moving the record instead of copying.Per past review feedback from xiexiaoy,
docRecords[i]is locally owned and can be moved instead of copied to avoid unnecessary allocations:🔎 Suggested change
// Store the verified record - Eloq::MongoRecord* record = static_cast<Eloq::MongoRecord*>(tuple.record_); - // Make a copy and store at the correct offset - auto recordCopy = std::make_unique<Eloq::MongoRecord>(*record); + // Move the record to avoid unnecessary copy + auto recordCopy = std::make_unique<Eloq::MongoRecord>(std::move(docRecords[i])); size_t offset = batchVectorIdx - startIdx;
448-454: Avoid prefetching base table records for upsert operations.Per past review feedback from xiexiaoy, prefetching with
isForWrite=trueduring upserts might acquire write-intents, which could cause lock contention issues. Consider either:
- Skipping the batch prefetch entirely for upserts, or
- Always using
isForWrite=falsefor the prefetch operation🔎 Suggested approach - skip prefetch for upserts
Add an early check in
_ensureRecordsFetched():void _ensureRecordsFetched() { assert(_cursor); + + // Skip prefetch for upserts to avoid acquiring write-intents + if (_opCtx->isUpsert()) { + return; + } const auto& batchVector = _cursor->getCurrentBatchVector();
🧹 Nitpick comments (2)
src/mongo/db/modules/eloq/src/eloq_index.cpp (2)
348-352: Consider defensive handling for potential empty batch vector.The
assert(!batchVector.empty())at line 352 could crash in edge cases. While_ensureRecordsFetched()is only called whenscanTuple != nullptr, there may be timing windows or edge cases where the batch vector could be empty.Based on past review comments from xiexiaoy asking "Might batchVector be empty here?", consider returning early instead of asserting:
🔎 Suggested defensive handling
void _ensureRecordsFetched() { assert(_cursor); const auto& batchVector = _cursor->getCurrentBatchVector(); - assert(!batchVector.empty()); + if (batchVector.empty()) { + return; // Nothing to prefetch + }
428-428: Replace assert with early return for robustness.If the defensive checks above are retained, this assert could crash when all entries are filtered. Even if removed, an early return is more defensive than crashing:
🔎 Suggested change
- assert(!recordIds.empty()); + if (recordIds.empty()) { + // Nothing to fetch - should not happen if scan guarantees valid entries + return; + }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/mongo/db/modules/eloq/data_substratesrc/mongo/db/modules/eloq/src/eloq_index.cpp
✅ Files skipped from review due to trivial changes (1)
- src/mongo/db/modules/eloq/data_substrate
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-28T10:55:21.020Z
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 279
File: src/mongo/db/modules/eloq/CMakeLists.txt:54-55
Timestamp: 2025-11-28T10:55:21.020Z
Learning: In src/mongo/db/modules/eloq/CMakeLists.txt, the CMake options EXT_TX_PROC_ENABLED, FORK_HM_PROCESS, and STATISTICS are used by the data_substrate submodule build system and should be retained as configuration interface options.
Applied to files:
src/mongo/db/modules/eloq/src/eloq_index.cpp
🔇 Additional comments (4)
src/mongo/db/modules/eloq/src/eloq_index.cpp (4)
109-111: LGTM!Proper cleanup of prefetched state on cursor reset ensures no stale data persists across cursor reuse.
318-329: LGTM!The index type check before
_ensureRecordsFetched()is intentional since ID indexes already have complete records in the scan result, while STANDARD/UNIQUE indexes require a separate lookup from the primary table.
522-558: LGTM!
_updateRecordPtr()correctly handles edge cases where prefetched records may not be available, falling back tonullptrwith appropriate logging. The downstream_curr()method properly handles null_recordPtr.
672-689: LGTM!The prefetch state management is well-structured:
- Member variables properly track batch alignment
_clearPrefetchedRecords()ensures complete state reset_getBatchFetchSize()applies sensible bounds (1-1000) to the configuration knob
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/mongo/db/modules/eloq/src/eloq_index.cpp (1)
440-443: Critical: Avoid prefetching base table records during upsert operations.Setting
isForWrite = _opCtx->isUpsert()and passing it tobatchGetKVcan acquire write-intents on the base table, which may cause contention or deadlocks during write operations.Prefetching should be skipped entirely when
_opCtx->isUpsert()is true. Add an early return in_ensureRecordsFetched()or at the start of this method to skip prefetching for write operations.Based on past review comments from xiexiaoy.
🔎 Proposed fix to skip prefetch for upserts
Add this check at the beginning of
_ensureRecordsFetched():void _ensureRecordsFetched() { assert(_cursor); + + // Skip prefetch for write operations to avoid acquiring write-intents + if (_opCtx->isUpsert()) { + return; + } const auto& batchVector = _cursor->getCurrentBatchVector();Then remove the isForWrite parameter from batchGetKV:
txservice::TxErrorCode err = - _ru->batchGetKV(_opCtx, tableName, schemaVersion, fetchTuples, isForWrite); + _ru->batchGetKV(_opCtx, tableName, schemaVersion, fetchTuples, false);
🧹 Nitpick comments (2)
src/mongo/db/modules/eloq/src/eloq_index.cpp (2)
348-352: Consider handling edge cases gracefully instead of asserting.The assertions at lines 349 and 352 could cause crashes if
_cursoris null orbatchVectoris empty. Based on past review comments, these conditions may be unexpected but defensive checks would be safer.Consider either documenting why these conditions are guaranteed, or handling them gracefully with early returns or error status.
487-490: Optimize record storage by moving instead of copying.Line 489 creates a copy of the fetched record with
std::make_unique<Eloq::MongoRecord>(*record). SincefetchTuplescontains the fetched records and won't be reused after this method, you can move the record instead of copying it to improve performance.Based on past review comments from xiexiaoy.
🔎 Proposed fix to move record
// Store the verified record Eloq::MongoRecord* record = static_cast<Eloq::MongoRecord*>(tuple.record_); // Make a copy and store at the correct offset - auto recordCopy = std::make_unique<Eloq::MongoRecord>(*record); + auto recordCopy = std::make_unique<Eloq::MongoRecord>(std::move(*record)); _prefetchedRecords[i] = std::move(recordCopy);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/mongo/db/modules/eloq/src/eloq_index.cpp
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-28T10:55:21.020Z
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 279
File: src/mongo/db/modules/eloq/CMakeLists.txt:54-55
Timestamp: 2025-11-28T10:55:21.020Z
Learning: In src/mongo/db/modules/eloq/CMakeLists.txt, the CMake options EXT_TX_PROC_ENABLED, FORK_HM_PROCESS, and STATISTICS are used by the data_substrate submodule build system and should be retained as configuration interface options.
Applied to files:
src/mongo/db/modules/eloq/src/eloq_index.cpp
🧬 Code graph analysis (1)
src/mongo/db/modules/eloq/src/eloq_index.cpp (2)
src/mongo/db/modules/eloq/src/base/eloq_key.h (7)
TxKey(270-272)TxKey(284-310)TxKey(285-287)TxKey(289-291)TxKey(293-309)TxKey(294-296)TxKey(298-300)src/mongo/db/modules/eloq/src/base/eloq_util.cpp (2)
TxErrorCodeToMongoStatus(11-75)TxErrorCodeToMongoStatus(11-11)
🔇 Additional comments (4)
src/mongo/db/modules/eloq/src/eloq_index.cpp (4)
476-484: Reconsider error handling for missing records.The code throws an InternalError if a record is not found during batch fetch. However, in concurrent workloads, records may be deleted between the index scan and the primary table lookup. This could cause unnecessary query failures.
While the current implementation ensures consistency, consider whether treating missing records as holes (skipping them) would be more appropriate for your use case. This would require careful consideration of isolation guarantees.
Based on past review comments.
Do you want to maintain strict error reporting for missing records, or should the scan handle concurrent deletions more gracefully?
500-536: LGTM!The
_updateRecordPtr()method correctly handles pointer alignment for all three index types. The bounds checking and error logging for UNIQUE/STANDARD indexes is appropriate.
651-667: LGTM!The member variables and helper methods are well-designed:
unique_ptrprovides automatic memory management_getBatchFetchSize()includes sensible bounds checking (1-1000)_clearPrefetchedRecords()properly resets all state
27-27: LGTM!The integration of the prefetching mechanism is well-placed:
- Include of
query_knobs.hprovides access to the batch size configuration- Clearing prefetched records in
reset()ensures clean state on cursor reuse- Lazy fetch in
_updatePosition()only for STANDARD/UNIQUE indexes is the correct approachAlso applies to: 109-111, 319-329
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/mongo/db/modules/eloq/src/eloq_index.cpp (2)
348-376: Consider removing defensive asserts.The asserts at lines 349 and 352 may be unnecessary given that
_ensureRecordsFetched()is only called from_updatePosition()after a successfulnextBatchTuple()call. At that point,_cursoris guaranteed to exist andbatchVectoris guaranteed to be non-empty. Removing these asserts would simplify the code without sacrificing safety.Based on past review comments from xiexiaoy.
🔎 Proposed simplification
void _ensureRecordsFetched() { - assert(_cursor); const auto& batchVector = _cursor->getCurrentBatchVector(); - assert(!batchVector.empty()); // Detect if a new batch has been fetched using batch count (more reliable than size)
394-417: Simplify RecordId extraction by removing unnecessary asserts.The asserts at lines 397, 403, and 408 are unnecessary because the scan guarantees that:
- Records will have Normal status
- Record pointers cannot be null for UNIQUE indexes
- Key pointers cannot be null for STANDARD indexes
Additionally, the assert at line 417 could theoretically fail if all entries are filtered out (though this shouldn't happen based on scan guarantees). Consider removing these defensive checks or replacing line 417's assert with a graceful early return.
Based on past review comments from xiexiaoy.
🔎 Proposed simplification
for (size_t i = startIdx; i < endIdx; ++i) { const auto& tuple = batchVector[i]; - // All entries in index scan results should be Normal and non-null - assert(tuple.status_ == txservice::RecordStatus::Normal); RecordId id; if (_indexType == IndexCursorType::UNIQUE) { // For UNIQUE indexes, RecordId is stored in the record data, not in the key const Eloq::MongoRecord* record = static_cast<const Eloq::MongoRecord*>(tuple.record_); - assert(record != nullptr); id = record->ToRecordId(false); } else { // For STANDARD indexes, RecordId is appended to the key const Eloq::MongoKey* key = tuple.key_.GetKey<Eloq::MongoKey>(); - assert(key != nullptr); KeyString ks(_idx->keyStringVersion()); ks.resetFromBuffer(key->Data(), key->Size()); id = KeyString::decodeRecordIdStrAtEnd(ks.getBuffer(), ks.getSize()); } recordIds.push_back(id); } - assert(!recordIds.empty()); + if (recordIds.empty()) { + return; // Nothing to fetch + }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/mongo/db/modules/eloq/src/eloq_index.cpp
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-28T10:55:21.020Z
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 279
File: src/mongo/db/modules/eloq/CMakeLists.txt:54-55
Timestamp: 2025-11-28T10:55:21.020Z
Learning: In src/mongo/db/modules/eloq/CMakeLists.txt, the CMake options EXT_TX_PROC_ENABLED, FORK_HM_PROCESS, and STATISTICS are used by the data_substrate submodule build system and should be retained as configuration interface options.
Applied to files:
src/mongo/db/modules/eloq/src/eloq_index.cpp
🧬 Code graph analysis (1)
src/mongo/db/modules/eloq/src/eloq_index.cpp (1)
src/mongo/db/modules/eloq/src/base/eloq_util.cpp (2)
TxErrorCodeToMongoStatus(11-75)TxErrorCodeToMongoStatus(11-11)
🔇 Additional comments (8)
src/mongo/db/modules/eloq/src/eloq_index.cpp (8)
27-27: LGTM!The new include is correctly added to support the batch fetch size configuration used in
_getBatchFetchSize().
110-111: LGTM!Properly clears prefetched records on cursor reset to ensure clean state.
319-329: LGTM!The index type check before calling
_ensureRecordsFetched()is appropriate because ID indexes already have records in the scan results and don't require prefetching. The separation keeps the logic clear.
478-486: Verify error handling strategy for missing records.The code treats records not found during batch fetch as hard errors (aborting with
InternalError). While the scan guarantees Normal records at scan time, concurrent deletions between the index scan and primary table lookup could cause this condition.Consider whether treating missing records as holes (similar to how ID indexes handle deleted entries) would provide better resilience in concurrent workloads, or confirm that hard failures are the intended behavior for data consistency guarantees.
498-534: LGTM!The
_updateRecordPtr()implementation correctly handles all three index types (ID, UNIQUE, STANDARD) with appropriate bounds checking and clear error logging for debugging.
649-665: LGTM!The new private members for prefetch tracking and helper methods are well-designed. The use of batch count (
_lastRecordsBatchCnt) for batch detection is more reliable than size-based detection, and the bounds checking in_getBatchFetchSize()ensures reasonable batch sizes.
797-872: LGTM!The refactoring of
_checkDuplicateKeysInternal()to use vectors for storing KeyString buffers and MongoKey objects ensures proper lifetime management and avoids potential dangling pointer issues during batch operations.
874-919: LGTM!The updates to
batchCheckDuplicateKey()andcheckDuplicateKeysForUpdate()are consistent with the internal helper method changes and maintain the correct duplicate detection logic.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.gitignore
🔇 Additional comments (2)
.gitignore (2)
189-189: ✅.cursorpattern correctly addressed.The previous review flagged that
.cursorshould use a leading/to restrict matching to the repository root. This has been properly corrected to/.cursor, aligning with root-level patterns like/buildand/mongoelsewhere in the file.
190-190: Other new patterns look good.The
/thoughtsand/dbpatterns follow the consistent convention of using leading slashes for root-level entries.Also applies to: 192-192
2f9d967 to
a3b9e4d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @src/mongo/db/modules/eloq/src/eloq_cursor.h:
- Around line 51-55: Callers of getCurrentBatchTupleIdx() must validate cursor
state first: before invoking getCurrentBatchTupleIdx(), check that
currentBatchTuple() returns non-null (or add and call a new predicate like
hasCurrentBatchTuple()/isCurrentBatchValid()) and only then call
getCurrentBatchTupleIdx(); alternatively implement an explicit state-validation
method on the cursor that checks _scanBatchIdx is neither UINT64_MAX nor zero
and use that from the places that currently call getCurrentBatchTupleIdx()
without checks.
♻️ Duplicate comments (1)
src/mongo/db/modules/eloq/src/eloq_index.cpp (1)
452-458: Skip prefetching for upsert operations to avoid unnecessary write-intents.Per previous review feedback from xiexiaoy, prefetching during upserts with
isForWrite=true(line 455) can acquire write-intents on records that may not be accessed. This could cause contention or unexpected locking behavior.Consider adding an early return in
_ensureRecordsFetched()to skip prefetching when_opCtx->isUpsert()is true.🔎 Suggested fix
Add at the beginning of
_ensureRecordsFetched():void _ensureRecordsFetched() { + // Skip prefetching for upserts to avoid acquiring unnecessary write-intents + if (_opCtx->isUpsert()) { + return; + } + assert(_cursor);
🧹 Nitpick comments (1)
src/mongo/db/modules/eloq/src/eloq_index.cpp (1)
395-418: Consider simplifying by removing redundant assertions.Per past review feedback, the scan guarantees that:
- Records have Normal status (line 398)
- Record pointers are non-null for UNIQUE indexes (line 404)
- Key pointers are non-null for STANDARD indexes (line 409)
These assertions document invariants but add cognitive overhead. The
assert(!recordIds.empty())at line 418 could be replaced with an early return for graceful handling if future changes introduce edge cases.🔎 Optional simplification
for (size_t i = startIdx; i < endIdx; ++i) { const auto& tuple = batchVector[i]; - // All entries in index scan results should be Normal and non-null - assert(tuple.status_ == txservice::RecordStatus::Normal); RecordId id; if (_indexType == IndexCursorType::UNIQUE) { // For UNIQUE indexes, RecordId is stored in the record data, not in the key const Eloq::MongoRecord* record = static_cast<const Eloq::MongoRecord*>(tuple.record_); - assert(record != nullptr); id = record->ToRecordId(false); } else { // For STANDARD indexes, RecordId is appended to the key const Eloq::MongoKey* key = tuple.key_.GetKey<Eloq::MongoKey>(); - assert(key != nullptr); KeyString ks(_idx->keyStringVersion()); ks.resetFromBuffer(key->Data(), key->Size()); id = KeyString::decodeRecordIdStrAtEnd(ks.getBuffer(), ks.getSize()); } recordIds.push_back(id); } - assert(!recordIds.empty()); + if (recordIds.empty()) { + return; // No records to fetch + }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.gitignoresrc/mongo/db/modules/eloq/src/eloq_cursor.hsrc/mongo/db/modules/eloq/src/eloq_index.cppsrc/mongo/db/query/query_knobs.cppsrc/mongo/db/query/query_knobs.h
🚧 Files skipped from review as they are similar to previous changes (1)
- .gitignore
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-28T10:55:21.020Z
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 279
File: src/mongo/db/modules/eloq/CMakeLists.txt:54-55
Timestamp: 2025-11-28T10:55:21.020Z
Learning: In src/mongo/db/modules/eloq/CMakeLists.txt, the CMake options EXT_TX_PROC_ENABLED, FORK_HM_PROCESS, and STATISTICS are used by the data_substrate submodule build system and should be retained as configuration interface options.
Applied to files:
src/mongo/db/query/query_knobs.hsrc/mongo/db/modules/eloq/src/eloq_index.cppsrc/mongo/db/query/query_knobs.cpp
🧬 Code graph analysis (2)
src/mongo/db/modules/eloq/src/eloq_cursor.h (1)
src/mongo/db/modules/eloq/src/eloq_index.cpp (1)
assert(349-377)
src/mongo/db/modules/eloq/src/eloq_index.cpp (2)
src/mongo/db/modules/eloq/src/base/eloq_key.h (7)
TxKey(270-272)TxKey(284-310)TxKey(285-287)TxKey(289-291)TxKey(293-309)TxKey(294-296)TxKey(298-300)src/mongo/db/modules/eloq/src/base/eloq_util.cpp (2)
TxErrorCodeToMongoStatus(11-75)TxErrorCodeToMongoStatus(11-11)
🔇 Additional comments (10)
src/mongo/db/query/query_knobs.h (1)
126-128: LGTM!The new server parameter declaration follows the existing conventions in the file and uses the appropriate atomic type for thread-safe runtime configuration.
src/mongo/db/query/query_knobs.cpp (1)
84-85: LGTM!The server parameter definition is consistent with the existing pattern and uses a reasonable default value of 100. The value is properly clamped in the consumer code.
src/mongo/db/modules/eloq/src/eloq_index.cpp (8)
27-27: LGTM!The include for
query_knobs.his necessary to access theinternalEloqIndexBatchFetchSizeparameter.
109-112: LGTM!Clearing prefetched records during reset ensures proper state management when the cursor is reused.
209-213: LGTM!Clearing prefetched records on cursor re-seek during restore is correct since the batch state will be invalidated.
319-347: LGTM!The lazy fetch logic is correctly integrated into the position update flow. ID indexes are properly excluded since they already have records in the scan result.
349-377: LGTM on batch detection logic.The batch detection using
getScanBatchCnt()(line 356) is more reliable than size comparison, addressing the earlier review feedback. The prefetch range check logic is correct.
478-491: Error handling approach is conservative but correct.The current implementation fails when a record referenced by the index scan cannot be found in the primary table (lines 484-486). This is safer than silently skipping missing records, as it surfaces potential data integrity issues.
For concurrent deletion scenarios, the index scan itself should not return entries for deleted records (assuming proper MVCC). If this error is observed frequently in production, consider revisiting whether graceful skipping is appropriate.
499-538: LGTM!The record pointer update logic correctly maps the current scan position to prefetched records. The ID index path properly uses the scan tuple record directly, while UNIQUE/STANDARD paths use the prefetched data. Edge cases are handled with appropriate logging.
652-669: LGTM!The new private members use
std::unique_ptrfor proper memory management. The batch size is appropriately clamped to a reasonable range (1-1000), and the helper methods provide clean abstractions for state management.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/mongo/db/modules/eloq/src/eloq_index.cpp (1)
454-457: Upsert operations may acquire unnecessary write-intents during prefetch.When
_opCtx->isUpsert()is true,batchGetKVis called withisForWrite=true, which may acquire write-intents on all prefetched records. This could cause contention for records that won't actually be modified.Consider either:
- Skipping the prefetch entirely for upsert operations, or
- Passing
isForWrite=falsehere since prefetching is for read optimization, not for acquiring write locks.Based on past review comments from xiexiaoy.
🔎 Suggested fix to skip prefetch for upserts
Add an early return at the beginning of
_ensureRecordsFetched():void _ensureRecordsFetched() { + // Skip prefetching for upserts to avoid acquiring unnecessary write-intents + if (_opCtx->isUpsert()) { + return; + } + assert(_cursor);
🧹 Nitpick comments (1)
src/mongo/db/modules/eloq/src/eloq_index.cpp (1)
492-492: Assert condition may be overly permissive.The condition
_prefetchedBatchStartIdx + _prefetchedRecords.size() <= endIdxshould be an equality check since:
_prefetchedBatchStartIdx = startIdx(line 427)_prefetchedRecords.size() = neededSize = endIdx - startIdx(lines 432-434)The sum should always equal
endIdx, not just be less than or equal.🔎 Suggested fix
- assert(_prefetchedBatchStartIdx + _prefetchedRecords.size() <= endIdx); + assert(_prefetchedBatchStartIdx + _prefetchedRecords.size() == endIdx);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/mongo/db/exec/text_or.cppsrc/mongo/db/modules/eloq/src/eloq_index.cpp
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-28T10:55:21.020Z
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 279
File: src/mongo/db/modules/eloq/CMakeLists.txt:54-55
Timestamp: 2025-11-28T10:55:21.020Z
Learning: In src/mongo/db/modules/eloq/CMakeLists.txt, the CMake options EXT_TX_PROC_ENABLED, FORK_HM_PROCESS, and STATISTICS are used by the data_substrate submodule build system and should be retained as configuration interface options.
Applied to files:
src/mongo/db/modules/eloq/src/eloq_index.cpp
🧬 Code graph analysis (1)
src/mongo/db/modules/eloq/src/eloq_index.cpp (2)
src/mongo/db/modules/eloq/src/base/eloq_key.h (7)
TxKey(270-272)TxKey(284-310)TxKey(285-287)TxKey(289-291)TxKey(293-309)TxKey(294-296)TxKey(298-300)src/mongo/db/modules/eloq/src/base/eloq_util.cpp (2)
TxErrorCodeToMongoStatus(11-75)TxErrorCodeToMongoStatus(11-11)
🔇 Additional comments (6)
src/mongo/db/modules/eloq/src/eloq_index.cpp (5)
109-111: LGTM - Proper lifecycle management.Clearing prefetched records on cursor reset ensures no stale data persists across cursor reuse.
318-326: LGTM - Correct separation of index types.The lazy fetch is correctly limited to STANDARD and UNIQUE indexes, as ID indexes already include records in the scan result.
348-376: Batch detection logic is correct.The use of
getScanBatchCnt()to detect new batches addresses the prior review concern about batch size comparison. The range-based check for determining if records are already fetched is sound.The
assert(!batchVector.empty())at line 352 is safe because_ensureRecordsFetched()is only called whenscanTuple != nullptr(lines 315-326), guaranteeing at least one tuple exists.
527-532: Verify if "before prefetched range" case is reachable.The condition
currentIndexScanBatchIdx < _prefetchedBatchStartIdx(line 527) suggests the cursor moved backward past the prefetched range. Given that_ensureRecordsFetched()should always fetch records starting from the current position, this case should theoretically never occur in normal operation.If this is defensive code for future changes, the current logging is appropriate. If it indicates a logic bug, consider asserting instead.
651-668: LGTM - Well-structured prefetch state management.The prefetch state variables are clearly documented, and the helper methods properly manage the lifecycle. The batch size bounds
[1, 1000]provide reasonable limits.src/mongo/db/exec/text_or.cpp (1)
47-47: LGTM: Minor code organization improvement.Consolidating the
using std::string;declaration is a good cleanup that eliminates duplication.
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @src/mongo/db/modules/eloq/src/eloq_index.cpp:
- Around line 398-421: Remove the redundant asserts that check tuple.status_ ==
txservice::RecordStatus::Normal and the non-nullness of Eloq::MongoRecord and
Eloq::MongoKey (these are guaranteed by the index scan), i.e., drop the
assertions around tuple.status_, record != nullptr and key != nullptr; then
replace the final assert(!recordIds.empty()) with graceful handling—check if
recordIds.empty() and perform an early return or return an empty result as
appropriate for the enclosing function instead of aborting. Ensure you keep
logic that populates recordIds for both IndexCursorType::UNIQUE and STANDARD
cases (use symbols _indexType, IndexCursorType::UNIQUE, Eloq::MongoRecord,
Eloq::MongoKey, recordIds).
🧹 Nitpick comments (2)
src/mongo/db/modules/eloq/src/eloq_index.cpp (2)
352-380: Consider whether defensive asserts are necessary.Lines 353 and 356 contain defensive asserts that may not be needed. Past review comments from xiexiaoy questioned when
_cursorwould be nullptr and whetherbatchVectorcould be empty at these points. If these conditions are guaranteed by the calling context, the asserts could be removed for clarity.However, the batch change detection at lines 359-365 correctly uses batch count (
_lastRecordsBatchCnt) instead of size, which reliably detects batch transitions and addresses previous review concerns.Based on past review comments from xiexiaoy.
483-491: Consider simplifying record validation.If the scan guarantees that all records in index results have
Normalstatus and are non-null (as indicated by past review comments), this validation check may be redundant. However, if you want to keep it as a defensive check for detecting internal errors, the current hard failure approach is reasonable.Based on past review comments from xiexiaoy.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/mongo/db/modules/eloq/src/eloq_index.cpp
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-28T10:55:21.020Z
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 279
File: src/mongo/db/modules/eloq/CMakeLists.txt:54-55
Timestamp: 2025-11-28T10:55:21.020Z
Learning: In src/mongo/db/modules/eloq/CMakeLists.txt, the CMake options EXT_TX_PROC_ENABLED, FORK_HM_PROCESS, and STATISTICS are used by the data_substrate submodule build system and should be retained as configuration interface options.
Applied to files:
src/mongo/db/modules/eloq/src/eloq_index.cpp
🧬 Code graph analysis (1)
src/mongo/db/modules/eloq/src/eloq_index.cpp (1)
src/mongo/db/modules/eloq/src/base/eloq_util.cpp (2)
TxErrorCodeToMongoStatus(11-75)TxErrorCodeToMongoStatus(11-11)
🔇 Additional comments (7)
src/mongo/db/modules/eloq/src/eloq_index.cpp (7)
27-27: LGTM!The include is necessary for accessing
internalEloqIndexBatchFetchSizeused in_getBatchFetchSize().
109-111: LGTM!Clearing prefetched records on reset ensures stale data isn't retained when the cursor is reinitialized.
298-298: LGTM!Clearing prefetched records when seeking ensures the cursor doesn't use stale prefetched data from the previous position.
318-330: LGTM!The logic correctly triggers lazy prefetching for STANDARD and UNIQUE indexes while skipping upserts to avoid acquiring unnecessary write-intents. This addresses past review feedback.
458-459: LGTM!The assertion at line 459 correctly verifies that upsert operations never reach the batch fetch path, ensuring write-intents are not acquired unnecessarily. This is a valuable invariant check.
503-545: LGTM!The
_updateRecordPtr()method correctly handles all three index types:
- ID indexes: Use the record directly from the scan tuple
- UNIQUE/STANDARD indexes: Look up prefetched records by offset, with proper bounds checking
The defensive check at line 513 ensures upsert operations never use prefetched records, maintaining consistency with the batch fetch logic.
659-676: LGTM!The prefetch state management is well-structured:
_prefetchedRecordsstores the fetched records_prefetchedBatchStartIdxtracks the range offset_lastRecordsBatchCntenables reliable batch change detection_getBatchFetchSize()applies sensible bounds [1, 1000] to the configurable batch size
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/mongo/db/modules/eloq/src/eloq_index.cpp (3)
398-421: Remove unnecessary defensive assertions.The assertions at lines 401, 407, 412, and 421 check conditions that are guaranteed by the index scan:
- Line 401:
tuple.status_is alwaysNormalfor index scan results- Line 407:
recordcannot be nullptr for UNIQUE indexes- Line 412:
keycannot be nullptr for STANDARD indexes- Line 421:
recordIds.empty()assertion could crash unnecessarilyThese defensive checks add complexity without value. Consider removing them or replacing the final assertion with graceful handling.
Based on past review comments from xiexiaoy and coderabbitai.
🔎 Proposed simplification
for (size_t i = startIdx; i < endIdx; ++i) { const auto& tuple = batchVector[i]; - // All entries in index scan results should be Normal and non-null - assert(tuple.status_ == txservice::RecordStatus::Normal); RecordId id; if (_indexType == IndexCursorType::UNIQUE) { // For UNIQUE indexes, RecordId is stored in the record data, not in the key const Eloq::MongoRecord* record = static_cast<const Eloq::MongoRecord*>(tuple.record_); - assert(record != nullptr); id = record->ToRecordId(false); } else { // For STANDARD indexes, RecordId is appended to the key const Eloq::MongoKey* key = tuple.key_.GetKey<Eloq::MongoKey>(); - assert(key != nullptr); KeyString ks(_idx->keyStringVersion()); ks.resetFromBuffer(key->Data(), key->Size()); id = KeyString::decodeRecordIdStrAtEnd(ks.getBuffer(), ks.getSize()); } recordIds.push_back(id); } - assert(!recordIds.empty()); + if (recordIds.empty()) { + return; // Nothing to fetch, early exit + }
458-459: Remove redundant upsert assertion.The assertion at line 459 checks
!isForWriteafter the upsert flag has already been evaluated. This is defensive coding after the fact—the caller at line 327 already ensures!_opCtx->isUpsert()before invoking_ensureRecordsFetched().Based on past review comments from xiexiaoy.
🔎 Proposed simplification
bool isForWrite = _opCtx->isUpsert(); - assert(!isForWrite);
483-491: Simplify record validation logic.The validation at lines 483-491 checks for non-Normal status or nullptr records and treats them as errors. However, based on past review comments, the scan guarantees that all returned records are Normal and non-null. The current error handling may be overly defensive.
Based on past review comments from xiexiaoy.
If the scan guarantees are absolute, consider simplifying this validation:
// Verify record is valid - fail if not - if (tuple.status_ != txservice::RecordStatus::Normal || tuple.record_ == nullptr) { - MONGO_LOG(1) << "Record at batch index " << batchVectorIdx - << " not found or has non-normal status: " - << static_cast<int>(tuple.status_); - // Return error if record cannot be found - uassertStatusOK(Status(ErrorCodes::InternalError, - "Failed to fetch record at batch index " + - std::to_string(batchVectorIdx))); - } + // Records are guaranteed to be Normal and non-null by scan + assert(tuple.status_ == txservice::RecordStatus::Normal); + assert(tuple.record_ != nullptr);Alternatively, if concurrent deletions are possible, handle missing records gracefully by treating them as holes (as suggested in earlier review rounds) instead of aborting the entire scan.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/mongo/db/modules/eloq/src/eloq_index.cpp
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-28T10:55:21.020Z
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 279
File: src/mongo/db/modules/eloq/CMakeLists.txt:54-55
Timestamp: 2025-11-28T10:55:21.020Z
Learning: In src/mongo/db/modules/eloq/CMakeLists.txt, the CMake options EXT_TX_PROC_ENABLED, FORK_HM_PROCESS, and STATISTICS are used by the data_substrate submodule build system and should be retained as configuration interface options.
Applied to files:
src/mongo/db/modules/eloq/src/eloq_index.cpp
📚 Learning: 2025-11-24T03:11:24.955Z
Learnt from: lzxddz
Repo: eloqdata/eloqdoc PR: 325
File: src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp:1362-1384
Timestamp: 2025-11-24T03:11:24.955Z
Learning: The EloqDoc codebase uses an older version of glog that supports the two-parameter signature `google::InitGoogleLogging(const char* argv0, PrefixCallback)` for custom log prefix formatting, as seen in src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp line 1383.
Applied to files:
src/mongo/db/modules/eloq/src/eloq_index.cpp
🧬 Code graph analysis (1)
src/mongo/db/modules/eloq/src/eloq_index.cpp (2)
src/mongo/db/modules/eloq/src/base/eloq_key.h (7)
TxKey(270-272)TxKey(284-310)TxKey(285-287)TxKey(289-291)TxKey(293-309)TxKey(294-296)TxKey(298-300)src/mongo/db/modules/eloq/src/base/eloq_util.cpp (2)
TxErrorCodeToMongoStatus(11-75)TxErrorCodeToMongoStatus(11-11)
🔇 Additional comments (10)
src/mongo/db/modules/eloq/src/eloq_index.cpp (10)
27-27: LGTM!The new include for
query_knobs.his necessary to access theinternalEloqIndexBatchFetchSizeconfiguration parameter used in_getBatchFetchSize().
109-111: LGTM!Clearing prefetched records during cursor reset ensures no stale data persists when the cursor is reused with new parameters.
298-298: LGTM!Clearing prefetched records when opening a new index scan ensures that prefetch state aligns with the new scan range.
318-330: LGTM!The conditional prefetch call correctly:
- Limits prefetching to STANDARD and UNIQUE indexes (secondary indexes)
- Skips prefetching for upsert operations to avoid unnecessary write-intent acquisition
347-350: LGTM!Calling
_updateRecordPtr()after updating ID and type bits ensures the record pointer is synchronized with the prefetched records for the current cursor position.
353-353: Clarify: Can_cursorbe nullptr here?The assert at line 353 assumes
_cursoris always valid when_ensureRecordsFetched()is called. Based on past review comments, this may be overly defensive. If_cursoris guaranteed to be non-null at this call site (invoked from_updatePosition()after a scan tuple is retrieved), consider removing this assert.Based on past review comments from xiexiaoy.
Can you confirm whether
_cursorcan be nullptr when_ensureRecordsFetched()is called from_updatePosition()? If not, this assert can be removed to simplify the code.
356-356: Clarify: CanbatchVectorbe empty here?The assert at line 356 assumes
batchVectoris never empty. However, ifgetCurrentBatchVector()can return an empty vector in edge cases (e.g., end of scan, error conditions), this assert will crash instead of handling the condition gracefully.Based on past review comments from xiexiaoy.
Can you confirm whether
getCurrentBatchVector()can return an empty vector? If so, replace the assert with an early return:const auto& batchVector = _cursor->getCurrentBatchVector(); - assert(!batchVector.empty()); + if (batchVector.empty()) { + return; // Nothing to prefetch + }
358-365: LGTM!Using batch count (
getScanBatchCnt()) to detect new batches is more reliable than size-based comparison, as it correctly identifies batch transitions even when consecutive batches have the same size.
503-548: LGTM!The
_updateRecordPtr()implementation correctly handles all three index types:
- ID indexes: Uses
_scanTupleRecorddirectly (no prefetch needed)- UNIQUE/STANDARD indexes: Retrieves prefetched records by offset, with proper bounds checking and null handling for upsert operations
The logic correctly aligns the record pointer with the current cursor position.
662-679: LGTM!The prefetch state management is well-structured:
_prefetchedRecords: Stores fetched records with appropriate ownership (unique_ptr)_prefetchedBatchStartIdx: Tracks the starting index for offset calculations_lastRecordsBatchCnt: Uses batch count for reliable batch transition detection- Helper methods provide clean encapsulation
The
_getBatchFetchSize()enforces reasonable bounds (1-1000) on the configured batch size.
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @tests/jstests/core/projection_dotted_paths.js:
- Around line 35-42: The test sets explain twice and the first assignment
(explain = coll.find({a: 1}, {_id: 0, "b.c": 1, c: 1}).explain("queryPlanner");)
is a stray/duplicate that gets overwritten; remove that first explain assignment
so the subsequent explain (with {_id:1, a:1, "b.c":1, "b.d":1, c:1}) is the one
used by the assertions (references: variable explain,
coll.find(...).explain("queryPlanner")).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/jstests/core/projection_dotted_paths.js
🔇 Additional comments (9)
tests/jstests/core/projection_dotted_paths.js (9)
10-17: LGTM! Test setup is correct.The test initialization properly loads the required library, creates the collection with appropriate index, and inserts test data.
19-25: LGTM! Covered index test is correct.The test properly verifies that projecting exactly the indexed fields (excluding
_id) results in a covered plan.
27-33: LGTM! Subset projection test is correct.The test properly verifies that projecting a subset of indexed fields also results in a covered plan.
44-49: LGTM! Non-indexed field test is correct.The test properly verifies that projecting a non-indexed field (that exists in the document) results in a non-covered plan.
51-57: LGTM! Non-existent field projection test is correct.The test properly verifies that projecting a non-indexed, non-existent field also results in a non-covered plan.
59-63: LGTM! IDHack query test is correct.The test properly verifies that projections work correctly with IDHack queries (queries by
_id).
85-92: LGTM! Multi-level nested projection test is correct.The test properly verifies that projections with multiple levels of nesting work correctly and result in covered plans when appropriate.
65-83: The MMAPv1 conditional is correct and should be retained.The codebase actively supports and tests MMAPv1 across 200+ references, including a dedicated test directory (
tests/jstests/mmap_v1/) and multiVersion upgrade/downgrade tests. The storage engine conditional at lines 79-83 is necessary and correctly implemented for the project's broader version support requirements. No changes needed.Likely an incorrect or invalid review comment.
94-97: SERVER-23229 cannot be verified through public sources—check internal MongoDB Jira.The reference to SERVER-23229 appears to be an internal MongoDB ticket (not publicly accessible on jira.mongodb.org). The commented test is appropriately blocked on this issue. To determine if the test should be enabled, the status of SERVER-23229 must be checked using internal MongoDB issue tracking. The existing comment sufficiently documents the rationale for why the test is disabled.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/mongo/db/modules/eloq/src/eloq_index.cpp (4)
352-380: Consider removing defensive assertions per past review feedback.Based on past review comments from xiexiaoy:
- Line 353: The
assert(_cursor)check may be unnecessary since_ensureRecordsFetched()is only called in_updatePosition()after confirmingscanTuple != nullptr, which implies_cursoris valid.- Line 356: The
assert(!batchVector.empty())check might be overly defensive. The scan guarantees should ensure this, but verify whether empty batches are possible in edge cases.If these conditions are guaranteed by the calling context and scan semantics, consider removing these assertions to simplify the code.
Based on past review comments from xiexiaoy.
401-429: Consider simplifying RecordId extraction per past review feedback.Based on past review comments from xiexiaoy, several defensive checks may be unnecessary:
- Line 404-406: The
Deletedstatus check. If the scan guarantees that onlyNormalrecords are returned, this check and the corresponding hole-handling logic could be simplified.- Line 409: The
assert(tuple.status_ == Normal)becomes redundant if the Deleted check is removed.- Line 415: The
assert(record != nullptr)for UNIQUE indexes may be unnecessary if the scan guarantees non-null records.- Line 420: The
assert(key != nullptr)for STANDARD indexes may be unnecessary if keys are guaranteed to be non-null.Consider confirming scan guarantees and removing these checks if they are indeed always satisfied.
Based on past review comments from xiexiaoy.
481-482: Redundant upsert check given upstream guards.Lines 481-482 check
isForWrite = _opCtx->isUpsert()and then assert!isForWrite. However, the calling code in_updatePosition()(lines 327, 533) already prevents_ensureRecordsFetched()from being called during upsert operations. This makes the check and assertion here redundant.Consider removing these lines since the upsert case is already filtered out before reaching this method.
Based on past review comments from xiexiaoy.
509-509: Consider removing defensive assertion per past review feedback.Line 509 asserts that
tuple.status_ == Normalandtuple.record_ != nullptr. Based on past review comments from xiexiaoy indicating that "Scan won't return a non-normal record" and successfulbatchGetKVcalls should guarantee valid records, this assertion may be unnecessary.If the scan semantics and
batchGetKVcontract guarantee these conditions, consider removing this assertion to simplify the code.Based on past review comments from xiexiaoy.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/mongo/db/modules/eloq/src/eloq_index.cpp
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-28T10:55:21.020Z
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 279
File: src/mongo/db/modules/eloq/CMakeLists.txt:54-55
Timestamp: 2025-11-28T10:55:21.020Z
Learning: In src/mongo/db/modules/eloq/CMakeLists.txt, the CMake options EXT_TX_PROC_ENABLED, FORK_HM_PROCESS, and STATISTICS are used by the data_substrate submodule build system and should be retained as configuration interface options.
Applied to files:
src/mongo/db/modules/eloq/src/eloq_index.cpp
📚 Learning: 2025-11-24T03:11:24.955Z
Learnt from: lzxddz
Repo: eloqdata/eloqdoc PR: 325
File: src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp:1362-1384
Timestamp: 2025-11-24T03:11:24.955Z
Learning: The EloqDoc codebase uses an older version of glog that supports the two-parameter signature `google::InitGoogleLogging(const char* argv0, PrefixCallback)` for custom log prefix formatting, as seen in src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp line 1383.
Applied to files:
src/mongo/db/modules/eloq/src/eloq_index.cpp
🧬 Code graph analysis (1)
src/mongo/db/modules/eloq/src/eloq_index.cpp (2)
src/mongo/db/modules/eloq/src/base/eloq_key.h (7)
TxKey(270-272)TxKey(284-310)TxKey(285-287)TxKey(289-291)TxKey(293-309)TxKey(294-296)TxKey(298-300)src/mongo/db/modules/eloq/src/base/eloq_util.cpp (2)
TxErrorCodeToMongoStatus(11-75)TxErrorCodeToMongoStatus(11-11)
🔇 Additional comments (6)
src/mongo/db/modules/eloq/src/eloq_index.cpp (6)
27-27: LGTM! Required import for batch fetch size configuration.The inclusion of
query_knobs.his necessary to accessinternalEloqIndexBatchFetchSizeused in_getBatchFetchSize().
109-111: LGTM! Proper prefetch state cleanup on cursor reset.Clearing prefetched records when the cursor is reset ensures no stale data is retained across cursor reuses.
298-298: LGTM! Correct prefetch state management on new scan.Clearing prefetched records after opening a new index scan prevents using stale data from the previous scan position.
318-350: LGTM! Well-integrated lazy prefetch logic.The lazy prefetch is triggered at the appropriate point after retrieving the scan tuple, with proper guards for index type and upsert operations. The
_updateRecordPtr()call is correctly positioned after_updateIdAndTypeBits().
523-568: LGTM! Proper record pointer management with prefetch integration.The implementation correctly:
- Handles different index types (ID vs UNIQUE/STANDARD)
- Uses prefetched records only for non-upsert operations
- Performs appropriate bounds checking with offset calculation
- Logs when records are not found in the prefetched range
The logic is sound and well-structured.
683-699: LGTM! Well-designed prefetch state management.The private members and helper methods are well-structured:
- Clear separation of prefetch state variables
_clearPrefetchedRecords()provides a single point for state cleanup_getBatchFetchSize()enforces reasonable bounds (1-1000) on the configuration knob, preventing misconfigurationThe implementation follows good encapsulation practices.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/mongo/db/modules/eloq/src/eloq_index.cpp (2)
483-484: Remove redundant assert—guaranteed by caller.The assert at line 484 verifies that
isForWriteis false (i.e., not an upsert). However, line 327 already ensures that_ensureRecordsFetched()is never called for upsert operations, making this assert redundant.🔎 Suggested simplification
- bool isForWrite = _opCtx->isUpsert(); - assert(!isForWrite); + bool isForWrite = false; // Never called for upserts (guarded at line 327)
507-507: Remove redundant assert—guaranteed by batchGetKV semantics.The assert at line 507 checks that fetched records have Normal status and are non-null. According to past review comments,
batchGetKVguarantees that all successfully fetched records meet these conditions, making this assert redundant.🔎 Suggested simplification
- // Verify record is valid - fail if not - assert(tuple.status_ == txservice::RecordStatus::Normal && tuple.record_ != nullptr); - // Records are already in _prefetchedRecords at the correct positions, no move neededBased on past review comments.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/mongo/db/modules/eloq/src/eloq_index.cpp
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 404
File: src/mongo/db/exec/text_or.cpp:261-261
Timestamp: 2026-01-06T06:34:10.846Z
Learning: In the context of batch index row lookup for secondary index scans (eloqdoc repository), WorkingSetMember objects returned for read-only index scans have state RID_AND_OBJ, which includes RID, IDX (keyData), and OBJ, not just RID and OBJ as the state name suggests.
📚 Learning: 2025-11-28T10:55:21.020Z
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 279
File: src/mongo/db/modules/eloq/CMakeLists.txt:54-55
Timestamp: 2025-11-28T10:55:21.020Z
Learning: In src/mongo/db/modules/eloq/CMakeLists.txt, the CMake options EXT_TX_PROC_ENABLED, FORK_HM_PROCESS, and STATISTICS are used by the data_substrate submodule build system and should be retained as configuration interface options.
Applied to files:
src/mongo/db/modules/eloq/src/eloq_index.cpp
🔇 Additional comments (7)
src/mongo/db/modules/eloq/src/eloq_index.cpp (7)
27-27: LGTM: Dependency added for batch size configuration.The inclusion of
query_knobs.henables access to the batch size configuration knob used in_getBatchFetchSize()at line 692.
109-111: LGTM: Proper lifecycle management.Clearing prefetched records during cursor reset ensures no stale data persists across cursor reuse.
298-298: LGTM: Clearing prefetch state on new scan.Clearing prefetched records after
indexScanOpen()is correct—a new scan invalidates any previously prefetched data.
318-350: LGTM: Lazy prefetch with correct upsert handling.The lazy prefetch at lines 323-330 correctly:
- Limits prefetching to STANDARD and UNIQUE indexes (ID indexes already have records)
- Skips prefetch for upsert operations to avoid unnecessary write-intent acquisition
- Updates the record pointer at lines 347-350 after ensuring records are fetched
This approach balances performance with correctness.
352-380: LGTM: Efficient prefetch range checking.The method correctly:
- Uses batch count to detect new batches (lines 359-364), which is reliable
- Checks if the current scan position is within the already-prefetched range (lines 372-376)
- Triggers fetch only when needed (line 379)
The asserts at lines 353 and 356 are reasonable for internal consistency checks.
519-564: LGTM: Correct record pointer management.The
_updateRecordPtr()method correctly handles all three index types:
- ID: Uses scan result directly (lines 523-525)
- UNIQUE/STANDARD: Uses prefetched records with proper bounds checking (lines 536-555)
- Upserts: Sets nullptr to avoid prefetch (lines 529, 557-559)
The bounds checking and logging at lines 536-554 provide good debugging support for out-of-range cases.
678-695: LGTM: Well-designed prefetch state management.The prefetch state members and helper methods are well-structured:
_prefetchedRecordsusesunique_ptrfor safe automatic memory management_lastRecordsBatchCntenables reliable batch transition detection_clearPrefetchedRecords()provides consistent cleanup_getBatchFetchSize()enforces reasonable bounds (1-1000) to prevent configuration errors
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/mongo/db/modules/eloq/src/eloq_index.cpp (3)
352-356: Defensive asserts are reasonable but consider the call context.The asserts at lines 353 and 356 are defensive checks. Given that
_ensureRecordsFetched()is only called after_cursor->nextBatchTuple()succeeds andscanTuple != nullptr, these conditions should always hold. The asserts serve as documentation of invariants rather than runtime guards.If you prefer fail-fast behavior in debug builds only, consider wrapping with
dassertinstead ofassertfor consistency with MongoDB conventions.
489-497: Unreachable code afteruassertStatusOK.Line 496 (
return;) is unreachable becauseuassertStatusOK()throws an exception when the status is not OK. This is dead code.🔎 Suggested fix
if (err != txservice::TxErrorCode::NO_ERROR) { MONGO_LOG(0) << "Batch fetch failed for table " << tableName.StringView() << ", range [" << startIdx << "-" << endIdx << "), error: " << err; // Clear prefetched records on error and return error _prefetchedRecords.clear(); _prefetchedBatchStartIdx = 0; uassertStatusOK(TxErrorCodeToMongoStatus(err)); - return; }
483-484: Consider simplifying:isForWriteis alwaysfalsehere.Since
_fetchRecordsForRange()is only called when!_opCtx->isUpsert()(line 327), the assignmentisForWrite = _opCtx->isUpsert()at line 483 will always evaluate tofalse. The assert at line 484 correctly documents this invariant.You could simplify by passing
falsedirectly tobatchGetKV:🔎 Suggested simplification
// Execute batch fetch const txservice::TableName& tableName = _idx->getTableName(); uint64_t schemaVersion = _ru->getIndexSchema(tableName)->SchemaTs(); - bool isForWrite = _opCtx->isUpsert(); - assert(!isForWrite); + // Note: This method is only called for read operations (not upserts) + // Upserts are filtered out in _updatePosition() to avoid acquiring write-intents + constexpr bool isForWrite = false; txservice::TxErrorCode err = _ru->batchGetKV(_opCtx, tableName, schemaVersion, fetchTuples, isForWrite);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/mongo/db/modules/eloq/src/eloq_index.cpp
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 404
File: src/mongo/db/exec/text_or.cpp:261-261
Timestamp: 2026-01-06T06:34:10.846Z
Learning: In the context of batch index row lookup for secondary index scans (eloqdoc repository), WorkingSetMember objects returned for read-only index scans have state RID_AND_OBJ, which includes RID, IDX (keyData), and OBJ, not just RID and OBJ as the state name suggests.
📚 Learning: 2025-11-28T10:55:21.020Z
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 279
File: src/mongo/db/modules/eloq/CMakeLists.txt:54-55
Timestamp: 2025-11-28T10:55:21.020Z
Learning: In src/mongo/db/modules/eloq/CMakeLists.txt, the CMake options EXT_TX_PROC_ENABLED, FORK_HM_PROCESS, and STATISTICS are used by the data_substrate submodule build system and should be retained as configuration interface options.
Applied to files:
src/mongo/db/modules/eloq/src/eloq_index.cpp
🔇 Additional comments (3)
src/mongo/db/modules/eloq/src/eloq_index.cpp (3)
401-429: Clarify whether Deleted records can appear in index scan results.Past review comments from xiexiaoy stated "Scan won't return a non-normal record." If this is true, the
Deletedstatus check (lines 404-406) and the subsequent assert at line 409 are both redundant.However, if Deleted records can appear (perhaps in edge cases during concurrent modifications), then keeping both makes sense.
Please clarify the expected behavior:
- If scans never return Deleted records, remove lines 404-406 and simplify the loop
- If Deleted records are possible, keep the skip logic but remove the assert at line 409 (since it's already guarded by the continue)
Based on past review comments from xiexiaoy.
518-563: LGTM!The
_updateRecordPtr()method correctly handles all index types:
- ID indexes use the scan tuple record directly
- UNIQUE/STANDARD indexes look up from prefetched records with proper bounds checking
- Upsert operations correctly skip prefetch lookup
- Missing records result in
nullptr, which_curr()handles gracefully by returning an IndexKeyEntry without record data
677-694: LGTM!The prefetch state management is well-designed:
_prefetchedRecordsusesunique_ptrfor proper ownership semantics_clearPrefetchedRecords()correctly resets all related state_getBatchFetchSize()clamps the configurable batch size to a safe range [1, 1000]The use of
_lastRecordsBatchCntto detect new batches (instead of comparing batch sizes) addresses the earlier review feedback about reliable batch transition detection.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @src/mongo/db/modules/eloq/src/eloq_index.cpp:
- Around line 402-430: Remove the redundant defensive checks in the loop:
eliminate the Deleted-status skip (tuple.status_ ==
txservice::RecordStatus::Deleted), the assert that tuple.status_ ==
txservice::RecordStatus::Normal, and the nullptr asserts for record and key;
assume the scan only yields Normal tuples and that for UNIQUE the
Eloq::MongoRecord* returned from tuple.record_ and for STANDARD the
Eloq::MongoKey* from tuple.key_.GetKey<Eloq::MongoKey>() are valid, then proceed
to compute RecordId (id = record->ToRecordId(false) for UNIQUE, and decode via
KeyString::decodeRecordIdStrAtEnd for STANDARD) and push i into
recordIdsIdxToBatchIdx and id into recordIds as before.
🧹 Nitpick comments (1)
src/mongo/db/modules/eloq/src/eloq_index.cpp (1)
354-357: Consider removing defensive assertions.Lines 354 and 357 contain assertions that check conditions guaranteed by the calling context:
- Line 354:
_cursorcannot be null when called from_updatePosition()after a successful scan- Line 357:
batchVectorcannot be empty since_updatePosition()only calls this whenscanTuple != nullptrBased on past review comments from xiexiaoy.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/mongo/db/modules/eloq/src/eloq_index.cpp
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 404
File: src/mongo/db/exec/text_or.cpp:261-261
Timestamp: 2026-01-06T06:34:10.846Z
Learning: In the context of batch index row lookup for secondary index scans (eloqdoc repository), WorkingSetMember objects returned for read-only index scans have state RID_AND_OBJ, which includes RID, IDX (keyData), and OBJ, not just RID and OBJ as the state name suggests.
📚 Learning: 2025-11-28T10:55:21.020Z
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 279
File: src/mongo/db/modules/eloq/CMakeLists.txt:54-55
Timestamp: 2025-11-28T10:55:21.020Z
Learning: In src/mongo/db/modules/eloq/CMakeLists.txt, the CMake options EXT_TX_PROC_ENABLED, FORK_HM_PROCESS, and STATISTICS are used by the data_substrate submodule build system and should be retained as configuration interface options.
Applied to files:
src/mongo/db/modules/eloq/src/eloq_index.cpp
📚 Learning: 2025-11-24T03:11:24.955Z
Learnt from: lzxddz
Repo: eloqdata/eloqdoc PR: 325
File: src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp:1362-1384
Timestamp: 2025-11-24T03:11:24.955Z
Learning: The EloqDoc codebase uses an older version of glog that supports the two-parameter signature `google::InitGoogleLogging(const char* argv0, PrefixCallback)` for custom log prefix formatting, as seen in src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp line 1383.
Applied to files:
src/mongo/db/modules/eloq/src/eloq_index.cpp
📚 Learning: 2025-09-25T12:09:01.276Z
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 211
File: src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp:95-115
Timestamp: 2025-09-25T12:09:01.276Z
Learning: The log states (LOG_STATE_TYPE_RKDB, LOG_STATE_TYPE_RKDB_S3, LOG_STATE_TYPE_RKDB_GCS) in the EloqDoc codebase are mutually exclusive - only one can be active at a time. The LOG_STATE_TYPE_RKDB_CLOUD macro is a helper that should only be defined for cloud variants (S3/GCS) and not when regular RKDB is active.
Applied to files:
src/mongo/db/modules/eloq/src/eloq_index.cpp
🔇 Additional comments (7)
src/mongo/db/modules/eloq/src/eloq_index.cpp (7)
20-20: LGTM!The new includes are necessary for the prefetch functionality:
<atomic>for thread-safe statistics tracking andquery_knobs.hfor the batch fetch size configuration knob.Also applies to: 28-28
110-112: LGTM!Properly clears prefetched state during cursor lifecycle transitions (reset and seek), preventing stale data from being used.
Also applies to: 299-299
319-351: LGTM!The integration of lazy prefetch into
_updatePosition()correctly:
- Triggers prefetch only for STANDARD/UNIQUE indexes (ID indexes already have records)
- Skips prefetch during upsert operations to avoid unnecessary locks
- Updates
_recordPtrafter position and type bits are updated
432-458: LGTM!The handling of empty
recordIds(when all records in the range are deleted) is correct—the code logs appropriately and returns early, skipping the unnecessarybatchGetKVcall.
488-514: Statistics tracking looks good.The use of atomic counters for thread-safe statistics collection is appropriate, and logging every 10,000 calls provides useful monitoring data without excessive overhead.
545-590: LGTM!The
_updateRecordPtr()implementation correctly:
- Uses scan results directly for ID indexes
- Looks up prefetched records for UNIQUE/STANDARD indexes with proper offset calculation and bounds checking
- Handles upserts by setting
_recordPtrtonullptr(no prefetch performed)- Provides clear logging for debugging
717-721: No action required. The configuration knobinternalEloqIndexBatchFetchSizeis properly declared inquery_knobs.h(line 128), exported inquery_knobs.cpp(line 85) with a default value of 100, and includes documentation. The code correctly uses.load()to access the atomic value, and the clamping logic [1, 1000] is sound.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/mongo/db/modules/eloq/src/eloq_cursor.cpp (1)
195-228: Consider log level and note inherent approximation in statistics.Two observations:
Race between increment and exchange: The counter increment (line 213) and accumulator updates (lines 214-215) are separate atomic operations. When thread A enters the
ifblock and exchanges, thread B may be mid-update. This means the window statistics are approximate, which is generally acceptable for instrumentation but worth documenting.Log level 0 may be too verbose:
MONGO_LOG(0)is typically always enabled. Logging every 10,000 calls could generate significant noise in production. Consider a higher log level (e.g.,MONGO_LOG(1)orMONGO_LOG(2)) unless this is specifically for production monitoring.Minor: The comment on line 219 mentions
memory_order_acquirebut the code usesmemory_order_acq_rel.🔎 Suggested adjustments
- // Log every 10000 calls with statistics from recent window + // Log every 10000 calls with approximate statistics from recent window + // Note: statistics are approximate due to non-atomic window boundaries if (count % WINDOW_SIZE == 0) { - // Use memory_order_acquire to ensure we see all updates before reading + // Use memory_order_acq_rel to synchronize with prior relaxed stores uint64_t windowTotalLatencyUs = totalLatencyUs.exchange(0, std::memory_order_acq_rel); uint64_t windowTotalFetchTuplesSize = totalFetchTuplesSize.exchange(0, std::memory_order_acq_rel); uint64_t avgLatencyUs = windowTotalLatencyUs / WINDOW_SIZE; uint64_t avgFetchTuplesSize = windowTotalFetchTuplesSize / WINDOW_SIZE; - MONGO_LOG(0) << "scanBatchTxReq statistics (recent " << WINDOW_SIZE << " calls): " + MONGO_LOG(1) << "scanBatchTxReq statistics (recent " << WINDOW_SIZE << " calls): " << "average latency: " << avgLatencyUs << " us, " << "average fetchTuples size: " << avgFetchTuplesSize; }src/mongo/db/modules/eloq/src/eloq_recovery_unit.cpp (1)
455-500: Lock-free instrumentation looks good for monitoring purposes.The implementation correctly uses atomic operations with appropriate memory ordering to track batch operation statistics without locks:
- Relaxed ordering for accumulation (lines 485-487) is suitable since no inter-thread synchronization is needed for stats
- Acq_rel ordering for the exchange (lines 492-493) properly ensures visibility when resetting the window
The window boundary race (where concurrent threads may add data between the modulo check and the exchange) means logged statistics are approximate rather than exact, but this is acceptable and typical for lock-free monitoring.
Minor comment inaccuracy
Line 491's comment mentions
memory_order_acquire, but the code correctly usesmemory_order_acq_relon lines 492-493. Consider updating the comment for accuracy:- // Use memory_order_acquire to ensure we see all updates before reading + // Use memory_order_acq_rel to ensure visibility of all updates when resetting uint64_t windowTotalLatencyUs = totalLatencyUs.exchange(0, std::memory_order_acq_rel);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/mongo/db/modules/eloq/src/eloq_cursor.cppsrc/mongo/db/modules/eloq/src/eloq_recovery_unit.cpp
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 404
File: src/mongo/db/exec/text_or.cpp:261-261
Timestamp: 2026-01-06T06:34:10.846Z
Learning: In the context of batch index row lookup for secondary index scans (eloqdoc repository), WorkingSetMember objects returned for read-only index scans have state RID_AND_OBJ, which includes RID, IDX (keyData), and OBJ, not just RID and OBJ as the state name suggests.
📚 Learning: 2025-11-28T10:55:21.020Z
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 279
File: src/mongo/db/modules/eloq/CMakeLists.txt:54-55
Timestamp: 2025-11-28T10:55:21.020Z
Learning: In src/mongo/db/modules/eloq/CMakeLists.txt, the CMake options EXT_TX_PROC_ENABLED, FORK_HM_PROCESS, and STATISTICS are used by the data_substrate submodule build system and should be retained as configuration interface options.
Applied to files:
src/mongo/db/modules/eloq/src/eloq_cursor.cppsrc/mongo/db/modules/eloq/src/eloq_recovery_unit.cpp
📚 Learning: 2025-11-24T03:11:24.955Z
Learnt from: lzxddz
Repo: eloqdata/eloqdoc PR: 325
File: src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp:1362-1384
Timestamp: 2025-11-24T03:11:24.955Z
Learning: The EloqDoc codebase uses an older version of glog that supports the two-parameter signature `google::InitGoogleLogging(const char* argv0, PrefixCallback)` for custom log prefix formatting, as seen in src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp line 1383.
Applied to files:
src/mongo/db/modules/eloq/src/eloq_cursor.cpp
📚 Learning: 2025-09-25T12:09:01.276Z
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 211
File: src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp:95-115
Timestamp: 2025-09-25T12:09:01.276Z
Learning: The log states (LOG_STATE_TYPE_RKDB, LOG_STATE_TYPE_RKDB_S3, LOG_STATE_TYPE_RKDB_GCS) in the EloqDoc codebase are mutually exclusive - only one can be active at a time. The LOG_STATE_TYPE_RKDB_CLOUD macro is a helper that should only be defined for cloud variants (S3/GCS) and not when regular RKDB is active.
Applied to files:
src/mongo/db/modules/eloq/src/eloq_cursor.cpp
🔇 Additional comments (2)
src/mongo/db/modules/eloq/src/eloq_cursor.cpp (2)
20-21: LGTM!The includes for
<atomic>and<butil/time.h>are appropriate for the lock-free instrumentation and high-resolution timing added below.Also applies to: 30-31
202-210: Timer placement is correct; consider whether to track error cases separately.The timer correctly captures the full execution time including the
Wait(). Note that statistics currently include both successful and failed calls. If you want to distinguish error latency from success latency, you could conditionally update the accumulators after checkingscanBatchTxReq.IsError(). Current behavior is reasonable for overall throughput monitoring.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/mongo/db/modules/eloq/src/eloq_index.cpp (2)
374-377: Consider graceful handling instead of assertions.The assertions at lines 374 and 377 assume
_cursoris always valid andbatchVectoris never empty when_ensureRecordsFetched()is called. However, assertions cause program termination in release builds if violated. If these conditions can occur during normal operation (e.g., edge cases, concurrent modifications), consider returning early or handling the cases gracefully instead.🔎 Suggested graceful handling
void _ensureRecordsFetched() { - assert(_cursor); + if (!_cursor) { + return; // No cursor, nothing to fetch + } const auto& batchVector = _cursor->getCurrentBatchVector(); - assert(!batchVector.empty()); + if (batchVector.empty()) { + return; // Empty batch, nothing to fetch + }
504-505: Redundant upsert assertion.Line 505 asserts that
isForWrite(derived from_opCtx->isUpsert()) is always false. This check is redundant because:
- Lines 348-350 in
_updatePosition()already skip calling_ensureRecordsFetched()for upsert operations- The assertion serves as a defensive check but adds no value in production builds
Consider removing this assertion and relying on the earlier guard, or convert it to a graceful early return if defense-in-depth is desired.
🔎 Suggested change
const txservice::TableName& tableName = _idx->getTableName(); uint64_t schemaVersion = _ru->getIndexSchema(tableName)->SchemaTs(); bool isForWrite = _opCtx->isUpsert(); - assert(!isForWrite); + if (isForWrite) { + // Should never reach here due to guard in _updatePosition, but handle gracefully + return; + }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/mongo/db/modules/eloq/src/eloq_index.cpp
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 404
File: src/mongo/db/exec/text_or.cpp:261-261
Timestamp: 2026-01-06T06:34:10.846Z
Learning: In the context of batch index row lookup for secondary index scans (eloqdoc repository), WorkingSetMember objects returned for read-only index scans have state RID_AND_OBJ, which includes RID, IDX (keyData), and OBJ, not just RID and OBJ as the state name suggests.
📚 Learning: 2025-11-24T03:11:24.955Z
Learnt from: lzxddz
Repo: eloqdata/eloqdoc PR: 325
File: src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp:1362-1384
Timestamp: 2025-11-24T03:11:24.955Z
Learning: The EloqDoc codebase uses an older version of glog that supports the two-parameter signature `google::InitGoogleLogging(const char* argv0, PrefixCallback)` for custom log prefix formatting, as seen in src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp line 1383.
Applied to files:
src/mongo/db/modules/eloq/src/eloq_index.cpp
📚 Learning: 2025-09-25T12:09:01.276Z
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 211
File: src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp:95-115
Timestamp: 2025-09-25T12:09:01.276Z
Learning: The log states (LOG_STATE_TYPE_RKDB, LOG_STATE_TYPE_RKDB_S3, LOG_STATE_TYPE_RKDB_GCS) in the EloqDoc codebase are mutually exclusive - only one can be active at a time. The LOG_STATE_TYPE_RKDB_CLOUD macro is a helper that should only be defined for cloud variants (S3/GCS) and not when regular RKDB is active.
Applied to files:
src/mongo/db/modules/eloq/src/eloq_index.cpp
🧬 Code graph analysis (1)
src/mongo/db/modules/eloq/src/eloq_index.cpp (3)
src/mongo/db/modules/eloq/src/eloq_record_store.cpp (10)
MONGO_LOG(98-126)MONGO_LOG(450-483)MONGO_LOG(519-526)MONGO_LOG(528-533)MONGO_LOG(535-540)MONGO_LOG(542-547)id(128-130)id(128-128)id(485-517)id(485-485)src/mongo/db/modules/eloq/src/base/eloq_key.h (7)
TxKey(270-272)TxKey(284-310)TxKey(285-287)TxKey(289-291)TxKey(293-309)TxKey(294-296)TxKey(298-300)src/mongo/db/modules/eloq/src/base/eloq_util.cpp (2)
TxErrorCodeToMongoStatus(11-75)TxErrorCodeToMongoStatus(11-11)
🔇 Additional comments (1)
src/mongo/db/modules/eloq/src/eloq_index.cpp (1)
742-746: Code already follows MongoDB conventions.Verification confirms that
internalEloqIndexBatchFetchSizeis declared asAtomicWord<int>insrc/mongo/db/query/query_knobs.h. The current implementation at lines 742-746 is already using the proper MongoDB pattern and requires no changes.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @src/mongo/db/exec/projection.cpp:
- Around line 150-153: Update the mismatched comment(s) that reference "10000
calls" to reflect the actual WINDOW_SIZE value of 100000; specifically adjust
the comments near the declarations of transformCallCount, totalLatencyUs, and
the WINDOW_SIZE constant so they state "resets every 100000 calls" (referencing
transformCallCount, totalLatencyUs, and WINDOW_SIZE in projection.cpp).
🧹 Nitpick comments (1)
src/mongo/db/exec/projection.cpp (1)
211-218: Consider using a higher log level for instrumentation output.
MONGO_LOG(0)logs at level 0, which is always enabled. In high-throughput scenarios, logging every 100,000 calls could add noise. Consider using a higher log level (e.g.,MONGO_LOG(1)orMONGO_LOG(2)) so this can be selectively enabled for debugging/profiling, or guarded behind a compile-time or runtime flag.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/mongo/db/exec/projection.cpp
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 404
File: src/mongo/db/exec/text_or.cpp:261-261
Timestamp: 2026-01-06T06:34:10.846Z
Learning: In the context of batch index row lookup for secondary index scans (eloqdoc repository), WorkingSetMember objects returned for read-only index scans have state RID_AND_OBJ, which includes RID, IDX (keyData), and OBJ, not just RID and OBJ as the state name suggests.
🪛 Cppcheck (2.19.0)
src/mongo/db/exec/projection.cpp
[error] 161-161: #error "Unsupported compiler family"
(preprocessorErrorDirective)
🔇 Additional comments (1)
src/mongo/db/exec/projection.cpp (1)
158-201: LGTM!The refactored control flow correctly handles both fast-path and non-fast-path cases. Member state mutations are properly consolidated within the fast-path block, and result assignment is consistent across both branches.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/mongo/db/exec/projection.cpp (1)
196-200: Minor: Redundant status assignment.
resultis already initialized toStatus::OK()on line 159. The assignment on line 200 is unnecessary since the fast path doesn't modifyresultto an error state.🔎 Suggested simplification
member->keyData.clear(); member->recordId = RecordId(); member->obj = Snapshotted<BSONObj>(SnapshotId(), bob.obj()); member->transitionToOwnedObj(); - result = Status::OK(); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/mongo/db/exec/projection.cpp
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 404
File: src/mongo/db/exec/text_or.cpp:261-261
Timestamp: 2026-01-06T06:34:10.846Z
Learning: In the context of batch index row lookup for secondary index scans (eloqdoc repository), WorkingSetMember objects returned for read-only index scans have state RID_AND_OBJ, which includes RID, IDX (keyData), and OBJ, not just RID and OBJ as the state name suggests.
🪛 Cppcheck (2.19.0)
src/mongo/db/exec/projection.cpp
[error] 161-161: #error "Unsupported compiler family"
(preprocessorErrorDirective)
🔇 Additional comments (2)
src/mongo/db/exec/projection.cpp (2)
31-32: LGTM!The new includes are appropriate for the atomic counters and timing functionality added to the
transformfunction.Also applies to: 45-46
206-218: LGTM - Lock-free statistics implementation.The atomic operations use appropriate memory orderings: relaxed for the incremental updates (sufficient for approximate statistics), and acquire-release for the window reset to ensure visibility of accumulated values. The window-based approach provides useful observability without lock contention.
Note: The comment on line 210 has the same "10000 calls" mismatch as previously flagged.
| currentIndexScanBatchIdx < prefetchedEndIdx) { | ||
| // Records already fetched for current position | ||
| return; | ||
| } |
There was a problem hiding this comment.
Please draw a simple graph in the comment to explain the concept of currentIndexScanBatchIdx and _prefetchedBatchStartIdx.
|
|
||
| // Get batch size from configuration (smaller than full scan batch) | ||
| size_t batchSize = _getBatchFetchSize(); | ||
| size_t endIdx = std::min(startIdx + batchSize, batchVector.size()); |
There was a problem hiding this comment.
Please declare endIdx as an argument of _fetchRecordsForRange, so others can guess the meaning of _fetchRecordsForRange without inspecting its implementation.
| _updateRecordPtr(); | ||
| } | ||
|
|
||
| void _ensureRecordsFetched() { |
There was a problem hiding this comment.
Please define a return type (status) instead of raising an exception.
| } | ||
|
|
||
| void _fetchRecordsForRange(size_t startIdx, | ||
| const std::vector<txservice::ScanBatchTuple>& batchVector) { |
There was a problem hiding this comment.
Please define a return type (status) instead of raising an exception.
| const auto& tuple = batchVector[i]; | ||
| // Skip Deleted records - they create holes in prefetchRecords (nullptr entries) | ||
| if (tuple.status_ == txservice::RecordStatus::Deleted) { | ||
| continue; |
There was a problem hiding this comment.
There shouldn't exist a deleted record.
| int batchSize = internalEloqIndexBatchFetchSize.load(); | ||
| // Ensure reasonable bounds: 1 to 1000 | ||
| return std::max(1, std::min(batchSize, 1000)); | ||
| } |
There was a problem hiding this comment.
Do not place member methods here. Place member methods together.
f311a24 to
51bbf36
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/mongo/db/modules/eloq/src/eloq_index.cpp (1)
736-746: Method placement after member variables.Past review suggested placing member methods together rather than after member variables. The helper methods
_clearPrefetchedRecords()and_getBatchFetchSize()are currently defined after the member variable declarations at lines 729-734.Consider moving these methods closer to other private methods for consistency, though this is a minor organizational concern.
Based on past review comment from xiexiaoy regarding line 746.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.gitignoresrc/mongo/db/exec/projection.cppsrc/mongo/db/exec/text_or.cppsrc/mongo/db/modules/eloq/src/eloq_cursor.cppsrc/mongo/db/modules/eloq/src/eloq_cursor.hsrc/mongo/db/modules/eloq/src/eloq_index.cppsrc/mongo/db/modules/eloq/src/eloq_recovery_unit.cppsrc/mongo/db/query/query_knobs.cppsrc/mongo/db/query/query_knobs.htests/jstests/core/projection_dotted_paths.js
🚧 Files skipped from review as they are similar to previous changes (4)
- src/mongo/db/query/query_knobs.h
- src/mongo/db/modules/eloq/src/eloq_recovery_unit.cpp
- src/mongo/db/modules/eloq/src/eloq_cursor.cpp
- .gitignore
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 404
File: src/mongo/db/exec/text_or.cpp:261-261
Timestamp: 2026-01-06T06:34:18.890Z
Learning: In the context of batch index row lookup for secondary index scans (eloqdoc repository), WorkingSetMember objects returned for read-only index scans have state RID_AND_OBJ, which includes RID, IDX (keyData), and OBJ, not just RID and OBJ as the state name suggests.
📚 Learning: 2026-01-06T06:34:10.846Z
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 404
File: src/mongo/db/exec/text_or.cpp:261-261
Timestamp: 2026-01-06T06:34:10.846Z
Learning: Guideline: In code paths involving batch index row lookups for secondary index scans in eloqdata/eloqdoc, treat the WorkingSetMember state RID_AND_OBJ as containing RID, IDX (keyData), and OBJ, not just RID and OBJ as the state name might imply. When reviewing, verify any references to this state reflect that IDX/keyData is included. Update comments, tests, and any assertions that assume RID_AND_OBJ excludes IDX to avoid incorrect behavior around read-only index scans.
Applied to files:
src/mongo/db/exec/text_or.cpp
📚 Learning: 2025-09-25T12:24:06.434Z
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 211
File: src/mongo/db/modules/eloq/cmake/build_eloq_log_service.cmake:26-80
Timestamp: 2025-09-25T12:24:06.434Z
Learning: The user githubzilla implemented a fix in commit fe98aaf to address the MEMORY state incorrectly triggering RocksDB discovery in build_eloq_log_service.cmake. This was a control flow issue where MEMORY builds were falling into an else branch that still attempted RocksDB discovery.
Applied to files:
src/mongo/db/exec/text_or.cpptests/jstests/core/projection_dotted_paths.js
📚 Learning: 2025-11-28T10:55:21.020Z
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 279
File: src/mongo/db/modules/eloq/CMakeLists.txt:54-55
Timestamp: 2025-11-28T10:55:21.020Z
Learning: In src/mongo/db/modules/eloq/CMakeLists.txt, the CMake options EXT_TX_PROC_ENABLED, FORK_HM_PROCESS, and STATISTICS are used by the data_substrate submodule build system and should be retained as configuration interface options.
Applied to files:
src/mongo/db/query/query_knobs.cppsrc/mongo/db/modules/eloq/src/eloq_index.cpp
📚 Learning: 2026-01-06T06:34:18.890Z
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 404
File: src/mongo/db/exec/text_or.cpp:261-261
Timestamp: 2026-01-06T06:34:18.890Z
Learning: In the context of batch index row lookup for secondary index scans (eloqdoc repository), WorkingSetMember objects returned for read-only index scans have state RID_AND_OBJ, which includes RID, IDX (keyData), and OBJ, not just RID and OBJ as the state name suggests.
Applied to files:
tests/jstests/core/projection_dotted_paths.js
📚 Learning: 2025-11-24T03:11:24.955Z
Learnt from: lzxddz
Repo: eloqdata/eloqdoc PR: 325
File: src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp:1362-1384
Timestamp: 2025-11-24T03:11:24.955Z
Learning: The EloqDoc codebase uses an older version of glog that supports the two-parameter signature `google::InitGoogleLogging(const char* argv0, PrefixCallback)` for custom log prefix formatting, as seen in src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp line 1383.
Applied to files:
src/mongo/db/modules/eloq/src/eloq_index.cpp
📚 Learning: 2025-09-25T12:09:01.276Z
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 211
File: src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp:95-115
Timestamp: 2025-09-25T12:09:01.276Z
Learning: The log states (LOG_STATE_TYPE_RKDB, LOG_STATE_TYPE_RKDB_S3, LOG_STATE_TYPE_RKDB_GCS) in the EloqDoc codebase are mutually exclusive - only one can be active at a time. The LOG_STATE_TYPE_RKDB_CLOUD macro is a helper that should only be defined for cloud variants (S3/GCS) and not when regular RKDB is active.
Applied to files:
src/mongo/db/modules/eloq/src/eloq_index.cpp
🧬 Code graph analysis (2)
src/mongo/db/modules/eloq/src/eloq_cursor.h (1)
src/mongo/db/modules/eloq/src/eloq_index.cpp (1)
assert(373-401)
src/mongo/db/modules/eloq/src/eloq_index.cpp (3)
src/mongo/db/modules/eloq/src/eloq_record_store.cpp (10)
MONGO_LOG(98-126)MONGO_LOG(450-483)MONGO_LOG(519-526)MONGO_LOG(528-533)MONGO_LOG(535-540)MONGO_LOG(542-547)id(128-130)id(128-128)id(485-517)id(485-485)src/mongo/db/modules/eloq/src/base/eloq_key.h (7)
TxKey(270-272)TxKey(284-310)TxKey(285-287)TxKey(289-291)TxKey(293-309)TxKey(294-296)TxKey(298-300)src/mongo/db/modules/eloq/src/base/eloq_util.cpp (2)
TxErrorCodeToMongoStatus(11-75)TxErrorCodeToMongoStatus(11-11)
🪛 Cppcheck (2.19.0)
src/mongo/db/exec/projection.cpp
[error] 161-161: #error "Unsupported compiler family"
(preprocessorErrorDirective)
🔇 Additional comments (13)
src/mongo/db/query/query_knobs.cpp (1)
84-85: LGTM!The new server parameter follows the existing pattern in this file and provides a configurable batch fetch size for Eloq index operations. The default value of 100 is reasonable, and the bounds enforcement [1, 1000] is appropriately handled at the consumption site in
_getBatchFetchSize().src/mongo/db/exec/text_or.cpp (2)
47-47: LGTM!The
using std::stringdeclaration is appropriately moved to be consistent with other using declarations.
261-263: Good documentation of the invariant change.The comment adequately explains why the
RID_AND_IDXinvariant was removed. Based on the learnings, with batch index row lookup,WorkingSetMemberstate isRID_AND_OBJwhich includes RID, IDX (keyData), and OBJ. The retainedinvariant(1 == wsm->keyData.size())confirms the keyData dependency is still validated.src/mongo/db/modules/eloq/src/eloq_cursor.h (1)
51-63: LGTM!The new const accessors provide clean read-only access to the batch state. The assertion in
getCurrentBatchTupleIdx()ensures valid cursor state, and callers in_ensureRecordsFetched()appropriately verifybatchVectoris non-empty before invoking these methods.tests/jstests/core/projection_dotted_paths.js (1)
10-98: Tests refactored with consistent assertion patterns.The projection tests have been streamlined with uniform
resultDoc/explainpatterns. Core coverage for dotted paths, multikey handling, and covered index plans is preserved.src/mongo/db/exec/projection.cpp (2)
155-220: Lock-free latency instrumentation looks correct.The windowed statistics collection pattern is consistent with other instrumentation in this PR. The atomic operations with appropriate memory ordering ensure thread-safe updates without locking overhead.
160-201: Refactored transform() control flow is cleaner.The explicit
Status resultvariable with unified return at line 220 improves readability compared to early returns. The fast-path handling forSIMPLE_DOCandCOVERED_ONE_INDEXis preserved correctly.src/mongo/db/modules/eloq/src/eloq_index.cpp (6)
69-96: LGTM - Cursor lifetime instrumentation.The lock-free latency tracking for cursor lifetime follows the same pattern used elsewhere in this PR. Statistics are collected every 10,000 cursors, which is appropriate for monitoring performance.
373-401: Well-structured lazy fetch implementation.The
_ensureRecordsFetched()method correctly:
- Uses batch count (not size) to detect new batches - addressing the prior review concern
- Checks if the current position is already within the prefetched range before fetching
- Delegates to
_fetchRecordsForRange()only when neededThe batch count comparison at line 381 is more reliable than size comparison for detecting batch transitions.
422-450: Deletion handling retained despite past review suggestion.Lines 424-427 skip
Deletedrecords, and line 430 assertsNormalstatus. Past reviews suggested these checks are redundant since "scan won't return a non-normal record."If the scan truly guarantees only
Normalstatus tuples, consider removing the deletion skip and simplifying the loop. However, if there's a defensive reason to keep this (e.g., future-proofing), documenting why would help.Based on past review comments from xiexiaoy.
504-505: Upsert prefetch guard correctly implemented.The assertion
assert(!isForWrite)at line 505 ensures thatbatchGetKVis never called with write intent during prefetch, addressing the past review concern about avoiding write-intent acquisition during upserts.
541-549: Good error handling with state cleanup.On batch fetch failure, prefetched records are correctly cleared before propagating the error via
uassertStatusOK. This prevents stale data from persisting after an error.
570-615: LGTM - _updateRecordPtr() implementation.The method correctly handles all three index cursor types:
IDindexes use_scanTupleRecorddirectlyUNIQUE/STANDARDindexes use prefetched records when not in upsert mode- Appropriate logging for debugging when records aren't found in prefetched range
51bbf36 to
0174248
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In @src/mongo/db/modules/eloq/src/eloq_index.cpp:
- Around line 20-29: Add an explicit #include <algorithm> to the top of
eloq_index.cpp (with the other includes) so uses of std::min/std::max are not
relying on transitive includes; also ensure any calls in the file use the std::
prefix if they don't already, and apply the same include addition to the other
affected sections (around lines 403-410 and 742-746) where std::min/std::max are
used.
- Around line 73-97: The destructor-only timing is wrong for pooled re-use; move
the latency-stop/aggregation logic out of EloqIndexCursor::~EloqIndexCursor and
into EloqIndexCursor::reset() so each checkout records the previous lifetime and
then restarts timing: in reset() call _timer.stop(), compute latencyUs and
update the same atomics/aggregation window (cursorLifetimeCallCount,
totalLatencyUs, WINDOW_SIZE) and then call _timer.start() to begin the new
session; remove or simplify the destructor’s stats logic (or have the destructor
call reset() to flush one final sample) so you don’t double-record.
In @tests/jstests/core/projection_dotted_paths.js:
- Around line 35-43: The stray explain assignment using coll.find({a: 1}, {_id:
0, "b.c": 1, c: 1}) should be removed because it is immediately overwritten and
doesn't match the subsequent “include _id” assertions; delete that line so the
only explain call is explain = coll.find({a: 1}, {_id: 1, a: 1, "b.c": 1, "b.d":
1, c: 1}).explain("queryPlanner"), leaving the assertions that call isIxscan and
isIndexOnly to operate on the correct explain result.
🧹 Nitpick comments (5)
src/mongo/db/query/query_knobs.h (1)
127-128: Consider adding a server-parameter validator (range) to match the intended clamp (1..1000).Right now the clamp lives in
EloqIndexCursor::_getBatchFetchSize(). A validator on the server parameter would prevent surprising negative/zero values from ever being set.src/mongo/db/query/query_knobs.cpp (1)
85-86: Wire a validator/default bounds here if possible (instead of only clamping at call sites).src/mongo/db/modules/eloq/src/eloq_recovery_unit.cpp (1)
457-507: Sorting + restore looks correct; consider improving exception safety around moving out ofbatch.If
txservice::BatchReadTxRequestconstruction throws,batchis already partially moved-from. If exceptions are realistic here, consider sorting indices andstd::swap-reordering in-place (or copying keys only) before the request is constructed.src/mongo/db/modules/eloq/src/eloq_cursor.h (1)
51-63: Preferinvariant/dassertand document the “valid only afternextBatchTuple()” contract.
getCurrentBatchTupleIdx()’s_scanBatchIdx - 1convention is easy to misuse; a short comment on when it’s valid would help, andassertmay be compiled out in release builds.src/mongo/db/modules/eloq/src/eloq_index.cpp (1)
742-746: Consider using.loadRelaxed()/ explicit memory order if available/idiomatic for this atomic knob.Not required, but it can make the intent (performance-only config read) clearer.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.gitignoresrc/mongo/db/exec/projection.cppsrc/mongo/db/exec/text_or.cppsrc/mongo/db/modules/eloq/src/eloq_cursor.cppsrc/mongo/db/modules/eloq/src/eloq_cursor.hsrc/mongo/db/modules/eloq/src/eloq_index.cppsrc/mongo/db/modules/eloq/src/eloq_recovery_unit.cppsrc/mongo/db/query/query_knobs.cppsrc/mongo/db/query/query_knobs.htests/jstests/core/projection_dotted_paths.js
🚧 Files skipped from review as they are similar to previous changes (3)
- .gitignore
- src/mongo/db/exec/text_or.cpp
- src/mongo/db/modules/eloq/src/eloq_cursor.cpp
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 404
File: src/mongo/db/exec/text_or.cpp:261-261
Timestamp: 2026-01-06T06:34:18.890Z
Learning: In the context of batch index row lookup for secondary index scans (eloqdoc repository), WorkingSetMember objects returned for read-only index scans have state RID_AND_OBJ, which includes RID, IDX (keyData), and OBJ, not just RID and OBJ as the state name suggests.
📚 Learning: 2025-11-28T10:55:21.020Z
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 279
File: src/mongo/db/modules/eloq/CMakeLists.txt:54-55
Timestamp: 2025-11-28T10:55:21.020Z
Learning: In src/mongo/db/modules/eloq/CMakeLists.txt, the CMake options EXT_TX_PROC_ENABLED, FORK_HM_PROCESS, and STATISTICS are used by the data_substrate submodule build system and should be retained as configuration interface options.
Applied to files:
src/mongo/db/query/query_knobs.hsrc/mongo/db/query/query_knobs.cppsrc/mongo/db/modules/eloq/src/eloq_cursor.hsrc/mongo/db/modules/eloq/src/eloq_index.cpp
📚 Learning: 2026-01-06T06:34:18.890Z
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 404
File: src/mongo/db/exec/text_or.cpp:261-261
Timestamp: 2026-01-06T06:34:18.890Z
Learning: In the context of batch index row lookup for secondary index scans (eloqdoc repository), WorkingSetMember objects returned for read-only index scans have state RID_AND_OBJ, which includes RID, IDX (keyData), and OBJ, not just RID and OBJ as the state name suggests.
Applied to files:
tests/jstests/core/projection_dotted_paths.js
📚 Learning: 2025-09-25T12:24:06.434Z
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 211
File: src/mongo/db/modules/eloq/cmake/build_eloq_log_service.cmake:26-80
Timestamp: 2025-09-25T12:24:06.434Z
Learning: The user githubzilla implemented a fix in commit fe98aaf to address the MEMORY state incorrectly triggering RocksDB discovery in build_eloq_log_service.cmake. This was a control flow issue where MEMORY builds were falling into an else branch that still attempted RocksDB discovery.
Applied to files:
tests/jstests/core/projection_dotted_paths.js
📚 Learning: 2025-11-24T03:11:24.955Z
Learnt from: lzxddz
Repo: eloqdata/eloqdoc PR: 325
File: src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp:1362-1384
Timestamp: 2025-11-24T03:11:24.955Z
Learning: The EloqDoc codebase uses an older version of glog that supports the two-parameter signature `google::InitGoogleLogging(const char* argv0, PrefixCallback)` for custom log prefix formatting, as seen in src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp line 1383.
Applied to files:
src/mongo/db/modules/eloq/src/eloq_index.cpp
🧬 Code graph analysis (1)
src/mongo/db/modules/eloq/src/eloq_cursor.h (1)
src/mongo/db/modules/eloq/src/eloq_index.cpp (1)
assert(373-401)
🪛 Cppcheck (2.19.0)
src/mongo/db/exec/projection.cpp
[error] 161-161: #error "Unsupported compiler family"
(preprocessorErrorDirective)
| #include <atomic> | ||
| #include <cassert> | ||
| #include <memory> | ||
| #include <string_view> | ||
| #include <utility> | ||
|
|
||
| #include "mongo/base/object_pool.h" | ||
| #include "mongo/db/index/index_descriptor.h" | ||
| #include "mongo/db/query/query_knobs.h" | ||
| #include "mongo/util/log.h" |
There was a problem hiding this comment.
Add <algorithm> explicitly (uses std::min/std::max).
Relying on transitive includes for std::min/std::max is fragile.
Proposed diff
#include <atomic>
+#include <algorithm>
#include <cassert>
#include <memory>
#include <string_view>
#include <utility>Also applies to: 403-410, 742-746
🤖 Prompt for AI Agents
In @src/mongo/db/modules/eloq/src/eloq_index.cpp around lines 20 - 29, Add an
explicit #include <algorithm> to the top of eloq_index.cpp (with the other
includes) so uses of std::min/std::max are not relying on transitive includes;
also ensure any calls in the file use the std:: prefix if they don't already,
and apply the same include addition to the other affected sections (around lines
403-410 and 742-746) where std::min/std::max are used.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @src/mongo/db/exec/projection.cpp:
- Around line 45-46: Remove the unused include directives by deleting the lines
that read '#include <butil/time.h>' and '#include <atomic>' from projection.cpp;
ensure no code references them afterward and rebuild to confirm there are no
missing dependencies.
- Around line 149-196: The COVERED_ONE_INDEX branch in
ProjectionStage::transform assumes keyIndex matches _includeKey.size() and
_keyFieldNames.size(); add explicit bounds checks/invariants before accessing
_includeKey[keyIndex] and _keyFieldNames[keyIndex] (e.g., assert/invariant that
_includeKey.size() == _keyFieldNames.size() and that keyIndex < both sizes, or
if violated return an error Status) so the loop over BSONObjIterator on
member->keyData[0].keyData cannot index out of range; update the loop to check
keyIndex against those sizes before using _includeKey[keyIndex] or
_keyFieldNames[keyIndex] and fail fast with a clear invariant or Status if the
contract is violated.
In @src/mongo/db/modules/eloq/src/eloq_cursor.cpp:
- Line 20: Remove the unused includes <atomic> and <butil/time.h> from
eloq_cursor.cpp (and eloq_cursor.h if present); locate the top-of-file include
lines referencing these headers and delete them, ensuring no other code depends
on symbols from those headers and leaving only the truly used includes (e.g.,
existing eloq_cursor.h, tx_* headers if present).
🧹 Nitpick comments (3)
src/mongo/db/modules/eloq/src/eloq_index.cpp (2)
379-454: Review deleted record handling logic.The implementation correctly handles deleted records in the index scan batch by:
- Maintaining
recordIdsIdxToBatchIdxmapping (lines 394, 423-424) to track the relationship between the filtered recordIds vector and the original batchVector- Skipping deleted records when building the recordIds vector (lines 401-403)
- Short-circuiting if all records are deleted (lines 450-454)
However, there's a subtle complexity: the code creates a sparse
_prefetchedRecordsvector (line 447) where deleted entries remain asnullptr. This is correct but adds complexity. Ensure this is well-understood by future maintainers.Consider adding a comment at line 447 explaining the sparse vector design:
// Reserve capacity and initialize with nullptr (for deleted records) // Creates a sparse vector where deleted entries remain as nullptr, // and only Normal status tuples have allocated MongoRecord objects
515-560: Verify offset calculation correctness in _updateRecordPtr.The lookup logic (lines 530-551) calculates the prefetch offset as:
size_t offset = currentIndexScanBatchIdx - _prefetchedBatchStartIdx;This appears correct given that:
_prefetchedBatchStartIdxis set tostartIdxin_fetchRecordsForRange(line 439)- The sparse
_prefetchedRecordsvector uses indices relative to this start (line 465)However, the bounds checking could be more explicit. The current check (line 534) verifies
offset < _prefetchedRecords.size(), but it might be clearer to also assert the relationship:assert(currentIndexScanBatchIdx >= _prefetchedBatchStartIdx); assert(offset < _prefetchedRecords.size());Consider strengthening the bounds checking for clarity:
♻️ Enhanced bounds checking
size_t currentIndexScanBatchIdx = _cursor->getCurrentBatchTupleIdx(); + assert(currentIndexScanBatchIdx >= _prefetchedBatchStartIdx && + "Current scan index should be >= prefetch start"); if (currentIndexScanBatchIdx >= _prefetchedBatchStartIdx) { size_t offset = currentIndexScanBatchIdx - _prefetchedBatchStartIdx; + assert(offset < _prefetchedRecords.size() && + "Offset should be within prefetched range"); if (offset < _prefetchedRecords.size() &&src/mongo/db/exec/projection.cpp (1)
31-32: Drop<atomic>unless it’s used (or add the intended usage in this PR).
Right now it’s unused in this TU and may trip IWYU / add avoidable compile cost.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/mongo/db/exec/projection.cppsrc/mongo/db/modules/eloq/data_substratesrc/mongo/db/modules/eloq/src/eloq_cursor.cppsrc/mongo/db/modules/eloq/src/eloq_index.cpp
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 404
File: src/mongo/db/exec/text_or.cpp:261-261
Timestamp: 2026-01-06T06:34:18.890Z
Learning: In the context of batch index row lookup for secondary index scans (eloqdoc repository), WorkingSetMember objects returned for read-only index scans have state RID_AND_OBJ, which includes RID, IDX (keyData), and OBJ, not just RID and OBJ as the state name suggests.
📚 Learning: 2025-11-28T10:55:21.020Z
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 279
File: src/mongo/db/modules/eloq/CMakeLists.txt:54-55
Timestamp: 2025-11-28T10:55:21.020Z
Learning: In src/mongo/db/modules/eloq/CMakeLists.txt, the CMake options EXT_TX_PROC_ENABLED, FORK_HM_PROCESS, and STATISTICS are used by the data_substrate submodule build system and should be retained as configuration interface options.
Applied to files:
src/mongo/db/modules/eloq/data_substratesrc/mongo/db/modules/eloq/src/eloq_cursor.cppsrc/mongo/db/modules/eloq/src/eloq_index.cpp
📚 Learning: 2025-11-24T03:11:24.955Z
Learnt from: lzxddz
Repo: eloqdata/eloqdoc PR: 325
File: src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp:1362-1384
Timestamp: 2025-11-24T03:11:24.955Z
Learning: The EloqDoc codebase uses an older version of glog that supports the two-parameter signature `google::InitGoogleLogging(const char* argv0, PrefixCallback)` for custom log prefix formatting, as seen in src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp line 1383.
Applied to files:
src/mongo/db/modules/eloq/src/eloq_cursor.cppsrc/mongo/db/modules/eloq/src/eloq_index.cpp
📚 Learning: 2025-09-25T12:09:01.276Z
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 211
File: src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp:95-115
Timestamp: 2025-09-25T12:09:01.276Z
Learning: The log states (LOG_STATE_TYPE_RKDB, LOG_STATE_TYPE_RKDB_S3, LOG_STATE_TYPE_RKDB_GCS) in the EloqDoc codebase are mutually exclusive - only one can be active at a time. The LOG_STATE_TYPE_RKDB_CLOUD macro is a helper that should only be defined for cloud variants (S3/GCS) and not when regular RKDB is active.
Applied to files:
src/mongo/db/modules/eloq/src/eloq_cursor.cpp
🪛 Cppcheck (2.19.0)
src/mongo/db/exec/projection.cpp
[error] 161-161: #error "Unsupported compiler family"
(preprocessorErrorDirective)
🔇 Additional comments (12)
src/mongo/db/modules/eloq/data_substrate (1)
1-1: Data_substrate submodule update aligns with batch fetch feature implementation.The updated commit supports the batch lookup infrastructure visible in
eloq_index.cpp(prefetch batch detection, batch size determination) andeloq_cursor.cpp(prefetch size configuration). The submodule provides transaction service, store handler, and logging infrastructure consumed throughout the Eloq module.Ensure CI validation passes with this submodule version to confirm compatibility with the new batch fetch logic in the parent repository.
src/mongo/db/modules/eloq/src/eloq_cursor.cpp (2)
194-198: Good: Prefetch size configuration enables batch optimization.Setting
prefetch_slice_cnt_before executing the batch request enables the prefetch semantics that align with the new batch fetch size control. This change properly integrates with the lazy batch prefetching infrastructure introduced in the related index cursor changes.
194-194: ThePrefetchSize()method is properly defined in theEloqCursorclass header file (eloq_cursor.hat line 65) asuint32_t PrefetchSize() {. The code on line 194 correctly calls this member method with no issues.Likely an incorrect or invalid review comment.
src/mongo/db/modules/eloq/src/eloq_index.cpp (9)
107-109: Good: Proper prefetch state management on cursor reset and seek.Clearing prefetched records during reset and seek operations prevents stale data issues and ensures consistency with the new cursor position. This is essential for correctness of the lazy fetch mechanism.
Also applies to: 295-295
349-377: Good: Efficient batch change detection using batch count.Using
_lastRecordsBatchCnt(line 357) to detect new batches is more reliable than comparing vector sizes, as acknowledged in the AI summary. This approach correctly handles the case where a new batch might coincidentally have the same size as the previous one.The prefetch range check (lines 369-373) efficiently avoids redundant fetches when the current position is already covered.
407-421: Good: Correct RecordId extraction for different index types.The code correctly handles RecordId extraction differently for UNIQUE vs STANDARD indexes:
- UNIQUE indexes: RecordId is stored in the record data (lines 410-413)
- STANDARD indexes: RecordId is appended to the key (lines 416-420)
This aligns with the EloqDoc index design. Based on learnings, WorkingSetMember objects have state RID_AND_OBJ which includes both RID and the indexed key data.
496-513: Good: Debug assertions validate fetch correctness.The debug-only verification (lines 496-506) ensures that:
- All fetched records have
Normalstatus- No records are
nullptrafterbatchGetKVThis is a good defensive programming practice. The assertions help catch issues during development without impacting production performance.
The final bounds check (line 508) ensures the prefetch range is valid.
674-691: Good: Clean helper methods for state management.The private members and helper methods are well-designed:
_clearPrefetchedRecords()(lines 681-685) provides a single place to reset all prefetch state_getBatchFetchSize()(lines 687-691) enforces reasonable bounds [1, 1000] on the batch sizeThe bounds enforcement prevents both underflow (minimum 1) and excessive memory allocation (maximum 1000).
379-513: Consider edge case: concurrent batch advancement during prefetch.While the current implementation appears correct for single-threaded cursor usage, consider whether there's any risk of the underlying
_cursorbatch advancing (vianextBatchTuple()calls) while_fetchRecordsForRangeis executing thebatchGetKVcall.If the cursor's batch can advance concurrently (e.g., due to async I/O or yielding during
batchGetKV), the_prefetchedBatchStartIdxmight become stale. However, based on the codebase patterns, cursor operations appear to be serialized per-cursor, so this should not be an issue.Can you confirm that:
batchGetKV(line 484) is synchronous and doesn't yield control to other operations?- No other code can call
_cursor->nextBatchTuple()while_fetchRecordsForRangeis executing?
315-327: The new EloqCursor accessor methods exist and are correctly used.All three accessor methods are properly declared in
eloq_cursor.has const methods:
getCurrentBatchTupleIdx()(line 52) — returnssize_tgetCurrentBatchVector()(line 57) — returnsconst std::vector<txservice::ScanBatchTuple>&getScanBatchCnt()(line 61) — returnssize_tThese methods are called as expected within
_ensureRecordsFetched()at the referenced lines and return the correct types for their usage.
20-20: External dependencies are properly declared.The variable
internalEloqIndexBatchFetchSizeis correctly declared asextern AtomicWord<int>inquery_knobs.hand exported inquery_knobs.cpp. The include directive at line 28 properly imports these declarations.
456-494: Pre-allocated MongoRecord pattern is correct and supported.The implementation correctly pre-allocates
MongoRecordobjects in_prefetchedRecordsbefore callingbatchGetKV. ThebatchGetKVfunction takes thebatchvector by reference, passes it throughBatchReadTxRequestfor execution, and populates the pre-allocated records in-place. This is the intended usage pattern—the pointers stored inScanBatchTuple(constructed at line 473-474) are dereferenced during execution to populate the records, as evidenced by the subsequent access totuple.record_andtuple.status_after the batch operation completes.
| Status ProjectionStage::transform(WorkingSetMember* member) { | ||
| // The default no-fast-path case. | ||
| Status result = Status::OK(); | ||
| if (ProjectionStageParams::NO_FAST_PATH == _projImpl) { | ||
| return _exec->transform(member); | ||
| } | ||
|
|
||
| BSONObjBuilder bob; | ||
|
|
||
| // Note that even if our fast path analysis is bug-free something that is | ||
| // covered might be invalidated and just be an obj. In this case we just go | ||
| // through the SIMPLE_DOC path which is still correct if the covered data | ||
| // is not available. | ||
| // | ||
| // SIMPLE_DOC implies that we expect an object so it's kind of redundant. | ||
| if ((ProjectionStageParams::SIMPLE_DOC == _projImpl) || member->hasObj()) { | ||
| // If we got here because of SIMPLE_DOC the planner shouldn't have messed up. | ||
| invariant(member->hasObj()); | ||
|
|
||
| // Apply the SIMPLE_DOC projection. | ||
| transformSimpleInclusion(member->obj.value(), _includedFields, bob); | ||
| result = _exec->transform(member); | ||
| } else { | ||
| invariant(ProjectionStageParams::COVERED_ONE_INDEX == _projImpl); | ||
| // We're pulling data out of the key. | ||
| invariant(1 == member->keyData.size()); | ||
| size_t keyIndex = 0; | ||
|
|
||
| // Look at every key element... | ||
| BSONObjIterator keyIterator(member->keyData[0].keyData); | ||
| while (keyIterator.more()) { | ||
| BSONElement elt = keyIterator.next(); | ||
| // If we're supposed to include it... | ||
| if (_includeKey[keyIndex]) { | ||
| // Do so. | ||
| bob.appendAs(elt, _keyFieldNames[keyIndex]); | ||
| BSONObjBuilder bob; | ||
|
|
||
| // Note that even if our fast path analysis is bug-free something that is | ||
| // covered might be invalidated and just be an obj. In this case we just go | ||
| // through the SIMPLE_DOC path which is still correct if the covered data | ||
| // is not available. | ||
| // | ||
| // SIMPLE_DOC implies that we expect an object so it's kind of redundant. | ||
| if ((ProjectionStageParams::SIMPLE_DOC == _projImpl) || member->hasObj()) { | ||
| // If we got here because of SIMPLE_DOC the planner shouldn't have messed up. | ||
| invariant(member->hasObj()); | ||
|
|
||
| // Apply the SIMPLE_DOC projection. | ||
| transformSimpleInclusion(member->obj.value(), _includedFields, bob); | ||
| } else { | ||
| invariant(ProjectionStageParams::COVERED_ONE_INDEX == _projImpl); | ||
| // We're pulling data out of the key. | ||
| invariant(1 == member->keyData.size()); | ||
| size_t keyIndex = 0; | ||
|
|
||
| // Look at every key element... | ||
| BSONObjIterator keyIterator(member->keyData[0].keyData); | ||
| while (keyIterator.more()) { | ||
| BSONElement elt = keyIterator.next(); | ||
| // If we're supposed to include it... | ||
| if (_includeKey[keyIndex]) { | ||
| // Do so. | ||
| bob.appendAs(elt, _keyFieldNames[keyIndex]); | ||
| } | ||
| ++keyIndex; | ||
| } | ||
| ++keyIndex; | ||
| } | ||
|
|
||
| member->keyData.clear(); | ||
| member->recordId = RecordId(); | ||
| member->obj = Snapshotted<BSONObj>(SnapshotId(), bob.obj()); | ||
| member->transitionToOwnedObj(); | ||
| result = Status::OK(); | ||
| } | ||
|
|
||
| member->keyData.clear(); | ||
| member->recordId = RecordId(); | ||
| member->obj = Snapshotted<BSONObj>(SnapshotId(), bob.obj()); | ||
| member->transitionToOwnedObj(); | ||
| return Status::OK(); | ||
| return result; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find where coveredKeyObj / COVERED_ONE_INDEX is constructed
rg -n --no-heading 'coveredKeyObj|COVERED_ONE_INDEX' -S src/mongo/db | head -n 200Repository: eloqdata/eloqdoc
Length of output: 2661
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# First, let's look at the projection.cpp file to understand the structure better
wc -l src/mongo/db/exec/projection.cppRepository: eloqdata/eloqdoc
Length of output: 96
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Let's examine the class definition and member variables in projection.cpp
head -n 150 src/mongo/db/exec/projection.cppRepository: eloqdata/eloqdoc
Length of output: 6004
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find where _includeKey and _keyFieldNames are declared and initialized
rg -n '_includeKey|_keyFieldNames' -A 2 -B 2 src/mongo/db/exec/projection.cppRepository: eloqdata/eloqdoc
Length of output: 1044
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check KeyData struct definition and how keyData is populated
rg -n 'struct KeyData|class KeyData' -A 5 src/mongo/db/exec/Repository: eloqdata/eloqdoc
Length of output: 42
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for where keyData is set/populated in the codebase
rg -n 'keyData\[' -B 2 -A 2 src/mongo/db/exec/ | head -n 100Repository: eloqdata/eloqdoc
Length of output: 3638
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check the working_set.h to understand member structure
rg -n 'struct WorkingSetMember|class WorkingSetMember' -A 30 src/mongo/db/exec/ | head -n 100Repository: eloqdata/eloqdoc
Length of output: 6180
🏁 Script executed:
#!/bin/bash
set -euo pypefail
# Look for any size validation between _coveredKeyObj and actual key data
rg -n '_coveredKeyObj' -B 3 -A 3 src/mongo/db/exec/projection.cppRepository: eloqdata/eloqdoc
Length of output: 113
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for KeyData struct definition
rg -n 'struct IndexKeyDatum|struct KeyData' -A 10 src/mongo/db/exec/working_set.hRepository: eloqdata/eloqdoc
Length of output: 531
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for validation of keyData in working_set.cpp around line 225-230
sed -n '220,240p' src/mongo/db/exec/working_set.cppRepository: eloqdata/eloqdoc
Length of output: 751
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for where the contract between coveredKeyObj and actual key elements is enforced
rg -n 'indexKeyPattern|getKeys' -B 5 -A 5 src/mongo/db/exec/working_set.cpp | head -n 80Repository: eloqdata/eloqdoc
Length of output: 509
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if there's validation that key pattern size matches key data size anywhere
rg -n 'keyPatternIt\.more|keyDataIt\.more' -B 3 -A 3 src/mongo/db/exec/ | head -n 150Repository: eloqdata/eloqdoc
Length of output: 2134
Add invariants to validate keyIndex bounds in COVERED_ONE_INDEX fast path.
The loop assumes the number of key elements always matches _includeKey and _keyFieldNames sizes. While other code paths (e.g., working_set.cpp, filter.h) enforce this contract with runtime checks, the projection transform function does not. Add bounds validation before array access to catch contract violations.
Proposed fix
// Look at every key element...
BSONObjIterator keyIterator(member->keyData[0].keyData);
while (keyIterator.more()) {
+ invariant(keyIndex < _includeKey.size());
BSONElement elt = keyIterator.next();
// If we're supposed to include it...
if (_includeKey[keyIndex]) {
// Do so.
bob.appendAs(elt, _keyFieldNames[keyIndex]);
}
++keyIndex;
}
+ invariant(keyIndex == _includeKey.size());🧰 Tools
🪛 Cppcheck (2.19.0)
[error] 161-161: #error "Unsupported compiler family"
(preprocessorErrorDirective)
🤖 Prompt for AI Agents
In @src/mongo/db/exec/projection.cpp around lines 149 - 196, The
COVERED_ONE_INDEX branch in ProjectionStage::transform assumes keyIndex matches
_includeKey.size() and _keyFieldNames.size(); add explicit bounds
checks/invariants before accessing _includeKey[keyIndex] and
_keyFieldNames[keyIndex] (e.g., assert/invariant that _includeKey.size() ==
_keyFieldNames.size() and that keyIndex < both sizes, or if violated return an
error Status) so the loop over BSONObjIterator on member->keyData[0].keyData
cannot index out of range; update the loop to check keyIndex against those sizes
before using _includeKey[keyIndex] or _keyFieldNames[keyIndex] and fail fast
with a clear invariant or Status if the contract is violated.
| */ | ||
| #define MONGO_LOG_DEFAULT_COMPONENT ::mongo::logger::LogComponent::kStorage | ||
|
|
||
| #include <atomic> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if atomic or butil/time.h types are used in eloq_cursor.h or cpp
rg -n --type=cpp -C2 '\b(std::atomic|bvar::LatencyRecorder|butil::)\b' src/mongo/db/modules/eloq/src/eloq_cursor.h src/mongo/db/modules/eloq/src/eloq_cursor.cppRepository: eloqdata/eloqdoc
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Also check for PrefetchSize method to understand the context
rg -n --type=cpp 'PrefetchSize' src/mongo/db/modules/eloq/src/eloq_cursor.h src/mongo/db/modules/eloq/src/eloq_cursor.cppRepository: eloqdata/eloqdoc
Length of output: 243
🏁 Script executed:
#!/bin/bash
# Check what's actually included at the top of eloq_cursor.cpp to see the new includes
head -n 35 src/mongo/db/modules/eloq/src/eloq_cursor.cppRepository: eloqdata/eloqdoc
Length of output: 1337
🏁 Script executed:
#!/bin/bash
# Search for atomic or butil usage in the main header file
cat src/mongo/db/modules/eloq/src/eloq_cursor.h | grep -i -E '(atomic|butil)'Repository: eloqdata/eloqdoc
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Check the data_substrate headers that are included
rg -n --type=cpp '\b(std::atomic|butil::)\b' src/mongo/db/modules/eloq/data_substrate/tx_service/include/Repository: eloqdata/eloqdoc
Length of output: 160
🏁 Script executed:
#!/bin/bash
# Get the full content of eloq_cursor.h to see what's actually in there
wc -l src/mongo/db/modules/eloq/src/eloq_cursor.hRepository: eloqdata/eloqdoc
Length of output: 107
🏁 Script executed:
#!/bin/bash
# Find the tx_execution.h and tx_record.h files
fd -type f 'tx_execution.h|tx_record.h' src/mongo/db/modules/eloq/Repository: eloqdata/eloqdoc
Length of output: 230
🏁 Script executed:
#!/bin/bash
# List the structure of data_substrate directory
find src/mongo/db/modules/eloq/data_substrate -type f -name "*.h" | head -20Repository: eloqdata/eloqdoc
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Look at eloq_cursor.h to see what's being declared/used
cat -n src/mongo/db/modules/eloq/src/eloq_cursor.hRepository: eloqdata/eloqdoc
Length of output: 4191
🏁 Script executed:
#!/bin/bash
# Find where tx_execution.h and tx_request.h actually are
find src/mongo/db/modules/eloq -name "tx_execution.h" -o -name "tx_request.h"Repository: eloqdata/eloqdoc
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Also search the entire directory structure for data_substrate
find src/mongo/db/modules/eloq -type d -name data_substrateRepository: eloqdata/eloqdoc
Length of output: 100
🏁 Script executed:
#!/bin/bash
# Explore the data_substrate directory structure
ls -la src/mongo/db/modules/eloq/data_substrate/Repository: eloqdata/eloqdoc
Length of output: 168
🏁 Script executed:
#!/bin/bash
# Find all .h files in data_substrate
find src/mongo/db/modules/eloq/data_substrate -name "*.h"Repository: eloqdata/eloqdoc
Length of output: 42
Remove unused includes <atomic> and <butil/time.h>.
These headers are not used anywhere in eloq_cursor.cpp or eloq_cursor.h. The referenced headers they might depend on (tx_execution.h, tx_request.h) are not present in the repository. Remove both includes.
🤖 Prompt for AI Agents
In @src/mongo/db/modules/eloq/src/eloq_cursor.cpp at line 20, Remove the unused
includes <atomic> and <butil/time.h> from eloq_cursor.cpp (and eloq_cursor.h if
present); locate the top-of-file include lines referencing these headers and
delete them, ensuring no other code depends on symbols from those headers and
leaving only the truly used includes (e.g., existing eloq_cursor.h, tx_* headers
if present).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/mongo/db/modules/eloq/src/eloq_index.cpp (1)
190-211: Verify prefetch state consistency across save/restore.The
save()andrestore()methods don't explicitly manage prefetched records. This appears intentional since:
save()is a no-op, preserving current staterestore()either returns early (if_eof) or re-seeks via_seekCursor()which clears prefetched records- If cursor position is preserved (cursor still open at same position), keeping prefetched data is correct
The design seems sound, but consider whether
saveUnpositioned()should also clear prefetched records for consistency, even thoughrestore()will eventually clear them.♻️ Optional: Clear prefetched records in saveUnpositioned() for consistency
void saveUnpositioned() override { MONGO_LOG(1) << "EloqIndexCursor::saveUnpositioned " << _indexName->StringView(); _cursor.reset(); + _clearPrefetchedRecords(); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/mongo/db/modules/eloq/src/eloq_index.cpp
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 404
File: src/mongo/db/exec/text_or.cpp:261-261
Timestamp: 2026-01-06T06:34:18.890Z
Learning: In the context of batch index row lookup for secondary index scans (eloqdoc repository), WorkingSetMember objects returned for read-only index scans have state RID_AND_OBJ, which includes RID, IDX (keyData), and OBJ, not just RID and OBJ as the state name suggests.
📚 Learning: 2025-11-24T03:11:24.955Z
Learnt from: lzxddz
Repo: eloqdata/eloqdoc PR: 325
File: src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp:1362-1384
Timestamp: 2025-11-24T03:11:24.955Z
Learning: The EloqDoc codebase uses an older version of glog that supports the two-parameter signature `google::InitGoogleLogging(const char* argv0, PrefixCallback)` for custom log prefix formatting, as seen in src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp line 1383.
Applied to files:
src/mongo/db/modules/eloq/src/eloq_index.cpp
🔇 Additional comments (9)
src/mongo/db/modules/eloq/src/eloq_index.cpp (9)
20-28: LGTM!The new includes are appropriate for the batch prefetch functionality -
<atomic>for the atomic server knob andquery_knobs.hforinternalEloqIndexBatchFetchSize.
107-109: LGTM!Correctly clears prefetched records during cursor reset to prevent stale data from persisting.
295-296: LGTM!Appropriately clears prefetched records when a new index scan is opened, ensuring stale data is not used.
315-350: LGTM!The lazy prefetch integration in
_updatePosition()is well-structured:
- Correctly skips prefetch for ID indexes (records already in scan result)
- Correctly skips for upsert operations to avoid lock contention
- Proper error propagation via
uassertStatusOK
352-382: LGTM!The lazy fetch logic correctly:
- Uses batch count to reliably detect when a new batch arrives
- Checks if the current position falls within the already-prefetched range
- Triggers a new fetch only when needed
384-519: LGTM!The batch fetch implementation is well-designed:
- Correctly handles deleted records by leaving nullptr slots
- Maintains proper index mapping between recordIds and batchVector positions
- Creates records directly in
_prefetchedRecordsbeforebatchGetKVto avoid extra copies- Properly clears state on error
- Uses debug-only assertions to verify data consistency invariants
521-568: LGTM!The
_updateRecordPtr()method correctly:
- Uses direct scan tuple record for ID indexes (no prefetch needed)
- Looks up prefetched records for UNIQUE/STANDARD indexes by offset
- Handles upsert case by setting
_recordPtrto nullptr- Uses
MONGO_UNREACHABLEfor invariant violations (should never happen if prefetch logic is correct)
651-661: LGTM!Clean utility methods with reasonable bounds clamping for the batch fetch size.
694-699: LGTM!Member variables are well-designed with appropriate ownership semantics (
unique_ptr) and default initialization.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/mongo/db/modules/eloq/src/eloq_index.cpp (1)
195-199: Clear prefetched records insaveUnpositioned().When
_cursor.reset()is called, the prefetched records become invalid since they are tied to the cursor's batch state. Failing to clear them can lead to incorrect data being returned or undefined behavior on subsequent operations.Proposed fix
void saveUnpositioned() override { MONGO_LOG(1) << "EloqIndexCursor::saveUnpositioned " << _indexName->StringView(); _cursor.reset(); + _clearPrefetchedRecords(); }
🤖 Fix all issues with AI agents
In @src/mongo/db/modules/eloq/src/eloq_index.cpp:
- Around line 522-569: _updateRecordPtr currently uses MONGO_UNREACHABLE when a
prefetched record is missing which crashes the process; replace these
unreachable calls with graceful handling: remove MONGO_UNREACHABLE in the
branches that check _prefetchedRecords[offset] and when currentIndexScanBatchIdx
< _prefetchedBatchStartIdx, set _recordPtr = nullptr (already done in places),
emit a warning log (MONGO_LOG_WARNING or MONGO_LOG(0)) with the same diagnostic
details, and either change _updateRecordPtr to return a boolean/Status
indicating success/failure or leave it void but ensure callers of
_updateRecordPtr (search for all call sites) check _recordPtr for nullptr and
handle the error path; update those callers accordingly so missing prefetched
records no longer abort the process.
- Around line 405-433: The loop building recordIds intentionally leaves deleted
entries as nullptr holes in _prefetchedRecords, so change _updateRecordPtr to
handle those gracefully instead of calling MONGO_UNREACHABLE: in method
_updateRecordPtr, detect when _prefetchedRecords[idx] is nullptr (or when the
corresponding recordIds/recordIdsIdxToBatchIdx entry represents a deleted tuple)
and simply skip updating that cursor entry (or set its record pointer to
nullptr) and continue; remove the fatal assertion, preserve existing handling
for the case where all records are deleted, and add a brief debug log or comment
explaining that nullptr means a deleted index entry.
🧹 Nitpick comments (1)
src/mongo/db/modules/eloq/src/eloq_index.cpp (1)
397-397: Fix stale comment reference.The comment references "line 421" but
_idxis declared at line 667, not 421.Proposed fix
- // Note: _idx is a member variable of EloqIndexCursor (const EloqIndex* _idx, line 421) + // Note: _idx is a member variable of EloqIndexCursor (const EloqIndex* _idx, line 667)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/mongo/db/modules/eloq/data_substratesrc/mongo/db/modules/eloq/src/eloq_index.cpp
✅ Files skipped from review due to trivial changes (1)
- src/mongo/db/modules/eloq/data_substrate
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 404
File: src/mongo/db/exec/text_or.cpp:261-261
Timestamp: 2026-01-06T06:34:18.890Z
Learning: In the context of batch index row lookup for secondary index scans (eloqdoc repository), WorkingSetMember objects returned for read-only index scans have state RID_AND_OBJ, which includes RID, IDX (keyData), and OBJ, not just RID and OBJ as the state name suggests.
📚 Learning: 2025-11-24T03:11:24.955Z
Learnt from: lzxddz
Repo: eloqdata/eloqdoc PR: 325
File: src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp:1362-1384
Timestamp: 2025-11-24T03:11:24.955Z
Learning: The EloqDoc codebase uses an older version of glog that supports the two-parameter signature `google::InitGoogleLogging(const char* argv0, PrefixCallback)` for custom log prefix formatting, as seen in src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp line 1383.
Applied to files:
src/mongo/db/modules/eloq/src/eloq_index.cpp
🔇 Additional comments (8)
src/mongo/db/modules/eloq/src/eloq_index.cpp (8)
108-110: LGTM - Prefetch state properly cleared on reset.Clearing prefetched records on cursor reset ensures stale data doesn't persist across resets.
296-296: LGTM - Prefetch cleared after index scan open.Clearing prefetched records after opening a new index scan ensures the prefetch state aligns with the new scan position.
316-331: LGTM - Lazy fetch correctly integrated into position update.The conditional logic appropriately limits prefetching to STANDARD/UNIQUE indexes (ID indexes already have record data) and skips prefetch for upsert operations to avoid unnecessary locks.
353-383: LGTM - Batch detection and range checking logic is sound.The use of batch count to detect new batches is more reliable than size-based detection. The range checking correctly determines when records need to be fetched.
488-488: LGTM - Good defensive checks.The invariant at line 488 ensures prefetch is never used for write operations, and the debug verification (lines 502-512) confirms all fetched records are valid.
Also applies to: 502-512
652-662: LGTM - Helper methods are correct.
_clearPrefetchedRecordsproperly resets all prefetch state, and_getBatchFetchSizeapplies reasonable bounds to the server parameter.
696-700: LGTM - Member variables appropriately declared.Using
unique_ptrfor_prefetchedRecordsensures proper memory management, and the batch tracking variables support the batch detection logic.
214-226: Verify prefetch validity across detach/reattach cycles.Prefetched records are not cleared when detaching/reattaching the cursor. While
restore()will re-seek (and clear prefetch) if the cursor is closed, prefetch state persists if the cursor remains open. Confirm this is the intended behavior and that prefetched records remain valid when the cursor stays open across detach/reattach boundaries.
| for (size_t i = startIdx; i < endIdx; ++i) { | ||
| const auto& tuple = batchVector[i]; | ||
| // Skip Deleted records - they create holes in prefetchRecords (nullptr entries) | ||
| if (tuple.status_ == txservice::RecordStatus::Deleted) { | ||
| continue; | ||
| } | ||
|
|
||
| // Only extract RecordId for Normal status tuples | ||
| invariant(tuple.status_ == txservice::RecordStatus::Normal); | ||
| RecordId id; | ||
| if (_indexType == IndexCursorType::UNIQUE) { | ||
| // For UNIQUE indexes, RecordId is stored in the record data, not in the key | ||
| const Eloq::MongoRecord* record = | ||
| static_cast<const Eloq::MongoRecord*>(tuple.record_); | ||
| invariant(record != nullptr); | ||
| id = record->ToRecordId(false); | ||
| } else { | ||
| // For STANDARD indexes, RecordId is appended to the key | ||
| const Eloq::MongoKey* key = tuple.key_.GetKey<Eloq::MongoKey>(); | ||
| invariant(key != nullptr); | ||
| KeyString ks(_idx->keyStringVersion()); | ||
| ks.resetFromBuffer(key->Data(), key->Size()); | ||
| id = KeyString::decodeRecordIdStrAtEnd(ks.getBuffer(), ks.getSize()); | ||
| } | ||
|
|
||
| recordIdsIdxToBatchIdx.push_back( | ||
| i); // Store batchVector index for this recordIds entry | ||
| recordIds.push_back(id); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n 'RecordStatus::Deleted' --type=cpp -C 3 -g '!*test*'Repository: eloqdata/eloqdoc
Length of output: 1879
🏁 Script executed:
sed -n '440,465p' src/mongo/db/modules/eloq/src/eloq_index.cppRepository: eloqdata/eloqdoc
Length of output: 1364
🏁 Script executed:
sed -n '535,560p' src/mongo/db/modules/eloq/src/eloq_index.cppRepository: eloqdata/eloqdoc
Length of output: 1822
🏁 Script executed:
rg -n 'getCurrentBatchTupleIdx\|batchVector' src/mongo/db/modules/eloq/src/eloq_index.cpp -B 2 -A 2 | head -60Repository: eloqdata/eloqdoc
Length of output: 42
🏁 Script executed:
# Check if cursor filters deleted records before returning
rg -n '_cursor\->.*batch\|fetchTuples' src/mongo/db/modules/eloq/src/eloq_index.cpp -C 3 | head -80Repository: eloqdata/eloqdoc
Length of output: 42
🏁 Script executed:
sed -n '380,440p' src/mongo/db/modules/eloq/src/eloq_index.cppRepository: eloqdata/eloqdoc
Length of output: 3247
🏁 Script executed:
sed -n '465,520p' src/mongo/db/modules/eloq/src/eloq_index.cppRepository: eloqdata/eloqdoc
Length of output: 2808
Address potential nullptr dereference for deleted index records.
The code skips deleted records when building recordIds (lines 408-410), intentionally creating nullptr holes in _prefetchedRecords at deleted record positions. However, _updateRecordPtr (lines 547-552) treats accessing these nullptr entries as an error and calls MONGO_UNREACHABLE, which crashes the process.
This creates a logical inconsistency: the code explicitly acknowledges that deleted records appear in scan results and handles the case where all records are deleted (line 530-531), yet it fatally crashes if the cursor subsequently returns an index pointing to a deleted record position.
Clarify the expected behavior:
- Should deleted records be filtered from cursor results before reaching
_updateRecordPtr, ensuring indices never point to deleted positions? - Or should deleted records be gracefully skipped during record pointer updates instead of crashing?
Without this clarification, deleted tuples in index scan results risk fatal process termination.
🤖 Prompt for AI Agents
In @src/mongo/db/modules/eloq/src/eloq_index.cpp around lines 405 - 433, The
loop building recordIds intentionally leaves deleted entries as nullptr holes in
_prefetchedRecords, so change _updateRecordPtr to handle those gracefully
instead of calling MONGO_UNREACHABLE: in method _updateRecordPtr, detect when
_prefetchedRecords[idx] is nullptr (or when the corresponding
recordIds/recordIdsIdxToBatchIdx entry represents a deleted tuple) and simply
skip updating that cursor entry (or set its record pointer to nullptr) and
continue; remove the fatal assertion, preserve existing handling for the case
where all records are deleted, and add a brief debug log or comment explaining
that nullptr means a deleted index entry.
| void _updateRecordPtr() { | ||
| MONGO_LOG(1) << "EloqIndexCursor::_updateRecordPtr " << _indexName->StringView(); | ||
|
|
||
| switch (_indexType) { | ||
| case IndexCursorType::ID: { | ||
| _recordPtr = _scanTupleRecord; | ||
| } break; | ||
| case IndexCursorType::UNIQUE: | ||
| case IndexCursorType::STANDARD: { | ||
| // We dont have prefetch records for upsert operations | ||
| if (!_opCtx->isUpsert()) { | ||
| // Look up prefetched record by index | ||
| // Get current scan batch index directly from EloqCursor (no need to search) | ||
| // The corresponding index of record vector is scanBatchIdx - | ||
| // _prefetchedBatchStartIdx | ||
| size_t currentIndexScanBatchIdx = _cursor->getCurrentBatchTupleIdx(); | ||
|
|
||
| if (currentIndexScanBatchIdx >= _prefetchedBatchStartIdx) { | ||
| size_t offset = currentIndexScanBatchIdx - _prefetchedBatchStartIdx; | ||
| if (offset < _prefetchedRecords.size() && | ||
| _prefetchedRecords[offset] != nullptr) { | ||
| _recordPtr = _prefetchedRecords[offset].get(); // Use prefetched record | ||
| MONGO_LOG(1) << "found. id: " << _id.toString() << ". record:" | ||
| << BSONObj{_recordPtr->EncodedBlobData()}.jsonString(); | ||
| } else { | ||
| _recordPtr = | ||
| nullptr; // Record not fetched (error case or out of range) | ||
| MONGO_LOG(0) << "RecordId not found in prefetched records at offset " | ||
| << offset << " (scan index " << currentIndexScanBatchIdx | ||
| << ") for index " << _indexName->StringView(); | ||
| MONGO_UNREACHABLE; | ||
| } | ||
| } else { | ||
| _recordPtr = nullptr; // Current index before prefetched range | ||
| MONGO_LOG(0) << "Current scan index " << currentIndexScanBatchIdx | ||
| << " is before prefetched range starting at " | ||
| << _prefetchedBatchStartIdx; | ||
| MONGO_UNREACHABLE; | ||
| } | ||
| } else { | ||
| // For upsert operations, we dont prefetch records | ||
| _recordPtr = nullptr; | ||
| } | ||
| } break; | ||
| default: | ||
| MONGO_UNREACHABLE; | ||
| } | ||
| } |
There was a problem hiding this comment.
Replace MONGO_UNREACHABLE with graceful error handling.
Lines 549-553 and 556-559 use MONGO_UNREACHABLE when prefetched records are unavailable, causing the process to crash. This is too strict—scenarios like deleted records (nullptr in _prefetchedRecords) or race conditions could trigger this path.
Instead of crashing, return an error status or set _recordPtr to nullptr and let the caller handle it gracefully.
Proposed fix to handle missing records gracefully
if (offset < _prefetchedRecords.size() &&
_prefetchedRecords[offset] != nullptr) {
_recordPtr = _prefetchedRecords[offset].get(); // Use prefetched record
MONGO_LOG(1) << "found. id: " << _id.toString() << ". record:"
<< BSONObj{_recordPtr->EncodedBlobData()}.jsonString();
} else {
- _recordPtr =
- nullptr; // Record not fetched (error case or out of range)
- MONGO_LOG(0) << "RecordId not found in prefetched records at offset "
- << offset << " (scan index " << currentIndexScanBatchIdx
- << ") for index " << _indexName->StringView();
- MONGO_UNREACHABLE;
+ // Record not fetched (deleted or out of range) - handle gracefully
+ _recordPtr = nullptr;
+ MONGO_LOG(1) << "RecordId not found in prefetched records at offset "
+ << offset << " (scan index " << currentIndexScanBatchIdx
+ << ") for index " << _indexName->StringView()
+ << " - possibly deleted or out of range";
}
} else {
- _recordPtr = nullptr; // Current index before prefetched range
- MONGO_LOG(0) << "Current scan index " << currentIndexScanBatchIdx
- << " is before prefetched range starting at "
- << _prefetchedBatchStartIdx;
- MONGO_UNREACHABLE;
+ // Current index before prefetched range - handle gracefully
+ _recordPtr = nullptr;
+ MONGO_LOG(1) << "Current scan index " << currentIndexScanBatchIdx
+ << " is before prefetched range starting at "
+ << _prefetchedBatchStartIdx;
}🤖 Prompt for AI Agents
In @src/mongo/db/modules/eloq/src/eloq_index.cpp around lines 522 - 569,
_updateRecordPtr currently uses MONGO_UNREACHABLE when a prefetched record is
missing which crashes the process; replace these unreachable calls with graceful
handling: remove MONGO_UNREACHABLE in the branches that check
_prefetchedRecords[offset] and when currentIndexScanBatchIdx <
_prefetchedBatchStartIdx, set _recordPtr = nullptr (already done in places),
emit a warning log (MONGO_LOG_WARNING or MONGO_LOG(0)) with the same diagnostic
details, and either change _updateRecordPtr to return a boolean/Status
indicating success/failure or leave it void but ensure callers of
_updateRecordPtr (search for all call sites) check _recordPtr for nullptr and
handle the error path; update those callers accordingly so missing prefetched
records no longer abort the process.
Summary by CodeRabbit
New Features
internalEloqIndexBatchFetchSizeserver parameter (default: 100) to control batch fetch size for secondary index operations.Tests
✏️ Tip: You can customize this high-level summary in your review settings.