[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. |
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. |
|
@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. |
|
@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! |
|
@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. |
The runner was implicitly postgres-only in two places: it greps the
config for `backend = "postgres"` before extracting a pg connection
string, and it runs `test_cli_lifecycle.py` (postgres-only) whenever
that connection string is set. Both worked accidentally on a mongo
config today — the postgres-backend grep just missed and everything
downstream was a no-op — but the coupling to config-file contents is
fragile.
Add an explicit `--backend {postgres,mongodb}` flag (default
`postgres` for backward compat). The flag gates the two postgres-only
paths and prints the backend in the target-info block. Everything
else — health check, credential provisioning, throttling +
import/export config mutation, pytest / rust / external / catalog-
check suites — stays backend-agnostic and needs no change.
Unblocks a mongo CI workflow that can delegate to `run-tests` the
same way `.github/workflows/integration.yml` does for postgres.
Nothing in the mongo backend has been verified against 6.x; local development, the bench-compare harness, and the container tag used in the planned CI workflow all use `mongo:7`. Bring the docs in line — stating 6.0+ implies a support surface we don't test and can't stand behind. Documentation-only change. No code touches the mongo-driver version floor; that's controlled by the `mongodb` crate's own minimum.
`devtools/run-tests` is a runner, not an orchestrator — it assumes the server is already up at `$EXTENDDB_TEST_ENDPOINT`. The postgres CI workflow supplies the server lifecycle (init, serve, poll /health) inline before delegating to `run-tests`. Local mongo runs had no equivalent — the bench-compare harness recreated the lifecycle each time by hand. `devtools/run-mongodb-tests` fills that gap: one entry point that spins up a `mongo:7` single-node replica set in Docker, initializes and serves extenddb against it, then delegates to `devtools/run-tests --backend mongodb`. Teardown on exit; `--keep` leaves everything up for post-run inspection. Arguments after `--` are forwarded to `run-tests` verbatim so callers can pick the suite (`--pytest`, `--comprehensive`, `--parallel`, `--filter …`). Default is `--pytest --comprehensive --parallel`. The mongo CI workflow (a follow-up commit on this branch) can call this script directly and drop most of its shell-level orchestration.
Upstream db0baba added `account_id` to `Storage::get_stream_records` so GetRecords is scoped to the shards owning account; the mongo backend still implemented the old 4-arg signature and returned records without an ownership check, so a caller could read another accounts stream records by presenting a forged shard iterator. Add the `account_id` parameter and an ownership guard that mirrors storage-postgres: resolve shard_id -> table_id from `stream_shards` (data db), then confirm a `tables` catalog document with that table_id is owned by the calling account (account_id lives inside the compound `_id`, so the comparison is done in Rust after a single table_id lookup). When the shard is unowned or absent, return ValidationException("Invalid ShardIterator") — matching DynamoDB, which does not distinguish "exists but not yours" from "does not exist". Verified by tests/test_cross_account_isolation.py::TestStreamAccountScoping ::test_shard_iterator_only_returns_owning_account_records against the mongo backend. Also syncs Cargo.lock (extenddb-storage-mongodb 0.1.0 -> 0.1.2) to the workspace version bump pulled in by the rebase.
…eering Start the server via extenddb serve (which daemonizes itself) instead of serve --foreground with a background &, and stop it via extenddb stop instead of kill. Set server.run_dir to the test output dir so serve and stop share an isolated PID-file location. Removes the manually-managed server.pid file.
pushdown.rs admitted Field <op> Field for all types, but a plain field type is unknown at compile time, so the emitted $expr compared the raw tagged subdocuments. Two Number fields (stored string-encoded) then compared lexically, so counter_a < counter_b evaluated backwards in both directions. Mark Field vs Field NotPushable so it falls back to the in-Rust evaluator, consistent with the existing N and B literal exclusions. Adds a regression test locking every comparator.
Binary sort keys are stored as lowercase hex strings, so begins_with is a
string-prefix range over the hex encoding. The upper bound was computed as
hex(increment_bytes(prefix)) -- incrementing the raw bytes then re-encoding
-- which is not the next prefix in fixed-width hex space and widens the
range. begins_with(0x2F,0xFF) produced ["2fff","3000") and wrongly matched
the stored key 0x30 ("30"); begins_with(0xFF) produced an empty range and
dropped every match.
Use next_string_prefix on the hex encoding, mirroring the string sort-key
path: sk_b >= hex(B) AND sk_b < next_string_prefix(hex(B)), dropping the
upper bound when the prefix is empty. Removes the now-unused increment_bytes
helper. Adds a regression test for both wire-level repros.
CreateTable and RestoreTableFromBackup now write the catalog row as CREATING with a status_transition_at timestamp when control_plane_delay_seconds > 0 (default 0.25), and return CREATING; a new background control_plane_worker flips rows to ACTIVE once the scheduled transition time passes. When the delay is 0 the row is written ACTIVE directly. Matches the postgres backend and real DynamoDB, which report CREATING before a table is usable. DeleteTable sets DELETING on the row but completes the drop synchronously within the request; the control-plane worker only reconciles the CREATING -> ACTIVE transition, not deletes. Restore delegates row creation to create_table and no longer forces the table ACTIVE inline, so it enters the same CREATING window; the data is copied via $out before the table is flipped to ACTIVE. Data-plane key-schema resolution against a non-ACTIVE table now returns ResourceNotFoundException (TableNotFound) instead of ResourceInUse, matching DynamoDB and the postgres backend. Restores WorkerStore::process_control_plane_transitions (fixing the compound _id query the previous no-op replaced) and spawns the poller from MongoRuntimeHooks::spawn_workers. Reverts the RFC and design-doc language that described control-plane transitions as inline. Fixes the conformance tests put_item_on_creating_table_returns_not_found and restore_table_from_backup.
…ndDB#218 main Rebase onto upstream main after PR ExtendDB#218 (serve lib decoupling), which replaced inventory backend registration with an explicit set_backend/Backend model and split the CLI into extenddb-app. Also adapts to backup-trait and worker changes and to new backup_arn_scoping conformance tests pulled in by the rebase. - Replace the six inventory::submit! blocks with a single extenddb_storage_mongodb::backend() constructor plus a server_components_factory fn, mirroring the postgres backend. - Drop the now-removed inventory dependency. - Feature-gate the thin bin: install the mongodb backend under --features mongodb, else postgres. - Scope describe_backup and delete_backup to account_id (added to the BackupEngine trait upstream); exclude DELETED backups from describe_backup so a deleted backup reads as BackupNotFoundException. - Give backup ARNs a timestamp-plus-8-hex-char random id so they are not guessable from creation time alone. - Return the spawned worker JoinHandles from spawn_workers, whose trait signature now requires Vec<JoinHandle<()>>.
Rebase onto upstream main (6dcb14c), whose per-index consumed-capacity work added global_secondary_indexes and local_secondary_indexes to TableKeyInfo. Load all secondary indexes from the catalog in table_key_info_from_doc and populate both lists (via a new index_info_from_doc helper) so per-index consumed capacity is computed from the cached TableKeyInfo without an extra describe_table per write, matching the postgres backend. has_lsi is now derived from the LSI list.
…mpletes RestoreTableFromBackup calls create_table, which schedules the CREATING to ACTIVE transition on a wall clock, and only then runs the $out copy. The transition worker flips any table whose status_transition_at has elapsed and never consults the copy, so ACTIVE does not imply the restored data is present. A client that waits for ACTIVE can read an empty table. The existing conformance coverage cannot catch this because a small backup finishes copying inside the transition window. This test seeds 40,000 items so the copy outlasts the window, and races an observer against the in-flight restore: the moment DescribeTable first reports ACTIVE, it counts the table. Observed on this branch: ACTIVE with 0 of 40,000 items readable. The test is a race detector by construction, which cuts one way only. It cannot fail when the ordering is correct, because a post-copy ACTIVE always yields a complete count. But a pass is weak evidence: on an idle server the copy can win the race and the defect goes unobserved. This is stated in the module docs so a green run is not read as proof.
…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].
…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.
Fold in the adaptations that previously lived in merge-conflict resolutions, so the history linearizes cleanly for the rebase-based merge queue: - Mutually-exclusive backend selection in the thin bin (compile_error guards + set_backend arms) and bin/Cargo.toml optional deps, matching the SQLite backend's one-backend-per-binary model. - Storage-trait updates: ManagementStore::default_account_id and the ServerComponentsOptions parameter on the server-components factory. - Build docs/CI updated to --no-default-features --features mongodb (AGENTS.md, getting-started, local-mongodb-setup, design doc, RFC, integration-mongodb.yml, run-mongodb-tests). - Dedup the restore_active_completeness test module registration.
a2c097d to
d873f65
Compare
Main gained the MongoDB backend (PR #207) after this branch was cut, and that backend predates the contract's widened core types, so merging without adapting it breaks the build: four struct initializers missing `vector_indexes` and one non-exhaustive match over `IndexType`. The adaptation is the one already proven on the implementation branch (feat/sqlite-vector-search, merge a8d5850), taken verbatim so the two branches cannot diverge on it: `..Default::default()` at the three initializer sites, matching what the Postgres backend does at the same sites for the same stated reason, and `core::types::partition_indexes` in place of the local `match`, the helper that exists so a new index kind cannot break a backend that does not implement it. MongoDB needs no vector code: `as_vector_search()` defaults to `None`, so it refuses vector work by omission. Verified: fmt and clippy -D warnings at 0, 800 workspace unit tests passed with 0 filtered out. The MongoDB integration suite needs a Mongo container and was not run locally; CI covers it.
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