[RFC] MongoDB Storage Backend - #207
Conversation
|
Hi @diegotoledano95, Thank you for the contribution RFC. The RFC looks great, but I did find some gaps while reviewing the reference implementation. Below are the findings, ranging from critical to minor. Please let us know if you need any of the below clarified, we'd be happy to help. Critical FindingsC1. GSI index collection uses GSI keys as document
|
|
The fixes for the gaps detailed in the comment above have been done and are ready for review. You can find them in the The RFC presented in this PR has also been changed to reflect those changes. The |
|
Thanks for the substantial revision. All 11 Critical and 15 Major findings from the first pass are addressed. The core data-plane design is sound: transactional GSI + stream propagation, the netstring composite Conformance (verified live)
Blocking1. Table 2. Binary
The string path already handles this correctly via 3. Field-vs-field conditions evaluate backwards.
Fix: mark 4. Rebase required The branch is based on 5. Steering violations in Non-blocking (worth tracking at merge)
|
|
@LeeroyHannigan Acknowledging the feedback, will start working on the blocking issues. Quick question, do you want to continue reviewing the code as we have on the forked branch? Or do you want to start adding the code here in the current PR or a new PR? |
|
Thanks @diegotoledano95 Would be great to get it here, along with your intended CI. #218 does change how backends register, so if you want to wait until we merge that in, make those changes on your fork and then push here, might be cleanest. |
|
@LeeroyHannigan Will do thanks! Would you have an ETA on #218 ? |
|
@diegotoledano95 #218 has just landed. That should unblock you. |
5f7700d to
947d2a2
Compare
|
@LeeroyHannigan Have pushed the changes for the blocking issues above, and put the code in this branch and PR as requested too. Please let me know what you think, thanks! |
947d2a2 to
d9fe4da
Compare
|
Thanks @diegotoledano95 for turning these around quickly, and for the mutation-checked tests that came with them. I rebuilt from Gates
Confirmed fixed, and re-proven live with the two wire-level tests from last round:
One functional bug I would like fixed before mergeRestore reports ACTIVE before the data copy finishes (
with a 250 ms poll cadence. Nothing in that path references the copy, so ACTIVE does not imply the restore is complete. The comment at Empirically it is load-dependent, which is what makes it easy to miss:
So a green run here is evidence of a lucky schedule rather than of correctness, and the existing conformance test cannot catch it because a tiny backup always finishes inside the window. A client doing wait-for-ACTIVE against a production-sized restore reads an empty table. Suggested fix: set ACTIVE, or schedule the transition, only after Two gaps worth closing in the same pass
One related note if you do wire the harness into CI: Smaller items
The two wire-proven bugs from last round are properly fixed, the CREATING modelling is sound for CreateTable, and the full integration suite is green apart from the restore case. Requesting changes on the restore race and the fmt gate, with the CI and test-coverage gaps strongly recommended alongside. |
Architecture design for the extenddb-storage-mongodb crate covering: - Collection schema (catalog_db + data_db) - Document structure (_id, pk, sk_*, item_data) - Concurrency model (transactions + optimistic versioning) - GSI synchronous propagation strategy - Stream record storage - Bootstrapper and configuration
Implements the full TableEngine, DataEngine, MetadataEngine, StreamEngine, BackupEngine, WorkerStore, and catalog traits against MongoDB 6.0+. Key design decisions: - Single-item writes (put/delete/update) use MongoDB transactions with snapshot read concern and majority write concern for atomicity - Stream records and GSI sync are in the same transaction as the data write - UpdateItem uses optimistic concurrency (_v version field) with session reuse across retries for performance under contention - Condition expressions compiled to MongoDB query filters via condition.rs - Numbers stored as strings in item_data to preserve DynamoDB 38-digit decimal precision - Binary sort key begins_with uses post-fetch filtering (BSON Binary comparison sorts by length first, making $gte/$lt unreliable for prefix matching) - Simple unconditional SET/REMOVE updates use native MongoDB operators via findOneAndUpdate for lower latency Wiring: adds mongodb feature flag to bin crate, registers backend via inventory, and generalizes cmd_serve backend validation. Requires: MongoDB 6.0+ configured as a replica set (even single-node) for multi-document transactions and snapshot reads.
- extenddb-mongo.toml: integration test config for MongoDB backend using ~/.extenddb/tls paths (portable) and enforce_reserved_keywords=true - extenddb.sample.toml: add [storage.mongodb] section - devtools/run-tests: export EXTENDDB_CONFIG; only set EXTENDDB_TEST_PG_CONNECTION_STRING for postgres URLs
- docs/local-mongodb-setup.md: MongoDB installation and replica set setup - docs/getting-started.md: add MongoDB build/init instructions - AGENTS.md: update architecture, prerequisites, pitfalls for MongoDB
stream_engine.rs and data_engine.rs wrote the shadow event_name column
via format!("{:?}", record.event_name), producing "Insert" / "Modify" /
"Remove". DynamoDB Streams' wire contract is uppercase: "INSERT" /
"MODIFY" / "REMOVE".
Add event_name_ddb_str() in stream_engine.rs to map StreamEventName to
its wire-format string, and use it at both call sites.
Unit test asserts each enum variant maps to the expected uppercase
string.
DynamoDB rejects a KeyConditionExpression sk BETWEEN :lo AND :hi with :lo > :hi as ValidationException. The engine layer's condition evaluator does this check for filter/condition expressions, but the KeyConditionExpression path in Query goes through the storage backend's sort-key filter builder, which was emitting $gte lo, $lte hi with no check — matching zero documents without an error. Add the check in build_sk_filter. Comparison is done in the AttributeValue domain before Decimal128/f64 conversion. Numeric comparison uses f64 for ordering only; values that would lose Decimal128 precision are rejected downstream in sk_to_bson. Unit tests cover S, N, and B ordering.
…lures Previously all four TransactWriteItems condition-failure sites in data_engine.rs (Put, Delete, Update, ConditionCheck) called condition_check_failed_with_item(None), discarding the pre-existing item that had already been loaded into scope. Callers that set ReturnValuesOnConditionCheckFailure=ALL_OLD on their transact op therefore got a CancellationReason with no Item field — inconsistent with DynamoDB, which returns the failing item under that flag. Thread return_values_on_ccf from TransactWriteOp through the mongo crate's OwnedTransactWriteOp, and add a small helper ccf_return_item that gates inclusion on both (a) ALL_OLD requested and (b) the item actually existed. Preserves DDB's guarantee that missing items never manifest as CancellationReason.Item. Adds unit tests for the helper covering all three code paths.
The mongo backend stores numeric partition/sort keys as BSON Decimal128
for correct numeric ordering. Decimal128 supports 34 significant
decimal digits; DynamoDB supports up to 38.
Previously the write path (data/mod.rs::item_to_document), key-filter
path (data/mod.rs::pk_filter), and sort-key comparison path
(data_engine.rs::sk_to_bson) all fell back to f64 on Decimal128 parse
failure. f64 has ~15 digits of precision, so values in the 35-38 digit
range were silently truncated, breaking numeric ordering guarantees on
sort keys (e.g. Query with ScanIndexForward could return items in an
order that disagrees with the callers numeric interpretation).
Reject values that exceed Decimal128 precision at all three sites with
a ValidationException explaining the limit. Document as a
MongoDB-backend-specific behavioral difference in
docs/differences-from-dynamodb.md.
Numbers in non-key attribute positions are unaffected: item_data
stores the DynamoDB number string verbatim inside the {"N": ...} tag
and is never numerically compared by the backend.
Adds unit tests for the write path and pk_filter path at the
34-digit boundary and beyond.
Long function bodies and lint-boundary formatting picked up by cargo fmt after the preceding four fix commits. No behavior change.
…ne.rs
Nested `if let Some(ref sk_cond) = key_condition.sk_condition` around
`if let SortKeyCondition::BeginsWith { .. } = sk_cond` collapsed into
a single pattern. Behavior identical.
Upstream pinned a stricter Rust toolchain in 6c59a25, whose clippy is stricter about the collapsible_if and collapsible_match lints. 16 sites in the original mongo backend contribution now trip these lints: - authorization_store.rs (1 site) - data_engine.rs (11 sites) - metadata_engine.rs (4 sites) Mechanical fix — every site is 'if outer { if inner { ... } }' collapsed to 'if outer && inner { ... }', or the equivalent 'if let' pattern. Applied via 'cargo clippy --fix -p extenddb-storage-mongodb', followed by 'cargo fmt --all' to re-align the resulting blocks. Behavior unchanged. 24 unit tests still pass.
…letes restore_table_from_backup created the table with a scheduled CREATING -> ACTIVE transition, then ran the $out copy. The transition is a wall-clock timer (now + control_plane_delay_seconds), not tied to the copy, so on a large restore the table went ACTIVE while $out was still running and a client waiting for ACTIVE could read an empty table. Add a defer_active flag to create_table_impl so the restore path creates the table CREATING with no scheduled transition, and set the table ACTIVE directly once $out drains. ACTIVE now implies the copy is complete by code ordering, not timing. No control-plane delay is applied on restore -- the copy is itself the CREATING window (unlike CreateTable, whose instant work needs a synthetic delay). Removes the now-inaccurate comment.
- Fail closed when the encryption key is missing: loading it with unwrap_or_default() made a missing key an empty string, which panics in aes_gcm (32-byte key required). Return MissingEncryptionKey, like postgres. - Apply the readPreference=primary rejection to every client via a shared connect_guarded(); previously only the data client was guarded, so the catalog/auth/settings/diagnostics/bootstrapper clients bypassed it. Gate the no-TLS warning to the server data client so short-lived CLI/management clients dont emit it -- it was leaking onto command stdout that tooling parses (it corrupted the settings value read by the GSI-async tests).
connection_string may carry user:pass@ credentials; a Serialize impl let them leave the process on any serialize path. Drop the derive (nothing serializes the config), matching postgres which derives only Debug, Clone, Deserialize.
restore_table_from_backup looked up the backup by ARN with no account predicate. The engine layer already enforces ARN ownership, so this is defence-in-depth, aligning restore with the account-scoped describe/delete backup paths.
CONTAINER_NAME and the default OUTPUT_DIR were shared across runs, so two concurrent invocations (even on different ports) would docker rm -f each others mongo and overwrite logs. Derive the container name from the mongo port and the output dir from the port plus PID so runs stay isolated.
Cover DynamoDB wire behaviors our MongoDB fixes touched that the suite
did not otherwise pin:
- begins_with on a binary sort key by unsigned byte prefix, plus the
all-0xFF upper-bound overflow edge and the empty-prefix whole-partition
edge.
- Condition expressions whose comparison operands are both document
paths (field-vs-field), evaluated as stored values.
Both files are dual-target, so PostgreSQL and real DynamoDB run them too.
Run the MongoDB pytest and rust integration suites as parallel jobs joined by a gate, mirroring integration.yml. Each job delegates to devtools/run-mongodb-tests, which bootstraps the single-node replica set (rs.initiate + wait-for-PRIMARY) that GitHub services: cannot express, then reuses the exact local test path to avoid CI/dev drift.
The backfill loops empty-but-not-done branch returned Ok(()) silently, leaving the index in CREATING to be retried each interval. That path should not occur (backfill_gsi_batch marks done when it scans fewer than batch_size docs), so emit a warn instead of failing closed silently — a persistent occurrence now surfaces as a GSI stuck in CREATING.
The sort-key BETWEEN inversion guard compares numeric bounds via f64. f64 rounding is monotonic, so a valid range is never wrongly rejected; the only gap is a genuinely inverted range distinguishable only beyond f64s ~15-17 significant digits, which returns an empty result instead of DynamoDBs ValidationException. Spell out the boundary in the code comment and record it in differences-from-dynamodb.md.
The RFC and design doc claimed integration tests run as `cargo test -p extenddb-storage-mongodb` and described a CI job that did not match reality. Update both to describe the actual setup: the dual-target tests/rust suite and pytest run via devtools/run-mongodb-tests from .github/workflows/integration-mongodb.yml. Also bump the two remaining "6.0" minimum-version references in the design doc to 7.0.
The reviewer asked for a multi-byte all-0xFF case; the prior test used a single-byte [0xFF] prefix. Switch it to [0xFF,0xFF] and add a longer [0xFF,0xFF,0x00] key so the no-upper-bound range is shown to include longer 0xFFFF-prefixed keys while excluding [0xFF,0x00].
8fe9f7e to
e759498
Compare
|
@LeeroyHannigan Thanks for the thorough second pass — the load-dependent restore repro in particular was exactly the kind of thing a green run hides. Rebased onto latest main and pushed. Point by point: Gates
Functional bug — restore reports ACTIVE before the copy finishes Fixed (commit "set restored table ACTIVE only after the data copy completes"). Restore no longer relies on the wall-clock transition: create_table is called with the transition deferred, the $out copy runs, and only then is the row set ACTIVE directly (no timer). ACTIVE now implies the copy has drained, by ordering rather than by timing assumption. The misleading comment is gone, and your reproducing scenario is covered by the committed test "cover restore reporting ACTIVE before the data copy completes" — thanks for offering it; the tree includes an equivalent concurrent-observer test. Gap — nothing in CI exercises the backend Added .github/workflows/integration-mongodb.yml (commit "ci(mongodb): add MongoDB integration workflow"). It mirrors integration.yml: a pytest job and a rust-integration job run in parallel, each building with --features mongodb and delegating to devtools/run-mongodb-tests, joined by a gate job. The orchestrator does the replica-set bootstrap (rs.initiate + wait-for-PRIMARY) that GitHub services: can't express, so CI runs the exact path used locally. Gap — no mongo-specific Rust integration tests Added dual-target tests/rust/ cases (commits "binary begins_with edges and field-vs-field conditions" and "use a multi-byte all-0xFF begins_with prefix"): binary begins_with by unsigned byte prefix, empty-prefix (whole partition), multi-byte all-0xFF, and field-vs-field condition comparisons. These run against Postgres and Mongo, so the fixes are now regression-protected on both backends. run-mongodb-tests container/output collision Fixed (commit "isolate run-mongodb-tests container and output per run"): the container name and output dir now derive from the mongo port (plus PID for the dir), so concurrent invocations no longer tear each other down or share a log. Smaller items
Full suite re-run through devtools/run-mongodb-tests against MongoDB 7 before pushing: rust integration 414/414, comprehensive 330/330, pytest 920 passed. The one pytest failure is TestAtomicCounter::test_atomic_counter hitting ProvisionedThroughputExceededException under concurrent load with throttling enabled — pre-existing, unrelated to this PR (the branch doesn't touch that test). Happy to iterate further on the BETWEEN edge or anything else. |
Resolve conflicts from the in-tree SQLite backend by adopting mains mutually-exclusive, one-backend-per-binary model: add `mongodb` to the compile_error guards and set_backend arm, make it an optional dep. Adapt the MongoDB backend to mains evolved storage traits (default_account_id; ServerComponentsOptions on the server-components factory). MongoDB now builds with `--no-default-features --features mongodb`; update CI, docs, and run-mongodb-tests accordingly.
|
@LeeroyHannigan A note on the three red CI jobs — none are in the MongoDB backend; the cross-backend CI and my dual-target test are surfacing pre-existing issues in the other backends. run-rust-integration (PostgreSQL) — restored_table_has_all_items_when_first_active: The dual-target restore-completeness test I added runs against Postgres and fails (1379/40000 at first-ACTIVE). It's the same restore race you flagged for MongoDB, in the Postgres backend: storage-postgres/src/backup_engine.rs calls create_table (:454), which schedules the CREATING→ACTIVE transition on a timer, then copies items one INSERT at a time (:480-501); with 40k items the copy outlasts the delay, so the control-plane worker flips ACTIVE mid-copy (the explicit ACTIVE at :520 just races it). MongoDB passes this test after my fix. Happy to apply the same ordering fix to Postgres in this PR or leave it to you — and let me know if you'd rather I hold the dual-target test back until Postgres is fixed. run-integration-sqlite (pytest) — two SQLite GSI tests:
I reproduced both SQLite tests on a clean main (d6afa1e) SQLite build in a separate worktree — they fail there independently of this PR. This PR touches neither the SQLite backend nor the shared GSI path; its only shared change is an additive StorageError::TransactionConflict variant + its engine mapping (RFC-0003 §4.3), which only MongoDB produces and is inert in the SQLite binary. So: my PR's CI is red, but on pre-existing bugs in the Postgres and SQLite backends. Let me know how you'd like to proceed — particularly whether the Postgres restore fix belongs in this PR. |
|
Brilliant @diegotoledano95 , thank you! The one flake, and the one ask. The single first-run failure was The red CI jobs are ours. Your dual-target tests surfaced three pre-existing bugs in our backends, and rather than asking you to gate the tests, we've fixed our side: #239 (GSI pagination tiebreaker, covers SQLite and Postgres), #245 (SQLite honored a stale cached GSI propagation delay for up to 30s, so the zero-delay synchronous path never engaged), and #246 (Postgres had the same restore race you fixed here: ACTIVE flipped on a timer decoupled from the copy; reproduced at 515/40000 items). Once those three land, a rebase should take this PR fully green with no changes on your side beyond the oracle fix above. I thought our fixes where already in for our own backends, but they must have got lost in the noise somewhere. |
…cating The restore-completeness observer swallowed scan errors (Err(_) => break) and returned the partial count, so a transient scan blip was misreported as missing data — a CI flake. Per the reviewers analysis the server-side CREATING->ACTIVE transition is atomic and correct; the defect was the test oracle. Retry the count scan (cursor cloned, not taken, so a retry re-scans the same page) and fail loudly if a page still errors after the budget, rather than under-counting.
|
@LeeroyHannigan Thanks so much for the response! I have pushed the requested change on the test. I will keep an eye on those PRs landing to rebase this and update branch, thank you! |
Pull in ExtendDB#239 (GSI pagination tiebreaker), ExtendDB#245 (SQLite stale GSI delay), and ExtendDB#246 (Postgres restore race) — the three pre-existing backend bugs the dual-target suite surfaced. Resolve the add/add conflict on restore_active_completeness.rs in favor of this branch version, which carries the reviewer-requested retry-on-scan-error oracle fix and a bounded observer; main branch Postgres restore fix lands via its own source change.
|
@LeeroyHannigan Regarding the remaining CI failure. The SQLite CI job is down to a single remaining failure, test_index_pagination_uses_base_key_schema_for_tiebreaker. #245 fixed the zero-delay GSI test (now green). #239 (cd09154) improved the pagination one but didn't fully close it — it now returns a contiguous prefix that stops early (7 of 12 items, varying 4–7 across runs), pointing at pagination terminating early / a cursor bound, with some timing sensitivity. It reproduces on clean origin/main — my branch doesn't touch storage-sqlite or the shared GSI path (byte-identical to main), so it's not introduced here. Flagging since it keeps the SQLite job red on my PR; happy to help repro but it looks like a follow-up to #239 on your side. |
…ction_string Address review on run-tests: add 'sqlite' to the valid --backend values and its error message, and make the PostgreSQL connection-string extraction non-fatal (2>/dev/null ... || true) so the SQLite CI job — which runs against a config with no connection_string — no longer aborts under set -e/pipefail before the test suite runs.
What
Adds docs/rfcs/0000-mongodb-backend.md, a draft RFC for adding MongoDB as a optional ExtendDB storage backend.
Why
MongoDB is a natural fit as an additional database target: data model alignment; high read/write throughput through horizontal scalability; infrastructure fit.
DynamoDB and MongoDB share the same data model approach - documents stored as schema-less JSON-like data. MongoDBs document model maps directly to the approach taken by DynamoDB with each item stored as a MongoDB BSON document with no impedance mismatch at the data model level. Unlike relational databases, the translation from JSON to BSON is direct without complicated relational mapping techniques required.
This PR proposes the RFC tracked by the below issue.
Closes #206
Related forked implementation code
Testing done
git diff --checkpython docs/build-docs.pyChecklist
cargo fmt --check) (No Rust code was changed)ADR / RFC: This PR