Skip to content

Move the CreateTxKey and CreateTxRecord function into CatalogFactory to support multi table engines #188

Merged
lzxddz merged 3 commits into
eloqdata:mainfrom
lzxddz:create_tx_key
Sep 24, 2025
Merged

Move the CreateTxKey and CreateTxRecord function into CatalogFactory to support multi table engines #188
lzxddz merged 3 commits into
eloqdata:mainfrom
lzxddz:create_tx_key

Conversation

@lzxddz

@lzxddz lzxddz commented Sep 17, 2025

Copy link
Copy Markdown
Collaborator

Summary by CodeRabbit

  • Refactor

    • Streamlined transaction and catalog initialization to remove duplicate entries and startup side-effects.
    • Consolidated key and record creation paths and made related APIs const-safe for more predictable behavior.
  • Chores

    • Updated internal modules/submodules to newer revisions.
  • Stability

    • Added stronger runtime assertions to enforce engine correctness and improve robustness.

@lzxddz lzxddz self-assigned this Sep 17, 2025
@coderabbitai

coderabbitai Bot commented Sep 17, 2025

Copy link
Copy Markdown

Caution

Review failed

The pull request is closed.

Walkthrough

Updated Eloq submodule pointers; refactored MongoCatalogFactory to require EloqDoc and add const-qualified key/record creators; removed global factory registration in EloqKVEngine and passed a local catalog_factories array into DataStoreServiceClient and related constructors.

Changes

Cohort / File(s) Summary of Changes
Eloq Catalog Factory
src/mongo/db/modules/eloq/src/base/eloq_catalog_factory.h, src/mongo/db/modules/eloq/src/base/eloq_catalog_factory.cpp
Made NegativeInfKey()/PositiveInfKey() const; added CreateTxKey() (two overloads), PackedNegativeInfinity(), and CreateTxRecord(); removed Sequences-specific branches and assert EloqDoc engine; use KeySchema key_version for TemplateCcMap.
KV Engine init & wiring
src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp
Removed global RegisterFactory() and its constructor call; reduced/adjusted local catalog_factories; pass catalog_factories into EloqDS::DataStoreServiceClient and updated related construction sites; introduced local wiring in initDataStoreService.
Submodule pointer updates
src/mongo/db/modules/eloq/log_service, src/mongo/db/modules/eloq/store_handler, src/mongo/db/modules/eloq/tx_service
Advanced submodule commit pointers; no visible code or public API changes in this diff.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant KV as EloqKVEngine
  participant DSC as DataStoreServiceClient
  participant DS as DataStoreService
  Note over KV: initDataStoreService() builds local catalog_factories
  KV->>KV: construct catalog_factories[3]
  KV->>DSC: new DataStoreServiceClient(catalog_factories, ds_config, DS)
  DSC-->>KV: client ready
  Note over KV: Global RegisterFactory() removed — no global registration side-effects
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

Suggested reviewers

  • xiexiaoy
  • thweetkomputer

Poem

I twitch my whiskers at a new key,
Old hooks loosened, tidy and free.
Keys and records hop into place,
Engines wired with a sprightly pace.
Submodules march — a small, proud race. 🐇

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title succinctly and accurately describes the primary change—moving CreateTxKey and CreateTxRecord into the CatalogFactory to support multi-table engines—and aligns with the diffs that add those methods and update callers; it is concise, specific, and focused on the main change.

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 293d7ef and 8101669.

📒 Files selected for processing (3)
  • src/mongo/db/modules/eloq/log_service (1 hunks)
  • src/mongo/db/modules/eloq/store_handler (1 hunks)
  • src/mongo/db/modules/eloq/tx_service (1 hunks)

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/modules/eloq/src/base/eloq_catalog_factory.cpp (1)

50-55: Same here—replace assert with invariant/uassert

Ensure the engine mismatch can’t silently proceed in release builds.

Apply this diff:

-    assert(table_name.Engine() == txservice::TableEngine::EloqDoc);
+    invariant(table_name.Engine() == txservice::TableEngine::EloqDoc);
🧹 Nitpick comments (3)
src/mongo/db/modules/eloq/store_handler (2)

1-1: Rollout/CI checklist for submodules.

  • Ensure CI clones submodules recursively for builds and tests.
  • Add a brief CHANGELOG/relnotes entry referencing Eloq submodule commit 9bfc88d6 and API shifts (CatalogFactory TxKey/TxRecord).
  • Coordinate downstream consumers pinned to the previous Eloq commit to avoid mixed deployments.

