Support TTL Feature#361
Conversation
WalkthroughEnable runtime TTL monitoring in manifests; refactor TTL monitor to enumerate databases/collections via the storage engine and perform per-index, batched deletions with error-tolerant logging; add a limits parameter to InternalPlanner delete APIs; pass in-memory docs to deleteDocument; add ELOQ cursor/recovery logging and JS tests for TTL and deletes. Changes
Sequence Diagram(s)sequenceDiagram
participant TTLMon as TTL Monitor
participant Stg as Storage Engine
participant DB as Database
participant Coll as Collection
participant Planner as InternalPlanner
participant WUOW as WriteUnitOfWork
Note over TTLMon,Stg: Enumeration-based batched TTL deletion
TTLMon->>Stg: listDatabases()
Stg-->>TTLMon: databases
loop per database
TTLMon->>Stg: enumerateCollections(db)
Stg-->>TTLMon: collections (with TTL indexes)
end
loop per TTL index
TTLMon->>WUOW: begin per-index WriteUnitOfWork
TTLMon->>Planner: deleteWithIndexScan(..., limits)
Planner->>Coll: scan index (LimitStage up to limits) and delete
Coll-->>Planner: deletedCount
Planner-->>TTLMon: deletedCount
WUOW->>TTLMon: commit
alt deletedCount == limits
TTLMon->>Planner: repeat same index (batched)
end
alt DBException occurs
TTLMon->>TTLMon: log error and continue
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/ttl.cpp (1)
152-172: UncommittedWriteUnitOfWorkin read-only loop.The
WriteUnitOfWorkat line 153 is created but never committed before the loop continues or ends. If WUOW is required for the read operations (getting index specs), it should be committed. If it's not required for read-only operations, it should be removed to avoid confusion and potential issues.for (const std::string& collectionNS : collections) { - WriteUnitOfWork wuow(&opCtx); UninterruptibleLockGuard noInterrupt(opCtx.lockState()); NamespaceString collectionNSS(collectionNS); AutoGetCollection autoGetCollection(&opCtx, collectionNSS, MODE_IS); Collection* coll = autoGetCollection.getCollection(); if (!coll) { // Skip since collection has been dropped. continue; } CollectionCatalogEntry* collEntry = coll->getCatalogEntry(); std::vector<std::string> indexNames; collEntry->getAllIndexes(&opCtx, &indexNames); for (const std::string& name : indexNames) { BSONObj spec = collEntry->getIndexSpec(&opCtx, name); if (spec.hasField(secondsExpireField)) { ttlIndexes.push_back(spec.getOwned()); } } }
🧹 Nitpick comments (2)
tests/jstests/eloq_basic/ttl_basic_test.js (2)
84-102: Robust expiration verification with appropriate timeout.The polling loop correctly waits for TTL deletions with a 60-second safety timeout (adequate for the 10-second maximum TTL). The assertion properly verifies that all documents are deleted.
Minor style note: Line 102 is missing a semicolon for consistency with the rest of the file, though JavaScript's automatic semicolon insertion makes this work.
108-109: Parameter restoration included.The test restores the original
ttlMonitorSleepSecsvalue, which is good practice.As an optional improvement, consider wrapping the test in a try-finally block to ensure parameter restoration even if the test fails mid-execution. However, this may not be critical in a test environment where processes are typically isolated.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
concourse/artifact/ELOQDSS_ELOQSTORE/eloqdoc.conf(1 hunks)concourse/artifact/ELOQDSS_ROCKSDB/eloqdoc.conf(1 hunks)concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/eloqdoc.conf(1 hunks)concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/eloqdoc_cluster_a.conf(1 hunks)concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/eloqdoc_cluster_b.conf(1 hunks)concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/eloqdoc_cluster_c.conf(1 hunks)concourse/scripts/common.sh(2 hunks)concourse/scripts/eloqdoc.yaml(1 hunks)src/mongo/db/ttl.cpp(2 hunks)src/mongo/shell/collection.js(1 hunks)tests/jstests/eloq_basic/ttl_basic_test.js(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 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:
concourse/scripts/common.shconcourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/eloqdoc.conf
📚 Learning: 2025-11-28T10:52:25.527Z
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 279
File: concourse/scripts/main.ent.bash:38-39
Timestamp: 2025-11-28T10:52:25.527Z
Learning: In the eloqdoc enterprise build (main.ent.bash), eloq_log_service is fetched separately as the resource `eloq_logservice_src` in the pipeline (main.ent.yml) and then symlinked into the data_substrate directory structure at build time using `ln -s $WORKSPACE/eloq_logservice_src eloq_log_service`. This is the intended architecture for placing eloq_log_service within data_substrate in the enterprise configuration.
Applied to files:
concourse/scripts/common.sh
🧬 Code graph analysis (1)
src/mongo/db/ttl.cpp (1)
tests/jstests/eloq_basic/ttl_basic_test.js (1)
dbName(20-20)
🔇 Additional comments (15)
src/mongo/shell/collection.js (1)
1479-1482: LGTM! Proper guard against empty aggregation results.This defensive check correctly handles the case when no documents match the query. Without this guard, accessing
res[0].non an empty array would returnundefinedrather than the expected0.src/mongo/db/ttl.cpp (2)
174-184: LGTM! Proper transaction handling for TTL deletions.Each TTL index operation is correctly wrapped in its own
WriteUnitOfWorkwith explicit commit. The error handling appropriately continues processing other indexes when one fails.
132-145: Acceptable tradeoff: reliability over efficiency.The refactor to enumerate all collections via the storage engine instead of using
TTLCollectionCacheis reasonable given the stated reliability concern. Note that this approach iterates all collections (not just TTL-enabled ones), which may be less efficient if there are many non-TTL collections, but correctness/reliability is more important for TTL cleanup.concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/eloqdoc_cluster_c.conf (1)
24-24: LGTM! TTL monitor enabled for cluster node C.Configuration change aligns with the PR objective to enable TTL monitoring across cluster configurations.
concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/eloqdoc_cluster_a.conf (1)
24-24: LGTM! TTL monitor enabled for cluster node A.Configuration change aligns with the PR objective to enable TTL monitoring across cluster configurations.
concourse/scripts/common.sh (2)
223-223: LGTM! Consistent configuration path update.Both
launch_eloqdocandlaunch_eloqdoc_fastnow use the sameeloqdoc.yamlconfiguration file, maintaining consistency between the launch methods.
199-199: Configuration fileeloqdoc.yamlexists and is properly configured.The
eloqdoc.yamlfile is present at./concourse/scripts/eloqdoc.yamland contains the expected TTL monitoring configuration withttlMonitorEnabled: truein the setParameter section. The configuration change is valid.concourse/artifact/ELOQDSS_ROCKSDB/eloqdoc.conf (1)
24-24: LGTM! TTL monitoring enabled for ROCKSDB configuration.This change enables TTL monitoring as intended by the PR. The configuration change is straightforward and consistent with the broader TTL feature rollout.
concourse/artifact/ELOQDSS_ELOQSTORE/eloqdoc.conf (1)
57-57: LGTM! TTL monitoring enabled for ELOQSTORE configuration.Consistent with other environment configurations in this PR.
concourse/scripts/eloqdoc.yaml (1)
23-23: LGTM! TTL monitoring enabled for script configuration.Aligns with the TTL feature enablement across all environments.
concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/eloqdoc_cluster_b.conf (1)
24-24: LGTM! TTL monitoring enabled for cluster B.Consistent enablement across cluster configurations.
concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/eloqdoc.conf (1)
24-24: LGTM! TTL monitoring enabled for ROCKSDB_CLOUD_S3 configuration.Final configuration update completing the TTL feature rollout across all environments.
tests/jstests/eloq_basic/ttl_basic_test.js (3)
5-14: Well-structured helper functions.The parameter access helpers provide clean abstractions with appropriate error handling for the admin commands.
20-53: Excellent test setup with proper configuration management.The test correctly:
- Defines clear test parameters (1000 docs, 1-10s TTL range)
- Saves and modifies
ttlMonitorSleepSecsfor faster test execution- Creates a TTL index with
expireAfterSeconds: 0, ensuring documents expire at theirexpireAttimestamp
59-76: Correct document insertion with randomized TTL.The bulk insert efficiently creates 1000 documents with properly calculated expiration timestamps. The random TTL distribution (1-10 seconds) provides good test coverage.
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/ttl.cpp (1)
152-161: WriteUnitOfWork created but never committed in collection enumeration loop.The
WriteUnitOfWork wuow(&opCtx)on line 153 is created but never committed before going out of scope. Since this section only performs read operations (reading index specs), theWriteUnitOfWorkappears unnecessary here and will be implicitly aborted.for (const std::string& collectionNS : collections) { - WriteUnitOfWork wuow(&opCtx); UninterruptibleLockGuard noInterrupt(opCtx.lockState()); NamespaceString collectionNSS(collectionNS); AutoGetCollection autoGetCollection(&opCtx, collectionNSS, MODE_IS);
🧹 Nitpick comments (2)
tests/jstests/eloq_basic/ttl_basic_test.js (1)
88-102: Original TTL parameter may not be restored if assertion fails.If
assert.eq(0, remaining, ...)on line 102 fails, thesetParameterValuecall on line 109 is never executed. Consider wrapping the test body in a try-finally block to ensure cleanup.+try { print("Waiting for TTL deletions..."); let remaining = docCount; let elapsed = 0; while (remaining > 0) { sleep(1000); elapsed++; remaining = dbTest[collName].countDocuments({}); print(`Elapsed ${elapsed}s, remaining documents: ${remaining}`); // Safety timeout: 60 sec if (elapsed > 60) { print("Timeout reached. Some documents may not have expired."); break; } } assert.eq(0, remaining, "All documents expired and deleted.") +} finally { + // Restore original TTL parameter + print(`Restoring ttlMonitorSleepSecs to ${originalTTL}...`); + setParameterValue("ttlMonitorSleepSecs", originalTTL); +} -// ----------------------------- -// Restore original TTL parameter -// ----------------------------- - -print(`Restoring ttlMonitorSleepSecs to ${originalTTL}...`); -setParameterValue("ttlMonitorSleepSecs", originalTTL); print("Test completed.");src/mongo/db/ttl.cpp (1)
176-188: Potential race condition when comparing global counter values.The batched deletion loop reads
ttlDeletedDocumentsbefore and after each batch to determine if more work is needed. SincettlDeletedDocumentsis a global counter that could theoretically be modified by other operations, this comparison could be affected. In practice, only one TTL monitor should be running, but consider using the return value fromDeleteStage::getNumDeleted(*exec)directly instead of relying on the global counter.Additionally,
LOG(0)on every batch could be verbose for collections with many expired documents.try { - long long before; const int limits = 1000; + long long numDeleted; do { - before = ttlDeletedDocuments; auto begin_time = Date_t::now(); WriteUnitOfWork wuow(&opCtx); - doTTLForIndex(&opCtx, idx, limits); + numDeleted = doTTLForIndex(&opCtx, idx, limits); wuow.commit(); auto end_time = Date_t::now(); - LOG(0) << "TTL pass deleted " << (ttlDeletedDocuments - before) + LOG(1) << "TTL batch deleted " << numDeleted << " documents from index " << idx << " in " << (end_time - before).count() << " ms"; - } while (ttlDeletedDocuments - before >= limits); + } while (numDeleted >= limits);This would also require changing
doTTLForIndexto return the number of deleted documents (numDeleted) instead ofvoid.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
concourse/artifact/ELOQDSS_ELOQSTORE/eloqdoc.conf(1 hunks)concourse/artifact/ELOQDSS_ROCKSDB/eloqdoc.conf(1 hunks)concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/eloqdoc.conf(1 hunks)concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/eloqdoc_cluster_a.conf(1 hunks)concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/eloqdoc_cluster_b.conf(1 hunks)concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/eloqdoc_cluster_c.conf(1 hunks)concourse/scripts/common.sh(2 hunks)concourse/scripts/eloqdoc.yaml(1 hunks)src/mongo/db/modules/eloq/src/eloq_index.cpp(1 hunks)src/mongo/db/modules/eloq/src/eloq_recovery_unit.cpp(1 hunks)src/mongo/db/query/internal_plans.cpp(3 hunks)src/mongo/db/query/internal_plans.h(1 hunks)src/mongo/db/ttl.cpp(4 hunks)tests/jstests/eloq_basic/ttl_basic_test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/eloqdoc_cluster_b.conf
- concourse/scripts/common.sh
- concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/eloqdoc.conf
- concourse/artifact/ELOQDSS_ROCKSDB/eloqdoc.conf
- concourse/artifact/ELOQDSS_ELOQSTORE/eloqdoc.conf
🧰 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_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_recovery_unit.cpp
🧬 Code graph analysis (2)
src/mongo/db/modules/eloq/src/eloq_index.cpp (1)
src/mongo/db/modules/eloq/src/eloq_record_store.cpp (6)
MONGO_LOG(88-116)MONGO_LOG(440-474)MONGO_LOG(514-518)MONGO_LOG(520-526)MONGO_LOG(528-534)MONGO_LOG(536-541)
src/mongo/db/query/internal_plans.cpp (1)
src/mongo/db/ttl.cpp (2)
opCtx(201-316)opCtx(201-201)
🔇 Additional comments (12)
concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/eloqdoc_cluster_c.conf (1)
24-24: LGTM!TTL monitor enablement is consistent with the PR objective and aligned with similar changes in other cluster configuration files.
src/mongo/db/modules/eloq/src/eloq_recovery_unit.cpp (1)
295-303: LGTM!Debug logging additions for cursor registration/unregistration are consistent with the existing logging patterns in this file and improve observability for cursor lifecycle debugging.
concourse/scripts/eloqdoc.yaml (1)
23-23: LGTM!TTL monitor enablement in the CI configuration is consistent with the feature rollout across other configuration files.
concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/eloqdoc_cluster_a.conf (1)
24-24: LGTM!TTL monitor enablement is consistent with the PR objective and aligned with similar changes in other cluster configuration files.
src/mongo/db/modules/eloq/src/eloq_index.cpp (2)
188-190: Verify intentional removal of cursor reset insave().The
save()method no longer resets_cursor, unlikeEloqRecordStoreCursor::save()which still calls_cursor.reset(). This behavioral difference may be intentional for TTL batch processing optimization, but creates an inconsistency between cursor implementations.Ensure this is the intended behavior and document why
EloqIndexCursorandEloqRecordStoreCursorhave different save/restore semantics.
198-209: Verify cursor validity assumption inrestore().The new logic skips re-seeking if the cursor is already open (
_cursor && _cursor->indexScanIsOpen()). This optimization assumes the cursor position remains valid betweensave()andrestore().A few considerations:
- If documents are inserted/deleted in the cursor's range between save/restore, the cursor may be positioned incorrectly or skip/duplicate records.
- The removed
assert(!_cursor)was previously a contract guarantee. Ensure callers don't rely on cursor re-initialization behavior.- The enhanced log output with
_eofstate is helpful for debugging.If the TTL feature guarantees cursor stability during batch operations, this is acceptable.
tests/jstests/eloq_basic/ttl_basic_test.js (1)
5-14: LGTM! Helper functions are well-structured.The helper functions properly wrap admin commands and include error handling with descriptive messages.
src/mongo/db/query/internal_plans.h (1)
115-116: LGTM! Backwards-compatible API extension.The new
limitsparameter with default value0maintains backwards compatibility with existing callers while enabling batched deletions when a limit is specified.src/mongo/db/query/internal_plans.cpp (2)
41-41: LGTM! Required include for LimitStage.The include is necessary for the new
LimitStageusage below.
148-150: LGTM! Correct LimitStage insertion in the plan tree.The
LimitStageis properly inserted between the fetch stage and delete stage whenlimits > 0, allowing batched deletions. The stage hierarchy will be:IXSCAN -> FETCH -> LIMIT -> DELETE.src/mongo/db/ttl.cpp (2)
132-145: LGTM! Direct storage engine enumeration replaces TTLCollectionCache.The comment explains the rationale ("TTLCollectionCache is unreliable"), and the new approach correctly enumerates all databases and collections from the storage engine. The use of
std::make_move_iteratoris efficient for building the combined collection list.
201-201: LGTM! Updated doTTLForIndex signature and call to deleteWithIndexScan.The
limitsparameter is correctly propagated through toInternalPlanner::deleteWithIndexScan, enabling batched TTL deletions.Also applies to: 303-304
There was a problem hiding this comment.
Actionable comments posted: 1
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/ttl.cpp (1)
152-172: Remove unnecessary WriteUnitOfWork for read operations.A
WriteUnitOfWorkis created for reading index specifications, but this segment only performs read operations (getAllIndexes,getIndexSpec). Write units of work are intended for write operations and are never needed for reads. Additionally, the WUOW is never committed, which would be incorrect if writes were actually performed.Apply this diff to remove the unnecessary WriteUnitOfWork:
// Get all TTL indexes from every collection. for (const std::string& collectionNS : collections) { - WriteUnitOfWork wuow(&opCtx); UninterruptibleLockGuard noInterrupt(opCtx.lockState()); NamespaceString collectionNSS(collectionNS); AutoGetCollection autoGetCollection(&opCtx, collectionNSS, MODE_IS);
🧹 Nitpick comments (2)
tests/jstests/eloq_basic/ttl_basic_test.js (2)
5-14: Improve error message with original response details.The error messages don't include the original response, which would help debugging when parameter operations fail.
function getParameterValue(paramName) { const res = db.adminCommand({ getParameter: 1, [paramName]: 1 }); - if (!res.ok) throw new Error("Failed to get parameter: " + paramName); + if (!res.ok) throw new Error("Failed to get parameter: " + paramName + ", error: " + tojson(res)); return res[paramName]; } function setParameterValue(paramName, value) { const res = db.adminCommand({ setParameter: 1, [paramName]: value }); - if (!res.ok) throw new Error("Failed to set parameter: " + paramName); + if (!res.ok) throw new Error("Failed to set parameter: " + paramName + ", error: " + tojson(res)); }
38-109: Consider wrapping test in try/finally for cleanup robustness.If the test fails (e.g., assertion at line 102 or any intermediate error), the
ttlMonitorSleepSecsparameter won't be restored to its original value. This could affect subsequent tests.Consider wrapping the core test logic:
try { // ... test logic from lines 42-102 ... } finally { print(`Restoring ttlMonitorSleepSecs to ${originalTTL}...`); setParameterValue("ttlMonitorSleepSecs", originalTTL); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
concourse/artifact/ELOQDSS_ELOQSTORE/eloqdoc.conf(1 hunks)concourse/artifact/ELOQDSS_ROCKSDB/eloqdoc.conf(1 hunks)concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/eloqdoc.conf(1 hunks)concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/eloqdoc_cluster_a.conf(1 hunks)concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/eloqdoc_cluster_b.conf(1 hunks)concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/eloqdoc_cluster_c.conf(1 hunks)concourse/scripts/common.sh(2 hunks)concourse/scripts/eloqdoc.yaml(1 hunks)src/mongo/db/modules/eloq/src/eloq_index.cpp(1 hunks)src/mongo/db/modules/eloq/src/eloq_recovery_unit.cpp(1 hunks)src/mongo/db/query/internal_plans.cpp(3 hunks)src/mongo/db/query/internal_plans.h(1 hunks)src/mongo/db/ttl.cpp(4 hunks)src/mongo/shell/collection.js(1 hunks)tests/jstests/eloq_basic/ttl_basic_test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- concourse/artifact/ELOQDSS_ROCKSDB/eloqdoc.conf
- src/mongo/db/modules/eloq/src/eloq_recovery_unit.cpp
- concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/eloqdoc_cluster_b.conf
- concourse/scripts/eloqdoc.yaml
- concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/eloqdoc_cluster_a.conf
- src/mongo/shell/collection.js
- concourse/scripts/common.sh
- concourse/artifact/ELOQDSS_ELOQSTORE/eloqdoc.conf
🧰 Additional context used
🧠 Learnings (2)
📚 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:
concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/eloqdoc.conf
📚 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:
concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/eloqdoc.conf
🧬 Code graph analysis (1)
src/mongo/db/query/internal_plans.cpp (1)
src/mongo/db/ttl.cpp (2)
opCtx(201-316)opCtx(201-201)
🔇 Additional comments (12)
concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/eloqdoc_cluster_c.conf (1)
24-24: TTL monitoring enabled consistently across all cluster configurations.The configuration change correctly enables TTL monitoring on line 24 across all cluster configuration files (eloqdoc.conf, eloqdoc_cluster_a.conf, eloqdoc_cluster_b.conf, eloqdoc_cluster_c.conf). Comprehensive test coverage exists in tests/jstests/eloq_basic/ttl_basic_test.js with functional validation of TTL document expiration and index management. The Eloq storage engine provides full TTL support with configurable monitoring intervals.
concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/eloqdoc.conf (1)
24-24: LGTM!Enabling
ttlMonitorEnabledaligns with the PR objective to support TTL functionality. The AI summary confirms similar changes across other configuration files for consistency.src/mongo/db/modules/eloq/src/eloq_index.cpp (2)
188-190: Verify cursor state preservation across save/restore cycle.The
save()method no longer resets the cursor (previously_cursor.reset()was called). This change relies onrestore()to handle the cursor appropriately. Ensure the cursor state remains valid across operations that may occur betweensave()andrestore(), particularly in yield scenarios where transaction state could change.
198-209: LGTM with verification note.The optimized restore logic avoids unnecessary cursor re-seeks when an open scan already exists. The conditional
!_cursor || !_cursor->indexScanIsOpen()properly handles both missing and closed cursor scenarios.Ensure that when the cursor scan is open, the internal
_keyposition is synchronized with the cursor's current position, so subsequentnext()calls continue from the correct point.tests/jstests/eloq_basic/ttl_basic_test.js (1)
88-102: LGTM - Good timeout handling and assertion.The polling loop with a 60-second safety timeout is appropriate for TTL tests where timing can vary. The final assertion correctly validates that all documents were deleted.
src/mongo/db/query/internal_plans.h (1)
115-116: LGTM!The new
limitsparameter with default value0maintains backward compatibility while enabling bounded deletions for TTL processing. The default of0(no limit) preserves existing behavior for all current callers.src/mongo/db/query/internal_plans.cpp (2)
41-41: LGTM!Required include for the new
LimitStageusage.
134-150: LGTM - Correct stage pipeline construction.The implementation correctly wraps the index scan with a
LimitStagebefore theDeleteStagewhenlimits > 0. This ensures bounded deletions for TTL batching.The stage pipeline order (IndexScan → Fetch → Limit → Delete) is correct for limiting the number of documents deleted per execution.
src/mongo/db/ttl.cpp (4)
176-188: Well-designed batching implementation.The per-index batching approach with fixed-size batches (1000 documents) and per-batch transactions is a solid design that:
- Limits transaction size to prevent long-running transactions
- Provides incremental progress with timing per batch
- Correctly terminates when fewer than
limitsdocuments are deleted
201-201: Signature change correctly supports batching.The addition of the
limitsparameter properly extends the API to support the new batching behavior.
303-304: The InternalPlanner API correctly accepts the limits parameter.The
deleteWithIndexScansignature in internal_plans.h (line 116) declaresint limits = 0as the final parameter, and the implementation matches. The call at lines 303-304 correctly passeslimitsas the final argument.
137-145: The review comment is incorrect. Based on the code structure and naming conventions,listCollectionsreturns fully qualified namespace strings in the formatdbName.collectionName, not bare collection names. The variable in ttl.cpp is namedcollectionNS(indicating namespace), the method isgetCollectionNamespaces(), and collections are stored in the internal catalog with their full namespace as keys. These strings are directly passed to theNamespaceStringconstructor without transformation, confirming they are already fully qualified. No namespace collision risk exists.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/mongo/db/query/internal_plans.h (1)
115-116: Consider parameter naming and type safety.The
limitsparameter enables batched deletions, which is good for TTL processing. Two minor suggestions:
Naming: Consider using
limit(singular) instead oflimitsto align with common conventions (e.g., SQLLIMIT, MongoDB'slimit()method).Type: Using
intallows negative values. Considersize_tor document that0means "no limit" and negative values are treated equivalently.These are optional refinements; the current implementation is functional and backward-compatible with the default value of
0.🔎 Suggested naming change (optional):
static std::unique_ptr<PlanExecutor, PlanExecutor::Deleter> deleteWithIndexScan( OperationContext* opCtx, Collection* collection, const DeleteStageParams& params, const IndexDescriptor* descriptor, const BSONObj& startKey, const BSONObj& endKey, BoundInclusion boundInclusion, PlanExecutor::YieldPolicy yieldPolicy, Direction direction = FORWARD, - int limits = 0); + size_t limit = 0);src/mongo/db/ttl.cpp (2)
152-154: Remove unnecessaryWriteUnitOfWorkfrom read-only section.The
WriteUnitOfWorkon line 153 wraps a read-only code path that only queries index specifications. This creates unnecessary transaction overhead for each collection during index discovery.🔎 Apply this diff to remove the unnecessary transaction:
for (const std::string& collectionNS : collections) { - WriteUnitOfWork wuow(&opCtx); UninterruptibleLockGuard noInterrupt(opCtx.lockState()); NamespaceString collectionNSS(collectionNS);
176-188: Consider making the batch size configurable.The hardcoded batch size of 1000 on line 177 could benefit from being a configurable server parameter to allow tuning based on workload characteristics.
The batching logic itself is correct: it processes TTL deletions in chunks and continues until fewer than
limitsdocuments are deleted. The use ofWriteUnitOfWorkper batch and the timing metrics are appropriate.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
concourse/artifact/ELOQDSS_ELOQSTORE/eloqdoc.conf(1 hunks)concourse/artifact/ELOQDSS_ROCKSDB/eloqdoc.conf(1 hunks)concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/eloqdoc.conf(1 hunks)concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/eloqdoc_cluster_a.conf(1 hunks)concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/eloqdoc_cluster_b.conf(1 hunks)concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/eloqdoc_cluster_c.conf(1 hunks)concourse/scripts/common.sh(2 hunks)concourse/scripts/eloqdoc.yaml(1 hunks)src/mongo/db/modules/eloq/src/eloq_index.cpp(1 hunks)src/mongo/db/modules/eloq/src/eloq_recovery_unit.cpp(1 hunks)src/mongo/db/query/internal_plans.cpp(3 hunks)src/mongo/db/query/internal_plans.h(1 hunks)src/mongo/db/ttl.cpp(4 hunks)tests/jstests/eloq_basic/ttl_basic_test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- concourse/scripts/common.sh
- concourse/artifact/ELOQDSS_ROCKSDB/eloqdoc.conf
- concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/eloqdoc_cluster_c.conf
- tests/jstests/eloq_basic/ttl_basic_test.js
- concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/eloqdoc_cluster_a.conf
- concourse/artifact/ELOQDSS_ELOQSTORE/eloqdoc.conf
- src/mongo/db/modules/eloq/src/eloq_recovery_unit.cpp
🧰 Additional context used
🧠 Learnings (3)
📚 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:
concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/eloqdoc.confconcourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/eloqdoc_cluster_b.conf
📚 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:
concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/eloqdoc.confsrc/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/query/internal_plans.cpp (1)
src/mongo/db/ttl.cpp (2)
opCtx(201-316)opCtx(201-201)
🔇 Additional comments (10)
concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/eloqdoc.conf (1)
21-24: LGTM!Enabling TTL monitoring is consistent with the PR objective. The configuration change aligns with similar updates across other configuration files in this PR.
concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/eloqdoc_cluster_b.conf (1)
21-24: LGTM!Consistent TTL monitoring enablement for the cluster_b configuration.
concourse/scripts/eloqdoc.yaml (1)
20-23: LGTM!TTL monitoring enabled in the scripts configuration, consistent with the other configuration updates.
src/mongo/db/modules/eloq/src/eloq_index.cpp (2)
198-209: Improved restore() logic with cursor state awareness.The changes enhance
restore()by:
- Including
_eofin the log message for better debugging- Early return when at EOF to avoid unnecessary operations
- Conditionally seeking only when the cursor is absent or not open (
!_cursor || !_cursor->indexScanIsOpen())This is a safer approach than unconditionally seeking after every restore, particularly beneficial for TTL batched deletions where save/restore cycles are frequent.
188-190: No action required. The cursor state handling insave()andrestore()is correct:
save()preserves the cursor state (does not reset it), which is the appropriate behavior for resuming iterationsaveUnpositioned()explicitly resets_cursorwhen an unpositioned save is neededrestore()safely validates cursor state with the!_cursor || !_cursor->indexScanIsOpen()check before attempting to seek- The early return when
_eofis true prevents unnecessary workThe code properly separates concerns between positioned saves and unpositioned saves, and the restore logic correctly handles cursor state transitions.
src/mongo/db/query/internal_plans.cpp (3)
41-41: LGTM!The include is necessary for the
LimitStageclass used in the batching logic below.
148-150: LGTM!The
LimitStagewrapping is correctly placed before theDeleteStage, and the conditional logic appropriately applies the limit only whenlimits > 0.
134-135: The newint limitsparameter is backward compatible with a default value.Since the parameter has a default value (
int limits = 0), existing call sites don't break. The call site in ttl.cpp correctly passes the parameter for TTL batching, while storage_interface_impl.cpp uses the default, which is appropriate for its use case.src/mongo/db/ttl.cpp (2)
201-201: LGTM!The function signature correctly accepts the
int limitsparameter to support batched TTL deletions, consistent with the changes ininternal_plans.cpp.
303-304: LGTM!The
limitsparameter is correctly passed todeleteWithIndexScan, enabling batched TTL deletions as intended.
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/ttl.cpp (1)
152-172: Remove unnecessary WriteUnitOfWork from read-only loop.The
WriteUnitOfWorkwrapping each collection's index enumeration (line 153) is unnecessary and potentially incorrect. The code only performs read operations (getting index specs), yet a WUOW withoutcommit()will be rolled back at scope exit. This adds overhead without benefit.🔎 Apply this diff to remove the unnecessary WUOW:
for (const std::string& collectionNS : collections) { - WriteUnitOfWork wuow(&opCtx); UninterruptibleLockGuard noInterrupt(opCtx.lockState()); NamespaceString collectionNSS(collectionNS); AutoGetCollection autoGetCollection(&opCtx, collectionNSS, MODE_IS);
🧹 Nitpick comments (2)
src/mongo/db/modules/eloq/src/eloq_recovery_unit.cpp (1)
295-298: LGTM! Debug logging addition is consistent with the codebase pattern.The logging statement follows the established pattern used throughout this file.
Optional: Consider adding cursor context for enhanced debuggability
If more detailed debugging is needed in the future, you could include the cursor pointer for correlation:
- MONGO_LOG(1) << "EloqRecoveryUnit::registerCursor"; + MONGO_LOG(1) << "EloqRecoveryUnit::registerCursor, cursor: " << static_cast<void*>(cursor);However, this is purely optional given the current debugging needs.
src/mongo/db/ttl.cpp (1)
177-177: Consider making batch size configurable.The hardcoded batch size of 1000 might benefit from being a server parameter to allow tuning based on workload characteristics and system resources.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
concourse/artifact/ELOQDSS_ELOQSTORE/eloqdoc.conf(1 hunks)concourse/artifact/ELOQDSS_ROCKSDB/eloqdoc.conf(1 hunks)concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/eloqdoc.conf(1 hunks)concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/eloqdoc_cluster_a.conf(1 hunks)concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/eloqdoc_cluster_b.conf(1 hunks)concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/eloqdoc_cluster_c.conf(1 hunks)concourse/scripts/common.sh(2 hunks)concourse/scripts/eloqdoc.yaml(1 hunks)src/mongo/db/modules/eloq/src/eloq_index.cpp(1 hunks)src/mongo/db/modules/eloq/src/eloq_recovery_unit.cpp(1 hunks)src/mongo/db/query/internal_plans.cpp(3 hunks)src/mongo/db/query/internal_plans.h(1 hunks)src/mongo/db/ttl.cpp(4 hunks)src/mongo/shell/collection.js(1 hunks)tests/jstests/eloq_basic/ttl_basic_test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- concourse/artifact/ELOQDSS_ELOQSTORE/eloqdoc.conf
- concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/eloqdoc_cluster_a.conf
- concourse/scripts/common.sh
- concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/eloqdoc_cluster_c.conf
- concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/eloqdoc_cluster_b.conf
- src/mongo/db/modules/eloq/src/eloq_index.cpp
🧰 Additional context used
🧠 Learnings (2)
📚 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:
concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/eloqdoc.confconcourse/artifact/ELOQDSS_ROCKSDB/eloqdoc.conf
📚 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:
concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/eloqdoc.confsrc/mongo/db/modules/eloq/src/eloq_recovery_unit.cpp
🧬 Code graph analysis (1)
src/mongo/db/query/internal_plans.cpp (1)
src/mongo/db/ttl.cpp (2)
opCtx(201-316)opCtx(201-201)
🔇 Additional comments (12)
src/mongo/db/modules/eloq/src/eloq_recovery_unit.cpp (1)
300-303: LGTM! Symmetric logging addition complements the registerCursor change.The logging statement maintains symmetry with
registerCursorand follows the established codebase pattern.concourse/scripts/eloqdoc.yaml (1)
23-23: LGTM! TTL monitoring enabled.Enabling
ttlMonitorEnabledaligns with the PR objective to support TTL functionality. The change is consistent with similar updates across other configuration files in this PR.concourse/artifact/ELOQDSS_ROCKSDB/eloqdoc.conf (1)
24-24: LGTM! Consistent TTL enablement.Configuration change is consistent with the TTL feature rollout across different storage engine variants.
concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/eloqdoc.conf (1)
24-24: LGTM! TTL monitoring activated.src/mongo/shell/collection.js (1)
1479-1481: LGTM! Good defensive programming.Adding the empty array guard prevents potential undefined access errors when
countDocumentsreturns no matching documents. The aggregation pipeline with$groupwill return an empty array when there are no documents to count, and this guard correctly returns 0 in that case.tests/jstests/eloq_basic/ttl_basic_test.js (1)
1-111: LGTM! Well-structured TTL test.The test demonstrates good practices:
- Helper functions encapsulate parameter management with error handling
- Proper cleanup of test data and configuration
- Random TTL values test realistic scenarios
- Reasonable 60-second timeout provides safety margin
- Restores original TTL monitor parameter after testing
The test validates that the TTL monitoring feature works end-to-end with batched deletions.
src/mongo/db/query/internal_plans.cpp (2)
41-41: LGTM! Necessary include for LimitStage.
134-150: LGTM! Correct limit stage implementation.The implementation correctly:
- Adds the
limitsparameter to the signature- Wraps the index scan with a
LimitStageonly whenlimits > 0- Maintains proper stage ordering: IndexScan → Limit → Delete
This enables batched TTL deletions while preserving backward compatibility (default
limits = 0means no limit).src/mongo/db/query/internal_plans.h (1)
115-116: LGTM! API extension with backward compatibility.The new
limitsparameter with default value 0 maintains backward compatibility while enabling batched deletion for TTL operations.src/mongo/db/ttl.cpp (3)
178-188: LGTM! Correct batching implementation.The batching logic correctly:
- Tracks deletions before each batch
- Wraps each batch in its own
WriteUnitOfWork(committed at line 183)- Loops while a full batch was deleted (indicating more may remain)
- Logs timing and deletion metrics per batch
This approach prevents long-running transactions while ensuring all expired documents are eventually deleted.
189-192: LGTM! Resilient error handling.Catching
DBExceptionand continuing to the next index ensures that a failure processing one TTL index doesn't block TTL cleanup for other indexes. The error is logged with sufficient detail for debugging.
303-304: LGTM! Correct limits propagation.The
limitsparameter is correctly passed todeleteWithIndexScan, enabling the batched deletion behavior implemented in the internal planner.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
tests/jstests/eloq_basic/collection_scan_delete.js (4)
8-9: Variable shadowing may cause confusion.Line 8 reassigns the global
dbvariable to itself viagetSiblingDB(). While this works in MongoDB shell context (wheredbis a pre-defined global), it can confuse readers and static analysis tools. Consider using a different variable name for clarity.🔎 Apply this diff to use a clearer variable name:
-var db = db.getSiblingDB(DB_NAME); -var coll = db.getCollection(COLL_NAME); +var testDb = db.getSiblingDB(DB_NAME); +var coll = testDb.getCollection(COLL_NAME);
26-26: Consider usingcountDocuments()instead of deprecatedcount().The
count()method is deprecated in MongoDB 4.x and later.countDocuments()is the recommended replacement.🔎 Apply this diff:
-print("Inserted documents:", coll.count()); +print("Inserted documents:", coll.countDocuments({}));
61-64: Consider usingdeleteMany()instead of deprecatedremove().The
remove()method is deprecated in MongoDB 4.x and later. The modern replacement isdeleteMany()which has the same semantics but aligns with current MongoDB best practices.🔎 Apply this diff:
-var result = coll.remove( +var result = coll.deleteMany( { k: { $gte: 0 } }, { hint: { $natural: 1 } } );
73-73: Consider usingcountDocuments()instead of deprecatedcount().The
count()method is deprecated in MongoDB 4.x and later.countDocuments()is the recommended replacement.🔎 Apply this diff:
-var remaining = coll.count(); +var remaining = coll.countDocuments({});
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/mongo/db/modules/eloq/src/eloq_record_store.cpp(3 hunks)tests/jstests/eloq_basic/collection_scan_delete.js(1 hunks)tests/jstests/eloq_basic/delete_many.js(1 hunks)
🧰 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_record_store.cpp
🧬 Code graph analysis (2)
tests/jstests/eloq_basic/collection_scan_delete.js (1)
tests/jstests/eloq_basic/ttl_basic_test.js (2)
bulk(61-61)remaining(85-85)
tests/jstests/eloq_basic/delete_many.js (1)
tests/jstests/eloq_basic/ttl_basic_test.js (1)
bulk(61-61)
🪛 Biome (2.1.2)
tests/jstests/eloq_basic/collection_scan_delete.js
[error] 8-8: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
🔇 Additional comments (3)
src/mongo/db/modules/eloq/src/eloq_record_store.cpp (3)
448-450: LGTM: Good defensive assertion.The debug assertion ensures the cursor's index scan is open when reusing an existing cursor, which helps catch state inconsistencies during development.
477-485: LGTM: Enhanced diagnostics and cursor management.The added logging provides useful transaction context, and explicitly resetting an existing cursor before seeking ensures a clean state for the exact match operation.
527-532: Verify that removing the cursor null assertion is safe.The
restore()method previously asserted that_cursoris null, enforcing a strict precondition. This assertion has been removed, allowingrestore()to succeed even when a cursor exists. Verify that:
- This relaxed precondition is intentional for the TTL feature
- Callers of
restore()properly handle the case where an existing cursor is not recreated- This doesn't mask state management bugs in cursor lifecycle
The comment suggests Mongo may call
seekExact()afterrestore(), which would not need a scan, but ensure this pattern is consistently handled.#!/bin/bash # Description: Search for calls to restore() to understand the calling patterns and verify they handle existing cursors properly rg -nP --type=cpp -C5 '->restore\(\)|\.restore\(\)' --glob='**/eloq*.cpp' --glob='**/eloq*.h'
| MONGO_LOG(1) << "EloqRecordStoreCursor::saveUnpositioned " << _tableName->StringView(); | ||
| // _lastMongoKey.reset(); | ||
| _cursor.reset(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove commented-out code.
The commented line should be removed entirely rather than left in place. If the reset of _lastMongoKey is no longer needed (as the change suggests), the commented code creates maintenance debt and uncertainty about the intended behavior.
🔎 Apply this diff to remove the commented code:
void saveUnpositioned() override {
MONGO_LOG(1) << "EloqRecordStoreCursor::saveUnpositioned " << _tableName->StringView();
- // _lastMongoKey.reset();
_cursor.reset();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| MONGO_LOG(1) << "EloqRecordStoreCursor::saveUnpositioned " << _tableName->StringView(); | |
| // _lastMongoKey.reset(); | |
| _cursor.reset(); | |
| MONGO_LOG(1) << "EloqRecordStoreCursor::saveUnpositioned " << _tableName->StringView(); | |
| _cursor.reset(); |
🤖 Prompt for AI Agents
In src/mongo/db/modules/eloq/src/eloq_record_store.cpp around lines 518 to 520,
remove the commented-out line "// _lastMongoKey.reset();" so only active code
remains; leave the log and _cursor.reset() intact to avoid dead/commented code
and maintenance confusion.
xiexiaoy
force-pushed
the
ttl
branch
2 times, most recently
from
9dbb183 to
1b37b95
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
src/mongo/db/modules/eloq/src/eloq_record_store.cpp (3)
516-518: Remove commented-out code.As noted in the previous review, the commented line should be removed entirely rather than left in place. Commented code creates maintenance debt and uncertainty about intended behavior.
🔎 Apply this diff to remove the commented code:
void saveUnpositioned() override { MONGO_LOG(1) << "EloqRecordStoreCursor::saveUnpositioned " << _tableName->StringView(); - // _lastMongoKey.reset(); _cursor.reset(); }
521-523: The save() method still lacks position tracking implementation.As identified in the previous review, the
save()method remains empty and doesn't persist the cursor position. For TTL batch operations with cursor yielding,save()must capture the current_lastMongoKeysorestore()can resume iteration. This critical issue persists across the entire save/restore/next path.
525-529: The restore() method still lacks position resumption logic.As identified in the previous review,
restore()remains empty and cannot reposition the cursor to resume iteration after a yield. While the comment correctly notes thatseekExact()doesn't require scan initialization, iteration resumption (critical for TTL batch operations) requires checking if_lastMongoKeyis set and repositioning the underlying cursor accordingly—similar to theEloqIndexCursor::restorepattern shown in the relevant snippets.
🧹 Nitpick comments (1)
tests/jstests/eloq_basic/collection_scan_delete.js (1)
14-29: Bulk insert implementation looks good.Note:
coll.count()(line 26) is deprecated in newer MongoDB versions in favor ofcountDocuments()orestimatedDocumentCount(). Since the comment indicates this is for MongoDB 4.x, this may be intentional for compatibility. Consider updating if broader version support is needed.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/mongo/db/modules/eloq/src/eloq_record_store.cpp(4 hunks)tests/jstests/eloq_basic/collection_scan_delete.js(1 hunks)tests/jstests/eloq_basic/delete_many.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/jstests/eloq_basic/delete_many.js
🧰 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_record_store.cpp
🧬 Code graph analysis (1)
src/mongo/db/modules/eloq/src/eloq_record_store.cpp (1)
src/mongo/db/modules/eloq/src/eloq_index.cpp (20)
key(110-126)key(110-110)key(128-149)key(128-130)key(171-177)key(171-172)key(227-253)key(227-227)key(466-472)key(466-466)key(483-488)key(483-483)key(497-503)key(497-497)MONGO_LOG(188-190)MONGO_LOG(192-196)MONGO_LOG(198-209)MONGO_LOG(211-216)MONGO_LOG(329-360)MONGO_LOG(378-402)
🪛 Biome (2.1.2)
tests/jstests/eloq_basic/collection_scan_delete.js
[error] 8-8: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
🔇 Additional comments (4)
tests/jstests/eloq_basic/collection_scan_delete.js (2)
1-12: Setup looks correct for MongoDB shell context.The static analysis warning about
dbbeing used before declaration (line 8) is a false positive —dbis a predefined global object in the MongoDB shell, sodb.getSiblingDB()is valid.Minor nit: Line 2's usage comment references
collection_scan_delete_test.jsbut the actual filename iscollection_scan_delete.js.
31-53: LGTM!Good use of
quit(1)to fail the test early if the expected COLLSCAN plan is not used. The explain verification logic is correct.src/mongo/db/modules/eloq/src/eloq_record_store.cpp (2)
460-460: LGTM!Explicit type casting improves code clarity.
476-482: LGTM!Enhanced logging with table name and transaction context improves observability for debugging TTL and cursor operations.
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_record_store.cpp (1)
446-529: TTL feature may be broken without cursor position tracking.This PR aims to support TTL with batched deletions, which requires cursor yielding during batch processing. However, the unresolved position tracking issues in
next(),save(), and the_lastMongoKeyupdate path mean cursors cannot correctly resume after yielding. This could result in skipped records, duplicate processing, or incorrect TTL deletion behavior.Run the following script to check if there are TTL-related tests that exercise cursor yielding:
#!/bin/bash # Description: Find TTL tests that might be affected by cursor yielding issues # Search for TTL test files echo "=== TTL test files in the PR ===" fd -t f "ttl.*\.(js|cpp)" . echo -e "\n=== Check for cursor save/restore patterns in TTL code ===" rg -n "save\(\)|restore\(\)" --type cpp src/mongo/db/ttl.cpp -C 3 echo -e "\n=== Look for batch limit or yield patterns ===" rg -n "batch|yield|limit" --type cpp src/mongo/db/ttl.cpp -C 2
♻️ Duplicate comments (5)
tests/jstests/eloq_basic/collection_scan_delete.js (1)
76-79: Test doesn't fail properly when verification fails.If
remaining !== 0, the test prints "ERROR" but exits with code 0 (success). This is inconsistent with line 52 wherequit(1)is called on error. The test will appear to pass in CI even when documents weren't properly deleted, defeating its purpose.🔎 Apply this diff to properly fail the test:
if (remaining !== 0) { print("ERROR: Not all documents were deleted"); + quit(1); } else { print("SUCCESS: All documents deleted via Collection Scan"); }src/mongo/db/modules/eloq/src/eloq_record_store.cpp (4)
446-473: CRITICAL: Position tracking still unimplemented, breaking cursor resumption for TTL operations.The defensive check
!_cursor->indexScanIsOpen()improves robustness. However, the critical issue from the previous review remains unresolved:next()never updates_lastMongoKeywith the current iteration position (specifically after line 468 where the key is extracted). This breaks cursor yielding/resumption, which is essential for TTL batch deletions mentioned in this PR.🔎 Apply this diff to implement position tracking in next():
RecordId id = key->ToRecordId(false); + // Update position for cursor save/restore + if (_lastMongoKey) { + _lastMongoKey->Copy(*key); + } else { + _lastMongoKey.emplace(*key); + } MONGO_LOG(1) << "id: " << id << ". record:" << BSONObj{record->EncodedBlobData()}.jsonString();
525-529: Verify restore() semantics with save/next position tracking.The
restore()method intentionally defers scan reopening tonext(). However, this design only works correctly ifnext()updates_lastMongoKeywith the current position (currently missing). Oncenext()is fixed to track position, verify that the deferred scan reopening in_seekCursor()(called fromnext()) correctly uses_lastMongoKeyto resume from the saved position.
516-518: Remove commented-out code (flagged in previous review).The commented line
// _lastMongoKey.reset();should be removed entirely rather than left in place, as previously requested.🔎 Apply this diff to remove the commented code:
void saveUnpositioned() override { MONGO_LOG(1) << "EloqRecordStoreCursor::saveUnpositioned " << _tableName->StringView(); - // _lastMongoKey.reset(); _cursor.reset(); }
521-523: CRITICAL: save() method lacks position tracking (flagged in previous review).The
save()method remains empty and doesn't persist the cursor position. This is critical for TTL batch operations that require cursor yielding. The entire save/restore/next path needs position tracking implementation as detailed in the previous review.🔎 Implement position tracking in save():
void save() override { MONGO_LOG(1) << "EloqRecordStoreCursor::save " << _tableName->StringView(); + // Position is maintained in _lastMongoKey, which is updated by next() + // No additional action needed here if next() is fixed }Note: This requires fixing
next()to update_lastMongoKeyfirst (see comment on lines 446-473).
🧹 Nitpick comments (1)
tests/jstests/eloq_basic/collection_scan_delete.js (1)
8-8: Consider using a distinct variable name for clarity.The pattern
var db = db.getSiblingDB(DB_NAME)relies on the right-handdbreferencing the MongoDB shell global before the localvar dbshadows it. While this may work in MongoDB shell context, it's flagged by static analysis and can be confusing to readers.🔎 Apply this diff to use a clearer variable name:
-var db = db.getSiblingDB(DB_NAME); +var testDb = db.getSiblingDB(DB_NAME); var coll = db.getCollection(COLL_NAME);Then update references to
db.getCollectionon line 9 to usetestDb.getCollection, or simply remove thevardeclaration and reassign the globaldbdirectly if that's the intended pattern.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/mongo/db/modules/eloq/src/eloq_record_store.cpp(4 hunks)tests/jstests/eloq_basic/collection_scan_delete.js(1 hunks)tests/jstests/eloq_basic/delete_many.js(1 hunks)
🧰 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_record_store.cpp
🧬 Code graph analysis (1)
tests/jstests/eloq_basic/collection_scan_delete.js (1)
tests/jstests/eloq_basic/delete_many.js (5)
bulk(11-11)i(12-12)i(34-34)start(23-23)end(28-28)
🪛 Biome (2.1.2)
tests/jstests/eloq_basic/collection_scan_delete.js
[error] 8-8: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
🔇 Additional comments (3)
tests/jstests/eloq_basic/delete_many.js (1)
1-52: LGTM! Variable declarations have been properly addressed.The test is well-structured and all variables are now properly declared with
leton their first use (lines 11, 23, 28). The subsequent reassignments ofbulk,start, andend(lines 33, 45, 50) correctly omitletsince the variables are already in scope. The test effectively compares delete performance using different index hints with proper timing and verification.src/mongo/db/modules/eloq/src/eloq_record_store.cpp (2)
460-460: Good type clarity improvement.Explicitly specifying
const Eloq::MongoKey*instead ofauto const*improves code readability and type safety.
476-477: LGTM: Enhanced logging for debugging.Adding transaction number context to the log messages improves observability for TTL and batched deletion operations.
Also applies to: 480-481
xiexiaoy
force-pushed
the
ttl
branch
2 times, most recently
from
534932b to
8e77a92
Compare
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 (2)
src/mongo/db/modules/eloq/src/eloq_record_store.cpp (2)
440-473: Critical: Position tracking still not implemented in next().While the cursor readiness check at line 446 is good, the critical issue from the previous review persists:
next()never updates_lastMongoKeywith the current iteration position. After extracting the key at line 468,_lastMongoKeyshould be set to preserve the cursor position for yield/resume cycles. Without this,save()cannot capture the iteration position, breaking cursor resumption during TTL batch deletions.🔎 Apply this diff to track position during iteration:
RecordId id = key->ToRecordId(false); + if (_lastMongoKey) { + _lastMongoKey->Copy(*key); + } else { + _lastMongoKey.emplace(*key); + } MONGO_LOG(1) << "id: " << id << ". record:" << BSONObj{record->EncodedBlobData()}.jsonString();
528-533: restore() doesn't reposition the cursor after yield.The
restore()method returns immediately without reopening the cursor or seeking to the saved position. While the comment suggests this is intentional for lazy reopening, this breaks the save/restore contract when combined with the emptysave()method. The pattern ineloq_index.cpp(lines 197-208) showsrestore()should checkif (!_cursor || !_cursor->indexScanIsOpen())and call_seekCursor()to reposition.Current behavior:
next()will eventually reopen via_seekCursor()(line 447), but only if_lastMongoKeywas populated. Sincesave()doesn't capture position andnext()doesn't update it, cursor resumption is broken.🔎 Apply this diff to implement proper cursor restoration:
bool restore() override { MONGO_LOG(1) << "EloqRecordStoreCursor::restore " << _tableName->StringView(); - // Don't open scan here. - // Mongo may call seekExact which don't need a scan in TxService + if (_eof) { + return true; + } + + if (!_cursor || !_cursor->indexScanIsOpen()) { + if (_lastMongoKey) { + // Reposition cursor after last returned key + _seekCursor(false); + } + } return true; }Note: This still requires fixing
next()to update_lastMongoKeyandsave()to capture position.
♻️ Duplicate comments (2)
src/mongo/db/modules/eloq/src/eloq_record_store.cpp (2)
517-517: Remove the commented-out code.The commented line
// _lastMongoKey.reset();should be removed as noted in the previous review. Leaving commented code creates maintenance debt and ambiguity about intended behavior.🔎 Apply this diff to remove the commented code:
void saveUnpositioned() override { MONGO_LOG(1) << "EloqRecordStoreCursor::saveUnpositioned " << _tableName->StringView(); - // _lastMongoKey.reset(); if (!_eof && _cursor && _cursor->currentBatchTuple() != nullptr) { _lastMongoKey.emplace(*_cursor->currentBatchTuple()->key_.GetKey<Eloq::MongoKey>()); }
524-526: save() lacks position capture for yield/resume cycles.The
save()method is empty and doesn't capture the cursor position. WhilesaveUnpositioned()handles position capture when resetting the cursor,save()should preserve the current iteration position for yield/resume without fully resetting. Sincenext()doesn't update_lastMongoKey(flagged separately),save()has no position to preserve, making cursor resumption unreliable.🔎 Apply this diff to capture position in save():
void save() override { MONGO_LOG(1) << "EloqRecordStoreCursor::save " << _tableName->StringView(); + if (!_eof && _cursor && _cursor->currentBatchTuple() != nullptr) { + if (_lastMongoKey) { + _lastMongoKey->Copy(*_cursor->currentBatchTuple()->key_.GetKey<Eloq::MongoKey>()); + } else { + _lastMongoKey.emplace(*_cursor->currentBatchTuple()->key_.GetKey<Eloq::MongoKey>()); + } + } }Note: This still requires fixing
next()to update_lastMongoKeyafter each advance.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.clangd(0 hunks)src/mongo/db/catalog/collection.h(2 hunks)src/mongo/db/catalog/collection_impl.cpp(1 hunks)src/mongo/db/catalog/collection_impl.h(1 hunks)src/mongo/db/exec/delete.cpp(1 hunks)src/mongo/db/modules/eloq/src/eloq_record_store.cpp(3 hunks)tests/jstests/eloq_basic/collection_scan_delete.js(1 hunks)tests/jstests/eloq_basic/delete_many.js(1 hunks)tests/jstests/eloq_basic/ttl_basic_test.js(1 hunks)
💤 Files with no reviewable changes (1)
- .clangd
🚧 Files skipped from review as they are similar to previous changes (3)
- src/mongo/db/catalog/collection.h
- src/mongo/db/catalog/collection_impl.cpp
- src/mongo/db/exec/delete.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_record_store.cpp
🧬 Code graph analysis (4)
src/mongo/db/catalog/collection_impl.h (2)
src/mongo/db/catalog/collection_impl.cpp (4)
deleteDocument(574-583)deleteDocument(574-580)deleteDocument(585-619)deleteDocument(585-592)src/mongo/db/catalog/collection.h (2)
BSONObj(435-438)StoreDeletedDoc(165-485)
tests/jstests/eloq_basic/delete_many.js (1)
tests/jstests/eloq_basic/ttl_basic_test.js (2)
bulk(61-61)i(64-64)
src/mongo/db/modules/eloq/src/eloq_record_store.cpp (1)
src/mongo/db/modules/eloq/src/eloq_index.cpp (6)
MONGO_LOG(188-190)MONGO_LOG(192-196)MONGO_LOG(198-209)MONGO_LOG(211-216)MONGO_LOG(329-360)MONGO_LOG(378-402)
tests/jstests/eloq_basic/collection_scan_delete.js (2)
tests/jstests/core/or4.js (1)
coll(14-14)tests/jstests/eloq_basic/delete_many.js (5)
bulk(11-11)i(12-12)i(34-34)start(23-23)end(28-28)
🪛 Biome (2.1.2)
tests/jstests/eloq_basic/collection_scan_delete.js
[error] 8-8: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
🔇 Additional comments (9)
src/mongo/db/catalog/collection_impl.h (1)
161-169: LGTM! Performance optimization to avoid redundant document fetches.The new overload allows callers to pass a pre-fetched
Snapshotted<BSONObj>document, eliminating an unnecessarydocForcall when the document is already available. The signature is consistent with the existing overload, and the implementation properly handles the new parameter.tests/jstests/eloq_basic/delete_many.js (1)
1-52: LGTM! Well-structured delete performance test.The test properly:
- Sets up a collection with multiple indexes
- Inserts documents via bulk operations
- Measures delete performance with different index hints (sk vs _id)
- Uses proper variable declarations throughout
All previously flagged issues have been addressed.
tests/jstests/eloq_basic/collection_scan_delete.js (2)
1-51: LGTM! Test setup and explain phase are well-structured.The test properly:
- Sets up test database and collection
- Inserts documents via bulk operation
- Uses
explainto validate COLLSCAN plan before executing- Fails early with
assertif the plan is incorrectNote: The static analysis warning at line 8 is a false positive. The
dbvariable shadows the global MongoDB shelldbobject, which is standard practice in MongoDB shell scripts.
52-72: LGTM! Delete execution and verification are correct.The test properly:
- Executes the collection scan delete with timing
- Uses
assert.eqto verify all documents are deleted, which will fail the test if documents remainThe use of
.remove()instead of.deleteMany()is likely intentional for MongoDB 4.x compatibility as indicated by the comment on line 1.tests/jstests/eloq_basic/ttl_basic_test.js (4)
5-14: LGTM! Well-structured helper functions.The helper functions properly wrap
adminCommandwith error checking and throw meaningful errors on failure. Using computed property names[paramName]is the correct approach for dynamic parameter names.
16-54: LGTM! Proper test setup with parameter management.The test correctly:
- Saves the original
ttlMonitorSleepSecsvalue for restoration- Sets it to 1 second to speed up test execution
- Creates a TTL index with
expireAfterSeconds: 0on theexpireAtfieldThis setup ensures the test runs efficiently while maintaining proper cleanup.
56-78: LGTM! Document insertion with random TTL values.The test properly:
- Generates random TTL values between 1-10 seconds
- Calculates
expireAttimestamps based on current time + TTL- Uses bulk unordered insert for efficient insertion of 10,000 documents
This approach effectively tests the TTL monitor with a realistic distribution of expiration times.
80-111: LGTM! Wait loop and cleanup are appropriate.The test correctly:
- Polls the collection every second to track document expiration
- Has a generous 120-second timeout (12× the maximum TTL)
- Asserts that all documents are eventually deleted
- Restores the original
ttlMonitorSleepSecsvalueNote: Line 102 is missing a semicolon after the
assert.eqcall, but this is optional in JavaScript and doesn't affect functionality.src/mongo/db/modules/eloq/src/eloq_record_store.cpp (1)
476-482: LGTM: Enhanced logging for debugging.The addition of transaction numbers to the logging statements improves observability and aids debugging of concurrent operations.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/mongo/db/modules/eloq/src/eloq_record_store.cpp (1)
515-521: Remove commented-out code.The commented line
// _lastMongoKey.reset();should be removed entirely rather than left in place. Commented-out code creates maintenance debt and uncertainty about the intended behavior.🔎 Apply this diff to remove the commented code:
void saveUnpositioned() override { MONGO_LOG(1) << "EloqRecordStoreCursor::saveUnpositioned " << _tableName->StringView(); - // _lastMongoKey.reset(); if (!_eof && _cursor && _cursor->currentBatchTuple() != nullptr) { _lastMongoKey.emplace(*_cursor->currentBatchTuple()->key_.GetKey<Eloq::MongoKey>()); } _cursor.reset(); }
🧹 Nitpick comments (1)
src/mongo/db/catalog/collection_impl.h (1)
161-169: Consider adding documentation for the new overload.The new
deleteDocumentoverload accepting a pre-fetched document is a useful optimization. Consider adding a brief comment explaining when to use this overload versus the original one (e.g., "Use this overload when the document is already in memory to avoid an extra fetch").🔎 Suggested documentation
+ /** + * Deletes the document with the given RecordId from the collection, using a pre-fetched + * document to avoid an additional fetch. Use this overload when the document is already + * available in memory. + * + * Parameters are the same as the other deleteDocument overload. + */ void deleteDocument( OperationContext* opCtx, StmtId stmtId, const RecordId& loc, const Snapshotted<BSONObj>& doc, OpDebug* opDebug, bool fromMigrate = false, bool noWarn = false, Collection::StoreDeletedDoc storeDeletedDoc = Collection::StoreDeletedDoc::Off) final;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.clangd(0 hunks)src/mongo/db/catalog/collection.h(2 hunks)src/mongo/db/catalog/collection_impl.cpp(1 hunks)src/mongo/db/catalog/collection_impl.h(1 hunks)src/mongo/db/exec/delete.cpp(1 hunks)src/mongo/db/modules/eloq/src/eloq_record_store.cpp(3 hunks)tests/jstests/eloq_basic/collection_scan_delete.js(1 hunks)tests/jstests/eloq_basic/delete_many.js(1 hunks)tests/jstests/eloq_basic/ttl_basic_test.js(1 hunks)
💤 Files with no reviewable changes (1)
- .clangd
🚧 Files skipped from review as they are similar to previous changes (2)
- src/mongo/db/exec/delete.cpp
- tests/jstests/eloq_basic/ttl_basic_test.js
🧰 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_record_store.cpp
🧬 Code graph analysis (3)
tests/jstests/eloq_basic/delete_many.js (1)
tests/jstests/eloq_basic/ttl_basic_test.js (2)
bulk(61-61)i(64-64)
src/mongo/db/catalog/collection.h (1)
src/mongo/db/catalog/collection_impl.cpp (4)
deleteDocument(574-583)deleteDocument(574-580)deleteDocument(585-619)deleteDocument(585-592)
src/mongo/db/modules/eloq/src/eloq_record_store.cpp (1)
src/mongo/db/modules/eloq/src/eloq_index.cpp (6)
MONGO_LOG(188-190)MONGO_LOG(192-196)MONGO_LOG(198-209)MONGO_LOG(211-216)MONGO_LOG(329-360)MONGO_LOG(378-402)
🪛 Biome (2.1.2)
tests/jstests/eloq_basic/collection_scan_delete.js
[error] 8-8: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
🔇 Additional comments (14)
src/mongo/db/catalog/collection.h (2)
232-239: LGTM!The pure virtual interface addition follows the existing pattern correctly and properly extends the Collection::Impl API.
487-497: LGTM!The public wrapper correctly forwards all parameters to the implementation and follows the established forwarding pattern used throughout this class.
src/mongo/db/catalog/collection_impl.cpp (2)
574-583: LGTM! Clean delegation pattern.The wrapper implementation correctly fetches the document once using
docForand delegates to the new overload, maintaining backward compatibility while avoiding code duplication.
585-619: LGTM! Efficient implementation that avoids redundant fetches.The new overload correctly uses the pre-fetched document throughout (
doc.value()for observers, index operations, etc.), which eliminates an unnecessary fetch when the document is already in memory. This optimization should improve performance for TTL deletions and other batch operations.The implementation correctly maintains:
- Snapshot consistency via
Snapshotted<BSONObj>- Capped collection guards
- Cursor invalidation
- Index unindexing
- Observer notifications
tests/jstests/eloq_basic/delete_many.js (3)
1-8: LGTM!The test setup properly drops the old collection, creates a fresh one, and creates the required index on
sk.
10-19: LGTM!The bulk insert operation correctly inserts 10,000 documents with
_id,sk, andvaluefields.
32-41: LGTM!The re-insert operation correctly repopulates the collection with the same 10,000 documents for the second delete test.
tests/jstests/eloq_basic/collection_scan_delete.js (3)
1-29: LGTM!The test setup is well-structured with clear constants, proper cleanup, bulk insert of test data, and diagnostic output. The use of
varis appropriate for MongoDB shell scripts.Note: The static analysis hint about line 8 is a false positive—
dbis a predefined global variable in the MongoDB shell environment.
31-50: LGTM!The explain phase properly validates that the delete plan uses COLLSCAN and includes diagnostic output. The assertion ensures the test fails early if the query planner doesn't use a collection scan as expected.
52-72: LGTM!The delete execution and verification phases are well-implemented. The test properly times the delete operation, prints diagnostic information, and uses an assertion to ensure all documents are deleted. This ensures the test will fail in CI if the delete doesn't work as expected.
src/mongo/db/modules/eloq/src/eloq_record_store.cpp (4)
446-448: LGTM! Cursor readiness check aligns with EloqIndexCursor pattern.The added
!_cursor->indexScanIsOpen()check correctly handles the edge case where the cursor object exists but the underlying scan is closed. This is consistent with the pattern used inEloqIndexCursor::restore()(eloq_index.cpp lines 203-206).
524-529: Position tracking implemented correctly.The
save()method now captures the current iteration position in_lastMongoKey, addressing the previously identified critical issue. Combined with the cursor readiness check innext()(line 446), this enables proper cursor resumption after yielding during TTL batch operations.
476-483: LGTM! Enhanced logging for debugging.Adding transaction numbers and table names to the logs improves observability for debugging TTL operations and cursor lifecycle events.
531-536: Lazy restoration pattern is valid and intentional.The implementation uses lazy restoration:
save()captures position via_lastMongoKey,restore()returns immediately without reopening, andnext()reopens the cursor on demand via_seekCursor()if the cursor is not open. This differs fromEloqIndexCursor::restore()which actively reopens the cursor if needed. Both approaches are correct and work as designed.The lazy approach is intentional here—the comment in
restore()explains: "Don't open scan here. Mongo may call seekExact which don't need a scan in TxService." This optimization avoids unnecessary scan initialization when not needed.For consistency across cursor implementations, consider whether aligning with the
EloqIndexCursorpattern (reopening inrestore()) would simplify maintenance, though the current approach is functionally sound.
| // Delete all documents using sk index | ||
| print("Start delete by sk..."); | ||
| let start = new Date(); | ||
| db.sk_test.deleteMany( | ||
| { sk: { $gte: 0 } }, | ||
| { hint: { sk: 1 } } | ||
| ); | ||
| let end = new Date(); | ||
| print("Delete by sk time(ms): " + (end - start)); | ||
| print("Remaining docs after sk delete: " + db.sk_test.countDocuments({})); |
There was a problem hiding this comment.
Add assertions to verify delete succeeded.
The test prints timing and remaining document counts but doesn't fail if documents aren't properly deleted. This is inconsistent with collection_scan_delete.js (lines 50, 72) which uses assertions. Without assertions, this test will always pass in CI even if the delete operation fails.
🔎 Apply this diff to add verification:
let end = new Date();
print("Delete by sk time(ms): " + (end - start));
-print("Remaining docs after sk delete: " + db.sk_test.countDocuments({}));
+let remaining = db.sk_test.countDocuments({});
+print("Remaining docs after sk delete: " + remaining);
+assert.eq(0, remaining, "All documents should be deleted by sk index");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Delete all documents using sk index | |
| print("Start delete by sk..."); | |
| let start = new Date(); | |
| db.sk_test.deleteMany( | |
| { sk: { $gte: 0 } }, | |
| { hint: { sk: 1 } } | |
| ); | |
| let end = new Date(); | |
| print("Delete by sk time(ms): " + (end - start)); | |
| print("Remaining docs after sk delete: " + db.sk_test.countDocuments({})); | |
| // Delete all documents using sk index | |
| print("Start delete by sk..."); | |
| let start = new Date(); | |
| db.sk_test.deleteMany( | |
| { sk: { $gte: 0 } }, | |
| { hint: { sk: 1 } } | |
| ); | |
| let end = new Date(); | |
| print("Delete by sk time(ms): " + (end - start)); | |
| let remaining = db.sk_test.countDocuments({}); | |
| print("Remaining docs after sk delete: " + remaining); | |
| assert.eq(0, remaining, "All documents should be deleted by sk index"); |
🤖 Prompt for AI Agents
In tests/jstests/eloq_basic/delete_many.js around lines 21 to 30, the test
prints timing and remaining document counts but lacks assertions so failures in
deleteMany won't fail CI; add explicit assertions after the delete to verify
that the expected number of documents were removed (e.g., assert that
countDocuments returns 0 or equals the expected remaining count) and/or assert
the deleteMany result indicates the expected deletedCount; keep the existing
timing prints but fail the test when deletion did not occur.
| // Delete all documents using _id index | ||
| print("Start delete by _id..."); | ||
| start = new Date(); | ||
| db.sk_test.deleteMany( | ||
| { _id: { $gte: 0 } }, | ||
| { hint: { _id: 1 } } | ||
| ); | ||
| end = new Date(); | ||
| print("Delete by _id time(ms): " + (end - start)); | ||
| print("Remaining docs after _id delete: " + db.sk_test.countDocuments({})); |
There was a problem hiding this comment.
Add assertions to verify delete succeeded.
Similar to the first delete test, this section lacks assertions to verify the delete operation succeeded. The test should fail if documents remain after deletion.
🔎 Apply this diff to add verification:
let end = new Date();
print("Delete by _id time(ms): " + (end - start));
-print("Remaining docs after _id delete: " + db.sk_test.countDocuments({}));
+let remaining = db.sk_test.countDocuments({});
+print("Remaining docs after _id delete: " + remaining);
+assert.eq(0, remaining, "All documents should be deleted by _id index");Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In tests/jstests/eloq_basic/delete_many.js around lines 43 to 52, the
delete-by-_id block prints timings but lacks assertions to verify the delete
succeeded; update the block to assert the operation removed documents by (1)
capturing the deleteMany result and asserting result.deletedCount equals the
expected number (or is >0), and (2) asserting the collection countDocuments() is
0 (or the expected remaining count) after deletion using the test/assert helper
(e.g., assert.eq), so the test fails if documents remain.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/mongo/shell/collection.js (1)
1479-1481: Good defensive fix to handle empty aggregation results.The guard correctly prevents a potential
TypeErrorwhen no documents match the query. When the$matchstage filters out all documents, the$groupstage produces no output, resulting in an empty array. Returning0is the correct count in this case.Optional: Use strict equality
Consider using
===instead of==for strict equality comparison:- if (res.length == 0) { + if (res.length === 0) {src/mongo/db/ttl.cpp (1)
177-188: Add safeguard to prevent infinite batching loop.The batching loop continues while
ttlDeletedDocuments - before >= limits, which could theoretically run indefinitely if deletions consistently match or exceed the limit. While unlikely in practice, adding a maximum iteration count or timeout would make the behavior more predictable.🔎 Add max iterations safeguard
+ const int maxBatches = 100; // Configurable limit + int batchCount = 0; long long before; const int limits = 1000; do { + if (++batchCount > maxBatches) { + LOG(1) << "TTL deletion reached maximum batch limit for index " << idx; + break; + } before = ttlDeletedDocuments; auto begin_time = Date_t::now();
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
.clangd(0 hunks)concourse/artifact/ELOQDSS_ELOQSTORE/eloqdoc.conf(1 hunks)concourse/artifact/ELOQDSS_ROCKSDB/eloqdoc.conf(1 hunks)concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/eloqdoc.conf(1 hunks)concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/eloqdoc_cluster_a.conf(1 hunks)concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/eloqdoc_cluster_b.conf(1 hunks)concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/eloqdoc_cluster_c.conf(1 hunks)concourse/scripts/common.sh(2 hunks)concourse/scripts/eloqdoc.yaml(1 hunks)src/mongo/db/catalog/collection.h(2 hunks)src/mongo/db/catalog/collection_impl.cpp(1 hunks)src/mongo/db/catalog/collection_impl.h(1 hunks)src/mongo/db/exec/delete.cpp(1 hunks)src/mongo/db/modules/eloq/src/eloq_index.cpp(1 hunks)src/mongo/db/modules/eloq/src/eloq_record_store.cpp(3 hunks)src/mongo/db/modules/eloq/src/eloq_recovery_unit.cpp(1 hunks)src/mongo/db/query/internal_plans.cpp(3 hunks)src/mongo/db/query/internal_plans.h(1 hunks)src/mongo/db/ttl.cpp(4 hunks)src/mongo/shell/collection.js(1 hunks)tests/jstests/eloq_basic/collection_scan_delete.js(1 hunks)tests/jstests/eloq_basic/delete_many.js(1 hunks)tests/jstests/eloq_basic/ttl_basic_test.js(1 hunks)
💤 Files with no reviewable changes (1)
- .clangd
🚧 Files skipped from review as they are similar to previous changes (12)
- src/mongo/db/query/internal_plans.cpp
- concourse/artifact/ELOQDSS_ROCKSDB/eloqdoc.conf
- src/mongo/db/catalog/collection.h
- concourse/scripts/eloqdoc.yaml
- tests/jstests/eloq_basic/delete_many.js
- tests/jstests/eloq_basic/ttl_basic_test.js
- concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/eloqdoc_cluster_b.conf
- src/mongo/db/query/internal_plans.h
- concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/eloqdoc_cluster_a.conf
- concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/eloqdoc.conf
- src/mongo/db/catalog/collection_impl.h
- src/mongo/db/modules/eloq/src/eloq_record_store.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.cppsrc/mongo/db/modules/eloq/src/eloq_recovery_unit.cpp
📚 Learning: 2025-11-28T10:52:25.527Z
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 279
File: concourse/scripts/main.ent.bash:38-39
Timestamp: 2025-11-28T10:52:25.527Z
Learning: In the eloqdoc enterprise build (main.ent.bash), eloq_log_service is fetched separately as the resource `eloq_logservice_src` in the pipeline (main.ent.yml) and then symlinked into the data_substrate directory structure at build time using `ln -s $WORKSPACE/eloq_logservice_src eloq_log_service`. This is the intended architecture for placing eloq_log_service within data_substrate in the enterprise configuration.
Applied to files:
concourse/scripts/common.sh
🧬 Code graph analysis (1)
tests/jstests/eloq_basic/collection_scan_delete.js (2)
tests/jstests/eloq_basic/delete_many.js (4)
bulk(11-11)i(12-12)i(34-34)end(28-28)tests/jstests/eloq_basic/ttl_basic_test.js (2)
bulk(61-61)i(64-64)
🪛 Biome (2.1.2)
tests/jstests/eloq_basic/collection_scan_delete.js
[error] 8-8: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
🔇 Additional comments (11)
src/mongo/db/exec/delete.cpp (1)
213-222: LGTM! Optimization correctly avoids redundant document fetch.Passing
member->objtodeleteDocumentis safe and efficient here because:
- The document's existence is guaranteed by the invariant at line 168
- Document validity is confirmed by
ensureStillMatchesat line 173- The document is already in-memory, avoiding an extra fetch
src/mongo/db/catalog/collection_impl.cpp (2)
574-583: LGTM! Clean wrapper preserves backward compatibility.The wrapper overload correctly fetches the document via
docFor(opCtx, loc)and delegates to the new overload, maintaining backward compatibility for existing callers while enabling the optimization path for callers with pre-fetched documents.
585-619: LGTM! Well-structured refactoring with consistent document usage.The new overload correctly:
- Accepts the pre-fetched document to avoid redundant fetch operations
- Uses
doc.value()consistently foraboutToDelete(line 599),unindexRecord(line 610), and observer notifications (line 618)- Preserves all existing logic including capped collection guards, cursor invalidation, and error handling
concourse/artifact/ELOQDSS_ELOQSTORE/eloqdoc.conf (1)
24-24: LGTM! TTL monitoring enabled.This configuration change correctly enables the TTL monitor for the ELOQDSS_ELOQSTORE deployment, aligning with the PR's TTL feature enablement.
concourse/artifact/ELOQDSS_ROCKSDB_CLOUD_S3/eloqdoc_cluster_c.conf (1)
24-24: LGTM! TTL monitoring enabled for cluster C.Consistent configuration change enabling TTL monitoring across the ROCKSDB_CLOUD_S3 cluster deployment.
concourse/scripts/common.sh (1)
199-199: LGTM! Launcher scripts now use TTL-enabled configuration.The switch from
store_rocksdb_cloud.yamltoeloqdoc.yamlaligns the launcher functions with the TTL-enabled configuration referenced in the PR.Also applies to: 223-223
src/mongo/db/modules/eloq/src/eloq_recovery_unit.cpp (1)
296-296: LGTM! Debug logging added for cursor lifecycle.These debug-level log statements provide useful observability for cursor registration and unregistration without impacting runtime performance.
Also applies to: 301-301
src/mongo/db/modules/eloq/src/eloq_index.cpp (2)
199-209: LGTM! Conditional cursor re-seek optimizes restore() performance.The enhanced logging and conditional logic avoid unnecessary seeks when the cursor remains open and positioned. The early return for
_eofand the check for an open cursor before seeking are sensible optimizations.
188-190: Verify that lightweight save() pattern handles all invalidation scenarios correctly.The implementation uses an intentional lightweight save() (logging only) paired with saveUnpositioned() for full reset. This optimization preserves cursor position between save/restore cycles, relying on restore() to re-seek using stored
_keyif the cursor becomes invalid.Verify:
- Whether
indexScanIsOpen()catches all cursor invalidation scenarios (e.g., index drop, collection reorganization during yield)- Whether
_keyremains valid if the document or index entry is deleted between save and restore- Whether
invalidate()method properly sets cursor state soindexScanIsOpen()returns false when needed- Whether
_seekCursor(_key, false)handles cases where the key no longer exists in the indexIf yield intervals can cause index modifications, consider using
saveUnpositioned()instead for these cases.tests/jstests/eloq_basic/collection_scan_delete.js (2)
8-8: Static analysis false positive -dbis a MongoDB shell global.The Biome warning about using
dbbefore declaration is a false positive. In the MongoDB shell environment,dbis a pre-defined global variable. The patterndb.getSiblingDB()is standard practice for switching database contexts.
70-72: LGTM! Test properly validates deletion with assert.The test correctly uses
assert.eq()to verify that all documents were deleted, which will fail the test with a clear error message if the deletion is incomplete.
| // EloqDoc: TTLCollectionCache is unreliable. | ||
| // | ||
| // TTLCollectionCache& ttlCollectionCache = | ||
| // TTLCollectionCache::get(getGlobalServiceContext()); | ||
| // std::vector<std::string> ttlCollections = ttlCollectionCache.getCollections(); | ||
| std::vector<std::string> databases, collections; | ||
| getGlobalServiceContext()->getStorageEngine()->listDatabases(&databases); | ||
| for (const std::string& dbName : databases) { | ||
| std::vector<std::string> colls; | ||
| getGlobalServiceContext()->getStorageEngine()->listCollections(dbName, &colls); | ||
| collections.insert(collections.end(), | ||
| std::make_move_iterator(colls.begin()), | ||
| std::make_move_iterator(colls.end())); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/mongo/db/ttl.cpp | head -200 | tail -100Repository: eloqdata/eloqdoc
Length of output: 4931
🏁 Script executed:
rg -n "listDatabases\|listCollections" src/mongo/db/ttl.cpp -B 5 -A 10Repository: eloqdata/eloqdoc
Length of output: 42
🏁 Script executed:
rg -n "admin|config|local" src/mongo/db/ttl.cppRepository: eloqdata/eloqdoc
Length of output: 42
Performance concern: full database/collection enumeration on every TTL pass.
The code enumerates all databases and collections via the storage engine (lines 138-145), building the entire collection list in memory before filtering to those with TTL indexes. While only collections with TTL indexes are ultimately processed, this full enumeration approach is inefficient for deployments with many databases or collections.
Consider:
- Skipping system databases (admin, config, local) during enumeration since they typically lack user-created TTL indexes
- Using incremental enumeration or caching to avoid full list reconstruction every 60 seconds
- Early termination if a quick catalog check shows no TTL indexes exist
| for (const std::string& collectionNS : collections) { | ||
| WriteUnitOfWork wuow(&opCtx); | ||
| UninterruptibleLockGuard noInterrupt(opCtx.lockState()); |
There was a problem hiding this comment.
Remove unnecessary WriteUnitOfWork wrapping read-only iteration.
The WriteUnitOfWork at line 153 wraps collection enumeration, which is a read-only operation. This is unnecessary and could cause issues:
WriteUnitOfWorkis intended for operations that modify data and require transactional guarantees- The iteration only reads collection metadata; no writes occur until line 182
- This could unnecessarily hold locks or create transaction overhead
🔎 Remove the WriteUnitOfWork wrapper
for (const std::string& collectionNS : collections) {
- WriteUnitOfWork wuow(&opCtx);
UninterruptibleLockGuard noInterrupt(opCtx.lockState());
NamespaceString collectionNSS(collectionNS);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/mongo/db/ttl.cpp around lines 152-154, remove the unnecessary
WriteUnitOfWork that currently wraps the read-only collection enumeration; the
loop only reads collection metadata and the first write happens later (around
line 182). Delete the WriteUnitOfWork instantiation before the loop and instead
create a WriteUnitOfWork scoped only around the actual write/update section (the
code at/after line 182), ensuring it is committed/rolled back in that narrower
scope and that any required locks/guards remain intact for the write window.
There was a problem hiding this comment.
Actionable comments posted: 1
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/catalog/collection_impl.cpp (1)
585-619: Add snapshot consistency check.The
updateDocumentmethod (line 649) includes an invariant to ensure snapshot consistency:invariant(oldDoc.snapshotId() == opCtx->recoveryUnit()->getSnapshotId());This
deleteDocumentoverload should include the same check to prevent deletion based on a stale document snapshot. Without this check, if a caller passes a document from a different snapshot, the deletion could operate on outdated data, risking consistency issues.🔎 Proposed fix
void CollectionImpl::deleteDocument(OperationContext* opCtx, StmtId stmtId, const RecordId& loc, const Snapshotted<BSONObj>& doc, OpDebug* opDebug, bool fromMigrate, bool noWarn, Collection::StoreDeletedDoc storeDeletedDoc) { + dassert(opCtx->lockState()->isCollectionLockedForMode(ns().toString(), MODE_IX)); + invariant(doc.snapshotId() == opCtx->recoveryUnit()->getSnapshotId()); + if (isCapped()) { log() << "failing remove on a capped ns " << _ns; uasserted(10089, "cannot remove from a capped collection"); return; }
♻️ Duplicate comments (1)
src/mongo/db/modules/eloq/src/eloq_record_store.cpp (1)
516-521: Remove commented-out code.The commented line at 517 should be removed entirely. If resetting
_lastMongoKeyis no longer needed (as the change suggests), the commented code creates maintenance debt and uncertainty about intended behavior.🔎 Apply this diff to remove the commented code
void saveUnpositioned() override { MONGO_LOG(1) << "EloqRecordStoreCursor::saveUnpositioned " << _tableName->StringView(); - // _lastMongoKey.reset(); if (!_eof && _cursor && _cursor->currentBatchTuple() != nullptr) { _lastMongoKey.emplace(*_cursor->currentBatchTuple()->key_.GetKey<Eloq::MongoKey>()); } _cursor.reset(); }
🧹 Nitpick comments (4)
tests/jstests/eloq_basic/collection_scan_delete.js (2)
26-26: PrefercountDocuments()for consistency.Line 26 uses the deprecated
count()method, while line 70 correctly usescountDocuments(). For consistency and to follow MongoDB best practices, consider usingcountDocuments()here as well.🔎 Suggested change
-print("Inserted documents:", coll.count()); +print("Inserted documents:", coll.countDocuments({}));
58-61: Consider usingdeleteMany()for modern API alignment.The test uses the legacy
remove()method. While valid for MongoDB 4.x, the moderndeleteMany()API is preferred and has identical functionality with the hint option.🔎 Suggested change
-var result = coll.remove( +var result = coll.deleteMany( { k: { $gte: 0 } }, { hint: { $natural: 1 } } );Note: The explain on line 36 should also be updated:
-var explain = coll.explain("executionStats").remove( +var explain = coll.explain("executionStats").deleteMany( { k: { $gte: 0 } }, { hint: { $natural: 1 } } );src/mongo/db/catalog/collection_impl.h (1)
161-169: Consider adding documentation for the new overload.The existing documentation (lines 138-151) describes the deletion semantics but doesn't explain when to use this overload versus the original. Consider adding a brief comment explaining that this overload should be used when the caller already has the document in memory to avoid redundant fetching.
🔎 Suggested documentation
+ /** + * Overload that accepts a pre-fetched document snapshot. Use this when the document + * is already in memory to avoid an additional fetch from the record store. + */ void deleteDocument( OperationContext* opCtx, StmtId stmtId, const RecordId& loc, const Snapshotted<BSONObj>& doc, OpDebug* opDebug, bool fromMigrate = false, bool noWarn = false, Collection::StoreDeletedDoc storeDeletedDoc = Collection::StoreDeletedDoc::Off) final;src/mongo/db/modules/eloq/src/eloq_record_store.cpp (1)
446-448: Consider updating_lastMongoKeyinnext()for robust position tracking.The additional
!_cursor->indexScanIsOpen()check improves defensive programming. However, position tracking currently relies solely onsave()accessingcurrentBatchTuple(). For better robustness during TTL batch operations with yields, consider updating_lastMongoKeyimmediately after extracting the key around line 468:RecordId id = key->ToRecordId(false); if (_lastMongoKey) { _lastMongoKey->Copy(*key); } else { _lastMongoKey.emplace(*key); }This ensures the position is always tracked during iteration, independent of whether
save()can access cursor state.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.clangd(0 hunks)src/mongo/db/catalog/collection.h(2 hunks)src/mongo/db/catalog/collection_impl.cpp(1 hunks)src/mongo/db/catalog/collection_impl.h(1 hunks)src/mongo/db/exec/delete.cpp(1 hunks)src/mongo/db/modules/eloq/src/eloq_record_store.cpp(3 hunks)src/mongo/db/modules/eloq/src/eloq_recovery_unit.cpp(2 hunks)tests/jstests/eloq_basic/collection_scan_delete.js(1 hunks)tests/jstests/eloq_basic/delete_many.js(1 hunks)tests/jstests/eloq_basic/ttl_basic_test.js(1 hunks)
💤 Files with no reviewable changes (1)
- .clangd
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/jstests/eloq_basic/delete_many.js
- src/mongo/db/exec/delete.cpp
- src/mongo/db/modules/eloq/src/eloq_recovery_unit.cpp
- tests/jstests/eloq_basic/ttl_basic_test.js
🧰 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_record_store.cpp
🧬 Code graph analysis (2)
src/mongo/db/catalog/collection.h (1)
src/mongo/db/catalog/collection_impl.cpp (4)
deleteDocument(574-583)deleteDocument(574-580)deleteDocument(585-619)deleteDocument(585-592)
src/mongo/db/catalog/collection_impl.h (2)
src/mongo/db/catalog/collection_impl.cpp (4)
deleteDocument(574-583)deleteDocument(574-580)deleteDocument(585-619)deleteDocument(585-592)src/mongo/db/catalog/collection.h (2)
BSONObj(435-438)StoreDeletedDoc(165-485)
🪛 Biome (2.1.2)
tests/jstests/eloq_basic/collection_scan_delete.js
[error] 8-8: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
🔇 Additional comments (7)
tests/jstests/eloq_basic/collection_scan_delete.js (2)
31-52: LGTM! Explain phase correctly validates COLLSCAN usage.The explain phase properly verifies that the delete operation uses a collection scan before executing the actual delete. The assertion on line 50 ensures the test fails early if the expected plan is not chosen, preventing false positives.
67-72: LGTM! Verification properly fails on error.The verification phase correctly uses
assert.eq()to fail the test when documents remain after deletion. This addresses the previous review concern about the test exiting successfully even when verification failed.src/mongo/db/catalog/collection.h (2)
232-239: LGTM!The pure virtual interface addition is consistent with the existing pattern and correctly declares the new overload with the document parameter.
487-497: LGTM!The inline wrapper correctly forwards to the implementation with the document parameter.
src/mongo/db/catalog/collection_impl.cpp (1)
574-583: LGTM!The wrapper correctly fetches the document snapshot and delegates to the new overload, maintaining clean separation of concerns.
src/mongo/db/modules/eloq/src/eloq_record_store.cpp (2)
476-482: LGTM - Enhanced logging for debugging.The addition of transaction numbers and cursor reset logging improves observability for TTL batch operations and cursor lifecycle debugging.
531-536: Verify lazy cursor restoration is sufficient for TTL operations.The
restore()method is a no-op, relying on lazy cursor recreation innext()via_seekCursor(). While the comment explains deferring scan opening, confirm this approach correctly handles:
- TTL batch delete scenarios where
restore()is called after yielding mid-iteration- Cases where
next()is called immediately afterrestore()- the cursor will be recreated using_lastMongoKey, which should be correct- The contract with MongoDB's RecordCursor interface expectations
This design appears intentional (cursor recreation deferred until needed), but verify it aligns with MongoDB's cursor lifecycle expectations and TTL monitor requirements.
| void save() override { | ||
| MONGO_LOG(1) << "EloqRecordStoreCursor::save"; | ||
| MONGO_LOG(1) << "EloqRecordStoreCursor::save " << _tableName->StringView(); | ||
| if (!_eof && _cursor && _cursor->currentBatchTuple() != nullptr) { | ||
| _lastMongoKey.emplace(*_cursor->currentBatchTuple()->key_.GetKey<Eloq::MongoKey>()); | ||
| } | ||
| _cursor.reset(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for EloqCursor class definition and currentBatchTuple implementation
echo "=== Searching for EloqCursor and relevant methods ==="
rg -n "class EloqCursor|currentBatchTuple|indexScanIsOpen" --type cpp -A 2 | head -100Repository: eloqdata/eloqdoc
Length of output: 6180
Maintain consistent defensive checks when accessing currentBatchTuple().
The save() method accesses currentBatchTuple() without verifying indexScanIsOpen(). While the code checks currentBatchTuple() != nullptr, this is inconsistent with the pattern used in next() (line 446) and eloq_index.cpp (line 205), which explicitly verify indexScanIsOpen() before accessing the cursor. For consistency and clarity, consider adding the defensive check:
if (!_eof && _cursor && _cursor->indexScanIsOpen() && _cursor->currentBatchTuple() != nullptr) {
_lastMongoKey.emplace(*_cursor->currentBatchTuple()->key_.GetKey<Eloq::MongoKey>());
}🤖 Prompt for AI Agents
In src/mongo/db/modules/eloq/src/eloq_record_store.cpp around lines 524 to 529,
the save() method accesses _cursor->currentBatchTuple() without first verifying
indexScanIsOpen(), which is inconsistent with other code paths; update the
conditional to also check _cursor->indexScanIsOpen() before accessing
currentBatchTuple() so you only dereference the cursor when an index scan is
open, maintaining the same defensive pattern used in next() and eloq_index.cpp.
2 participants
Summary by CodeRabbit
New Features
Tests
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.