1-1: Data/compat sanity check (key versioning).

Upstream summary mentions key_version now sourced from table_schema->KeySchema()->SchemaTs(). Validate that:

  • Existing tables’ schema timestamps map to the expected key versions.
  • Any on-disk readers or migration tools understand the new derivation.

If helpful, I can draft a short migration note covering expected key_version values across common table schemas.

src/mongo/db/modules/eloq/src/base/eloq_catalog_factory.h (1)

20-25: Explicitly include and document pointer lifetime for PackedNegativeInfinity

  • size_t is used in this header; prefer explicitly including to avoid relying on transitive includes.
  • Clarify that the returned pointer is a process-wide static sentinel and must not be freed.

Apply this diff:

 #include <memory>
+#include <cstddef>
 #include <string>
 #include <unordered_map>
 #include <utility>
 #include <vector>
@@
-    const txservice::TxKey* PackedNegativeInfinity() const override;
+    // Returns a pointer to a process-wide static sentinel TxKey. Do not free.
+    const txservice::TxKey* PackedNegativeInfinity() const override;

Also applies to: 90-90

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 78d2334 and faa19e5.

📒 Files selected for processing (6)
  • src/mongo/db/modules/eloq/log_service (1 hunks)
  • src/mongo/db/modules/eloq/src/base/eloq_catalog_factory.cpp (3 hunks)
  • src/mongo/db/modules/eloq/src/base/eloq_catalog_factory.h (1 hunks)
  • src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp (3 hunks)
  • src/mongo/db/modules/eloq/store_handler (1 hunks)
  • src/mongo/db/modules/eloq/tx_service (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/mongo/db/modules/eloq/src/base/eloq_catalog_factory.h (1)
src/mongo/db/modules/eloq/src/base/eloq_catalog_factory.cpp (12)
  • NegativeInfKey (154-156)
  • NegativeInfKey (154-154)
  • PositiveInfKey (158-160)
  • PositiveInfKey (158-158)
  • CreateTxKey (162-164)
  • CreateTxKey (162-162)
  • CreateTxKey (166-168)
  • CreateTxKey (166-166)
  • PackedNegativeInfinity (170-172)
  • PackedNegativeInfinity (170-170)
  • CreateTxRecord (174-177)
  • CreateTxRecord (174-174)
🔇 Additional comments (8)
src/mongo/db/modules/eloq/tx_service (1)

1-1: Submodule bump — in-repo API/ABI checks done; confirm CI & external callers

  • CreateTxKey/CreateTxRecord calls in this repo now use catalog_factory->CreateTxKey / CreateTxRecord (examples: src/mongo/db/modules/eloq/store_handler/data_store_service_client.cpp:2136–2144; data_store_service_client_closure.cpp:633–635, 981–985, 1047–1050).
  • NegativeInfKey()/PositiveInfKey() are present and const in catalog factories (src/mongo/db/modules/eloq/src/base/eloq_catalog_factory.cpp:154–160; src/mongo/db/modules/eloq/tx_service/src/eloq_basic_catalog_factory.cpp:180–187).
  • No RegisterFactory references found.
  • TxService ctor now requires CatalogFactory* catalog_factory[3] (src/mongo/db/modules/eloq/tx_service/include/tx_service.h:1101–1102); no TxService instantiations found in this repo — verify external consumers (and any other repos) are updated for the new TxService/DataStoreServiceClient ctor signatures.
  • Ensure CI runs git submodule update --init --recursive (or pins a tagged upstream commit) before build.
src/mongo/db/modules/eloq/log_service (1)

1-1: Submodule bump verified — remote commit present; CI and API compatibility must be confirmed.

  • Location: src/mongo/db/modules/eloq/log_service
  • Commit 050631894d8a9fc098f2f162d8de38deea421509 exists and is reachable on remote branch origin/create_tx_key (commit date: Sep 16, 2025).
  • git submodule update --init --recursive checked out the submodule and its nested tx-log-protos (3c3a1cf5).
  • Action: ensure CI fetches submodules recursively (e.g., actions/checkout@v4 with submodules: recursive, fetch-depth: 0) and manually confirm the bumped log_service is compatible with the Eloq catalog factory API changes referenced in the PR.
src/mongo/db/modules/eloq/store_handler (1)

1-1: Approve: submodule bump verified — CatalogFactory APIs in use, no external RegisterFactory

  • RegisterFactory: no occurrences outside src/mongo/db/modules/eloq (repo search returned none).
  • NegativeInfKey/PositiveInfKey: declared as const methods in src/mongo/db/modules/eloq/src/base/eloq_catalog_factory.h and callers in tx_service/store_handler invoke catalog_factory->NegativeInfKey()/PositiveInfKey(), matching the const-qualified signatures.
  • CreateTxKey/CreateTxRecord: callers use CatalogFactory::CreateTxKey/CreateTxRecord across eloq (e.g. store_handler data_store_service_scanner.cpp:450–451, data_store_service_client.cpp:2136/2144/2301).
src/mongo/db/modules/eloq/src/base/eloq_catalog_factory.h (1)

85-91: Const-correct factory API additions look good

Signatures align with the cpp. No behavioral concerns.

src/mongo/db/modules/eloq/src/base/eloq_catalog_factory.cpp (1)

154-177: TxKey/TxRecord factory implementations look correct

  • Const-qualification matches the header.
  • TxKey by-value return should move the unique_ptr payload efficiently.
  • PackedNegativeInfinity forwarding is clear.
src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp (3)

492-492: ```shell
#!/bin/bash
set -euo pipefail

echo "== Searching repo for TableEngine symbols =="
rg -n --hidden -S '\b(TableEngine|txservice::TableEngine)\b' || true

echo "== Searching for enum/count/max/constants related to TableEngine =="
rg -n --hidden -S 'enum\s+(class\s+)?TableEngine|TableEngine::(Count|Max)|kTableEngineCount|TABLE_ENGINE_COUNT|TableEngineCount' || true

echo "== Searching for txservice, CatalogFactory, EloqDoc references =="
rg -n --hidden -S '\btxservice\b|\bCatalogFactory\b|EloqDoc' || true

echo "== Showing matches and nearby context in eloq_kv_engine.cpp if present =="
if [ -f src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp ]; then
echo "File found: src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp"
rg -n -C3 'CatalogFactory|TableEngine|EloqDoc' src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp || true
echo "--- File head (first 200 lines) ---"
sed -n '1,200p' src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp || true
else
echo "File not found: src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp"
fi


---

`553-554`: **Same lifetime risk for DataStoreServiceClient call — verify copy or persist storage**

You pass a stack array (src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp:553-554; also 701-702) — `txservice::CatalogFactory* catalog_factories[3] = {nullptr, nullptr, &_catalogFactory};` — confirm DataStoreServiceClient makes its own copy of the array; if it retains the pointer, make the storage member-backed and pass catalog_factories.data() so it outlives the call.

---

`492-509`: **Verify ownership of catalog_factories — possible dangling pointer**

File: src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp (lines 492–509)

txservice::CatalogFactory* catalog_factories[3] = {nullptr, nullptr, &_catalogFactory}; is allocated on the stack and passed to TxService. If TxService stores the pointer rather than copying the array contents, that pointer will dangle after the constructor returns. Confirm TxService's constructor/field semantics (copy vs. retain). Automated search for TxService under mongo/db/modules/eloq/tx_service returned no results; run from the repo root:
rg -nP '\bTxService\b|TxService\s*\(' -S -n -C3
and verify whether the ctor copies the array or keeps the pointer.

If the ctor retains the pointer, change to a member-backed array or pass a std::array by value. Example fix (keeps semantics, extends lifetime):

```diff
-    txservice::CatalogFactory* catalog_factories[3] = {nullptr, nullptr, &_catalogFactory};
-    _txService = std::make_unique<txservice::TxService>(catalog_factories, ...
+    // e.g., in the class: std::array<txservice::CatalogFactory*, txservice::kTableEngineCount> _catalogFactories{};
+    _catalogFactories.fill(nullptr);
+    _catalogFactories[static_cast<size_t>(txservice::TableEngine::EloqDoc)] = &_catalogFactory;
+    _txService = std::make_unique<txservice::TxService>(_catalogFactories.data(), ...

Comment on lines +40 to 41
assert(table_name.Engine() == txservice::TableEngine::EloqDoc);
return std::make_unique<MongoTableSchema>(table_name, catalog_image, version);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Don’t use assert for runtime invariants in production paths

assert() compiles out in release builds; prefer invariant()/uassert to fail fast with diagnostics.

Apply this diff:

-    assert(table_name.Engine() == txservice::TableEngine::EloqDoc);
+    invariant(table_name.Engine() == txservice::TableEngine::EloqDoc);

And add the required include near the other includes:

+#include "mongo/util/assert_util.h"
📝 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.

Suggested change
assert(table_name.Engine() == txservice::TableEngine::EloqDoc);
return std::make_unique<MongoTableSchema>(table_name, catalog_image, version);
#include "mongo/util/assert_util.h"
invariant(table_name.Engine() == txservice::TableEngine::EloqDoc);
return std::make_unique<MongoTableSchema>(table_name, catalog_image, version);
🤖 Prompt for AI Agents
In src/mongo/db/modules/eloq/src/base/eloq_catalog_factory.cpp around lines
40-41, replace the C assert used for a runtime invariant with a production-safe
check and add the required include: change "assert(table_name.Engine() ==
txservice::TableEngine::EloqDoc);" to use invariant (e.g.
"invariant(table_name.Engine() == txservice::TableEngine::EloqDoc);") or uassert
with an appropriate error code/message, and add #include
"mongo/util/assert_util.h" alongside the other includes. Ensure the new check
remains in release builds and provides a clear diagnostic on failure.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (2)
src/mongo/db/modules/eloq/src/base/eloq_catalog_factory.cpp (2)

33-34: Remove unused include

sequences/sequences.h appears unused after refactor; drop it to reduce coupling.

-#include "mongo/db/modules/eloq/tx_service/include/sequences/sequences.h"

62-69: Avoid unsafe downcast in CreateSkCcMap

static_cast to MongoTableSchema* is unchecked. Either verify engine and keep static_cast with an invariant, or use dynamic_cast (if RTTI enabled).

Minimal guard:

-    const auto* mongo_table_schema = static_cast<const MongoTableSchema*>(table_schema);
-    if (mongo_table_schema != nullptr) {
+    invariant(table_schema && table_schema->TableName().Engine() == txservice::TableEngine::EloqDoc);
+    const auto* mongo_table_schema = static_cast<const MongoTableSchema*>(table_schema);
+    {
         uint64_t key_version = table_schema->IndexKeySchema(table_name)->SchemaTs();
         return std::make_unique<txservice::TemplateCcMap<MongoKey, MongoRecord, true, true>>(
             shard, cc_ng_id, table_name, key_version, table_schema, false);
     }
-    return nullptr;
+    MONGO_UNREACHABLE;
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 635bad3 and 37d0206.

📒 Files selected for processing (6)
  • src/mongo/db/modules/eloq/log_service (1 hunks)
  • src/mongo/db/modules/eloq/src/base/eloq_catalog_factory.cpp (3 hunks)
  • src/mongo/db/modules/eloq/src/base/eloq_catalog_factory.h (1 hunks)
  • src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp (3 hunks)
  • src/mongo/db/modules/eloq/store_handler (1 hunks)
  • src/mongo/db/modules/eloq/tx_service (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • src/mongo/db/modules/eloq/log_service
  • src/mongo/db/modules/eloq/store_handler
  • src/mongo/db/modules/eloq/tx_service
🧰 Additional context used
🧬 Code graph analysis (1)
src/mongo/db/modules/eloq/src/base/eloq_catalog_factory.h (1)
src/mongo/db/modules/eloq/src/base/eloq_catalog_factory.cpp (12)
  • NegativeInfKey (154-156)
  • NegativeInfKey (154-154)
  • PositiveInfKey (158-160)
  • PositiveInfKey (158-158)
  • CreateTxKey (162-164)
  • CreateTxKey (162-162)
  • CreateTxKey (166-168)
  • CreateTxKey (166-166)
  • PackedNegativeInfinity (170-172)
  • PackedNegativeInfinity (170-170)
  • CreateTxRecord (174-177)
  • CreateTxRecord (174-174)
🔇 Additional comments (6)
src/mongo/db/modules/eloq/src/base/eloq_catalog_factory.h (1)

85-92: Const-correct factory API additions look good

The added const-qualified overrides align with the intended usage.

Please confirm txservice::CatalogFactory declares these exact signatures to avoid accidental non-override (ODR/ABI) issues.

src/mongo/db/modules/eloq/src/base/eloq_catalog_factory.cpp (3)

18-24: Don’t use assert for runtime invariants in production paths

Use invariant(...) so checks remain in release builds and produce diagnostics.

Apply:

+#include "mongo/util/assert_util.h"
@@
-    assert(table_name.Engine() == txservice::TableEngine::EloqDoc);
+    invariant(table_name.Engine() == txservice::TableEngine::EloqDoc);
@@
-    assert(table_name.Engine() == txservice::TableEngine::EloqDoc);
+    invariant(table_name.Engine() == txservice::TableEngine::EloqDoc);
@@
-    assert(range_table_name.Type() == txservice::TableType::RangePartition);
+    invariant(range_table_name.Type() == txservice::TableType::RangePartition);
@@
-    assert(start_key.Type() == txservice::KeyType::NegativeInf || start_key.IsOwner());
+    invariant(start_key.Type() == txservice::KeyType::NegativeInf || start_key.IsOwner());
@@
-    assert(start != nullptr);
+    invariant(start != nullptr);
@@
-    assert(range_table_name.Type() == txservice::TableType::RangePartition);
+    invariant(range_table_name.Type() == txservice::TableType::RangePartition);

Also applies to: 40-40, 50-50, 77-77, 87-87, 91-91, 130-130


154-177: LGTM: new TxKey/TxRecord factory methods

Straightforward and consistent with MongoKey/MongoRecord wrappers.


170-172: Confirm lifetime of PackedNegativeInfinity pointer

Ensure MongoKey::PackedNegativeInfinityTxKey() returns a stable, process-lifetime object (e.g., function-static), not a temporary.

src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp (2)

701-703: Confirm new DataStoreServiceClient signature and pointer ownership

Constructor now takes catalog_factories first — verify all call sites were updated and that the client does not retain a pointer to the caller's stack array. Call site: src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp:701-702 (catalog_factories defined at src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp:552). If the client stores the raw pointer, change the API or caller so ownership/lifetime are safe (copy/own or use heap/shared ownership).


491-507: Verify factory index mapping matches TableEngine enum

TableEngine's declaration isn't present in the checked-in tx_service headers, so I can't confirm that catalog_factories[3] with element at index 2 maps to txservice::TableEngine::EloqDoc. Use enum-indexed access (e.g. catalog_factories[static_cast<size_t>(txservice::TableEngine::EloqDoc)] = &_catalogFactory) or add an assert/documented mapping; update both occurrences in src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp (around lines 491 and 552).

Comment on lines +491 to 492
txservice::CatalogFactory* catalog_factories[3] = {nullptr, nullptr, &_catalogFactory};
_txService = std::make_unique<txservice::TxService>(catalog_factories,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Make catalog_factories static to avoid dangling-pointer risk

Both arrays are stack-allocated and their addresses are passed into long-lived services. If either service stores the pointer instead of copying, this becomes a UAF. Make them function-static to guarantee lifetime.

Apply this diff in both places:

-    txservice::CatalogFactory* catalog_factories[3] = {nullptr, nullptr, &_catalogFactory};
+    static txservice::CatalogFactory* catalog_factories[3] = {nullptr, nullptr, &_catalogFactory};

Also applies to: 553-553

🤖 Prompt for AI Agents
In src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp around lines 491-492 (and
also at line 553), the local array catalog_factories is stack-allocated and its
address is passed into long-lived services, risking a dangling-pointer UAF;
change the declaration to a function-static array (e.g., static
txservice::CatalogFactory* catalog_factories[3] = {nullptr, nullptr,
&_catalogFactory};) so the array has static storage duration and won’t go out of
scope before the services finish using it.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp (1)

701-703: Constructor wiring change: verify engine-index mapping and lifetime.

  • Confirm DataStoreServiceClient expects a pointer to an array of exactly 3 factories and does not require a terminator/size parameter.
  • Ensure index positions align with txservice::TableEngine enum values. Consider a helper to map enum→index to avoid order brittleness.
  • If you don’t make the array static, ensure the client copies the array, or pass a member-owned array’s .data().
src/mongo/db/modules/eloq/src/base/eloq_catalog_factory.cpp (1)

50-50: Avoid assert for runtime invariants across the factory.

These checks guard runtime contracts; keep them enabled in production.

Apply:

-    assert(table_name.Engine() == txservice::TableEngine::EloqDoc);
+    invariant(table_name.Engine() == txservice::TableEngine::EloqDoc);
-    assert(range_table_name.Type() == txservice::TableType::RangePartition);
+    invariant(range_table_name.Type() == txservice::TableType::RangePartition);
-    assert(start_key.Type() == txservice::KeyType::NegativeInf || start_key.IsOwner());
+    invariant(start_key.Type() == txservice::KeyType::NegativeInf || start_key.IsOwner());
-    assert(start != nullptr);
+    invariant(start != nullptr);
-    assert(range_table_name.Type() == txservice::TableType::RangePartition);
+    invariant(range_table_name.Type() == txservice::TableType::RangePartition);

And ensure:

#include "mongo/util/assert_util.h"

is included near the top.

Also applies to: 77-77, 87-87, 91-91, 130-130

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 37d0206 and 293d7ef.

📒 Files selected for processing (6)
  • src/mongo/db/modules/eloq/log_service (1 hunks)
  • src/mongo/db/modules/eloq/src/base/eloq_catalog_factory.cpp (3 hunks)
  • src/mongo/db/modules/eloq/src/base/eloq_catalog_factory.h (1 hunks)
  • src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp (3 hunks)
  • src/mongo/db/modules/eloq/store_handler (1 hunks)
  • src/mongo/db/modules/eloq/tx_service (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • src/mongo/db/modules/eloq/tx_service
  • src/mongo/db/modules/eloq/log_service
  • src/mongo/db/modules/eloq/store_handler
🧰 Additional context used
🧬 Code graph analysis (1)
src/mongo/db/modules/eloq/src/base/eloq_catalog_factory.h (1)
src/mongo/db/modules/eloq/src/base/eloq_catalog_factory.cpp (12)
  • NegativeInfKey (154-156)
  • NegativeInfKey (154-154)
  • PositiveInfKey (158-160)
  • PositiveInfKey (158-158)
  • CreateTxKey (162-164)
  • CreateTxKey (162-162)
  • CreateTxKey (166-168)
  • CreateTxKey (166-166)
  • PackedNegativeInfinity (170-172)
  • PackedNegativeInfinity (170-170)
  • CreateTxRecord (174-177)
  • CreateTxRecord (174-174)
🔇 Additional comments (5)
src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp (2)

491-491: Avoid dangling pointer: make the local factory array non-ephemeral (static or member-owned).

This stack-allocated array’s address is passed into long‑lived services; if they retain the pointer, it becomes a UAF after the constructor returns. Make it function‑static (if EloqKVEngine is effectively a singleton) or store it as a member and pass .data().

Minimal change (matches prior suggestion):

-    txservice::CatalogFactory* catalog_factories[3] = {nullptr, nullptr, &_catalogFactory};
+    static txservice::CatalogFactory* catalog_factories[3] = {nullptr, nullptr, &_catalogFactory};

Safer alternative (member-owned) — add a member std::array<txservice::CatalogFactory*, 3> _catalogFactories; and do:

-    txservice::CatalogFactory* catalog_factories[3] = {nullptr, nullptr, &_catalogFactory};
+    _catalogFactories[0] = nullptr;
+    _catalogFactories[1] = nullptr;
+    _catalogFactories[2] = &_catalogFactory;

And pass _catalogFactories.data() where used.


552-552: Same UAF risk for the DSS client path; apply the same fix here.

This local array may outlive the function if the client holds the pointer. Make it static or reuse a member-owned array as suggested above.

-    txservice::CatalogFactory* catalog_factories[3] = {nullptr, nullptr, &_catalogFactory};
+    static txservice::CatalogFactory* catalog_factories[3] = {nullptr, nullptr, &_catalogFactory};
src/mongo/db/modules/eloq/src/base/eloq_catalog_factory.h (1)

85-92: LGTM: const-correct factory API additions.

The new const-qualified overrides for NegativeInfKey/PositiveInfKey and TxKey/TxRecord creators look consistent.

src/mongo/db/modules/eloq/src/base/eloq_catalog_factory.cpp (2)

40-40: Replace assert with invariant/uassert for production safety.

assert compiles out in release builds; use invariant so violations are caught with diagnostics.

-    assert(table_name.Engine() == txservice::TableEngine::EloqDoc);
+    invariant(table_name.Engine() == txservice::TableEngine::EloqDoc);

Add include (once):

+#include "mongo/util/assert_util.h"

154-177: LGTM: new const-qualified key/record factory methods.

  • TxKey creation via unique_ptr is clear and transfers ownership correctly.
  • PackedNegativeInfinity returning a const TxKey* is fine; verify it points to a process‑lifetime object.

@xiexiaoy xiexiaoy self-requested a review September 23, 2025 10:58
@lzxddz lzxddz merged commit bd60381 into eloqdata:main Sep 24, 2025
1 of 2 checks passed
zhangh43 pushed a commit to zhangh43/eloqdoc that referenced this pull request Sep 28, 2025
…to support multi table engines (eloqdata#188)

* Move CreateTxKey and CreaetTxRecord to CatalogFactory

* update submoudles
@coderabbitai coderabbitai Bot mentioned this pull request Nov 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants