feat: SQLite storage backend (in-tree) with developer mode#182
feat: SQLite storage backend (in-tree) with developer mode#182LeeroyHannigan wants to merge 3 commits into
Conversation
3317349 to
ba7fdee
Compare
| // binary that also contains a production backend. Postgres is the production | ||
| // backend, so the combination is rejected at compile time — there is no runtime | ||
| // path by which a Postgres deployment can serve in dev mode. | ||
| #[cfg(all(feature = "dev-mode", feature = "postgres"))] |
There was a problem hiding this comment.
What would you think of making this require sqlite or sqlite-memory with dev mode, rather than prevent postgres and dev mode specifically? Most backends we add will be considered production backends, I think, but very few will be considered as suitable for dev only.
| @@ -44,6 +44,76 @@ pub struct ServeArgs { | |||
| /// Bind the listening socket, daemonize, then start the tokio runtime. | |||
There was a problem hiding this comment.
This comment looks detached from the function it was associated with (run()).
| .unwrap_or_else(|| DEFAULT_SECRET.to_owned()); | ||
|
|
||
| // The credential store dispatches on the access-key prefix; keep that | ||
| // invariant intact rather than special-casing it for dev mode. |
There was a problem hiding this comment.
I don't understand this. I mean, I understand the words but I don't understand why we need to do anything special for dev mode.
There was a problem hiding this comment.
Dev mode only relaxes authorization (the IAM policy decision) SigV4 authentication stays identical to production, so the server must hold a credential whose secret matches what the SDK signs with, or every request fails signature verification. seed_dev_credential supplies that: it adopts AWS_ACCESS_KEY_ID/SECRET if present, else seeds AWS's well-known example key, keeping us a drop-in for DynamoDB Local without forking (and under-testing) the auth path.
There was a problem hiding this comment.
That seems orthogonal to the key type. What bad thing happens if someone uses an ASIA-type key in dev mode?
jcshepherd
left a comment
There was a problem hiding this comment.
Part 1 of N. Few minor comments/questions around dev mode, etc.
Addresses PR #182 review feedback (jcshepherd): the run() doc comment was left detached from run() when seed_dev_credential was inserted between them. Signed-off-by: Lee Hannigan <lhnng@amazon.com>
…ng postgres Addresses PR #182 review feedback (jcshepherd): invert the dev-mode compatibility guard from a deny-list (dev-mode + postgres) to an allow-list (dev-mode requires sqlite). Every backend is a production backend unless proven otherwise, so a deny-list would have to grow with each new backend; requiring a known dev backend keeps any current or future production backend incompatible with dev-mode automatically. sqlite-memory enables sqlite, so one check covers both modes. Signed-off-by: Lee Hannigan <lhnng@amazon.com>
| ); | ||
| } | ||
|
|
||
| // The default account is created during bootstrap (by `init`, or by |
There was a problem hiding this comment.
Do we have an explicit concept of a default account or admin account? If not, what is the assurance that the first account returned by list_all_accounts() is a/the admin account? Or is it okay to create the dev user under any account?
There was a problem hiding this comment.
Fixed. There's now an explicit default-account concept: bootstrap records default_account_id in settings (exposed via CatalogStore::default_account_id()), and seed_dev_credential resolves the account through it rather than list_all_accounts().first(). Recorded on both bootstrap paths (file-backed init + in-memory serve-time), idempotent so it also backfills existing catalogs.
| let metrics = Arc::new(extenddb_core::metrics::MetricsCollector::new()); | ||
|
|
||
| let tls_enabled = app_config.server.tls.enabled; | ||
| let tls_enabled = app_config.server.tls.enabled && !cfg!(feature = "dev-mode"); |
There was a problem hiding this comment.
Minor: I'd feel a shade more comfortable if there was a nice is_tls_enabled() utility function that wrapped this && . I haven't made it through the rest of the review but I'm seeing several checks in manage_http.rs on tls.enabled that don't appear to be updated.
| ); | ||
| // Monotonic clamp within the partition (RFC 3339 strings sort lexically). | ||
| let part_max: Option<String> = | ||
| sqlx::query_scalar("SELECT MAX(ready_at) FROM gsi_pending WHERE worker_partition = ?") |
There was a problem hiding this comment.
Yes idx_gsi_pending_claim ON gsi_pending (worker_partition, ready_at, id), which matches the drain query exactly (filter partition, order by ready_at, tiebreak id)
| async fn next_stream_seq( | ||
| tx: &mut sqlx::Transaction<'_, sqlx::Sqlite>, | ||
| ) -> Result<i64, StorageError> { | ||
| sqlx::query("UPDATE seq_counters SET value = value + 1 WHERE name = 'stream'") |
There was a problem hiding this comment.
Does this imply a global ordering for all stream records across all shards? Is that what we want?
There was a problem hiding this comment.
The counter is global/monotonic, but records are stored and consumed per shard (stream_records PK (shard_id, sequence_number)), so consumers only ever see per-shard order, which is all DynamoDB guarantees. Global monotonicity is just a cheap superset on single-writer SQLite; it doesn't imply any cross-shard ordering contract.
jcshepherd
left a comment
There was a problem hiding this comment.
Left a few more comments/questions.
General questions: are there "migration scripts" to go with this PR, for setting up system tables, etc., needed by ExtendDB in MySQL?
I assume you mean SQLLite? Schema setup is one authoritative migration, |
|
Just want to say I pulled this down to build and started testing with it. Very excited here. 😋 |
|
Thanks @eldondevat 🚀 We'll get things finalized this week and hopefully get it into your hands very soon 💪🏻 |
| .await?; | ||
| } | ||
| } | ||
|
|
||
| tx.commit() | ||
| .await | ||
| .map_err(|e| StorageError::Internal(e.to_string()))?; | ||
|
|
||
| // Data DDL after catalog commit. | ||
| if let Some(updates) = &input.global_secondary_index_updates { |
There was a problem hiding this comment.
couple of things I'd like to discuss regarding GSI creation and backfill process,
- [HIGH] Backfill runs under the global write lock, so it blocks writes to all base tables in the instance for the full duration
- GSI is advertised ACTIVE (after Catalog table COMMIT) before base table data is backfilled, since these are non-atomic multi step update. (DescribeTable respond with ACTIVE for the index, but Get or Query will result in resource not found since data table does not yet exist)
- Create + backfill is non-atomic, and a process crash between them leaves an ACTIVE index row with no data table, and nothing reconciles it on restart
| maps: &ExpressionMaps, | ||
| stream: Option<&StreamCapture>, | ||
| ) -> Result<Option<Item>, StorageError> { | ||
| let indexes = fetch_indexes_for_table(&key_info.table_id, &self.pool).await?; |
There was a problem hiding this comment.
[not a blocker]
table's index metadata is fetched from the catalog on the write hot-path for every request (put, update, delete, and TWI). I believe we should cache this on the already cached TableKeyInfo and skip the read. Same pattern exists in the Postgres backend, and can be resolved by making this change on the shared layer.
| let mut errored = false; | ||
| loop { | ||
| match process_gsi_batch(&engine).await { | ||
| Ok(n) if n > 0 => continue, |
There was a problem hiding this comment.
[not a blocker] GSI async propagation, divergent failure modes across backends.
Postgres and SQLite handle async GSI propagation differently on failure. Postgres runs 4 workers partitioned by pk-hash and, on a bad update, logs-and-drops it so the queue keeps draining (liveness preserved, that one index write silently lost).
SQLite uses a single worker across all tables/accounts and, on a bad row, rolls back the claimed batch and re-claims the same rows forever (no drop). So one poison row permanently stalls all GSI propagation instance-wide and the queue grows unbounded, not just head-of-line delay.
Neither of these matches DynamoDB; flagging the divergence as a discussion point.
| -- Idempotency tokens for TransactWriteItems. | ||
| CREATE TABLE IF NOT EXISTS idempotency_tokens ( | ||
| token TEXT PRIMARY KEY, | ||
| fingerprint TEXT NOT NULL, | ||
| created_at TEXT NOT NULL DEFAULT (strftime('%Y-%m-%dT%H:%M:%fZ','now')) | ||
| ); |
There was a problem hiding this comment.
this table is keyed on token alone, but TWI idempotency tokens are unique per account, not globally. Key it on (account_id, token) so two accounts reusing the same token don't clash (currently they do: silent replay or false IdempotentParameterMismatch across accounts).
| // Pre-fetch index metadata per distinct table (outside the write lock). | ||
| let mut table_indexes: HashMap<String, Vec<IndexMeta>> = HashMap::new(); | ||
| for op in ops { | ||
| let name = op_table_name(op); | ||
| if !table_indexes.contains_key(name) { | ||
| let indexes = fetch_indexes_for_table(op_table_id(op), &self.pool).await?; | ||
| table_indexes.insert(name.to_owned(), indexes); | ||
| } | ||
| } | ||
|
|
||
| let _writer = self.write_lock.lock().await; |
There was a problem hiding this comment.
there is a TOCTOU race condition here, index metadata is read before taking write_lock, so a GSI added by a concurrent UpdateTable in this gap can be missed, and this write won't maintain it, leaving those items permanently missing from the new GSI. I believe we should read the index set after acquiring the lock.
this also exists on other write APIs.
Adds the SQLite storage backend (crates/storage-sqlite) as an alternative to PostgreSQL for local/dev use, with GSI async-propagation parity to the Postgres queue design and the review-driven correctness fixes: - idempotency tokens keyed per (account_id, token) - index metadata read inside the write lock on all write paths (TOCTOU) - GSI queue drops unprocessable rows instead of stalling propagation - GSIs build as CREATING and are reconciled to ACTIVE on startup after backfill; DescribeTable reports CREATING until the backfill completes - data-plane ops on a CREATING/DELETING table return ResourceNotFound Signed-off-by: Lee Hannigan <lhnng@amazon.com>
4e704f8 to
968e746
Compare
|
Thanks @yesyayen — really appreciate the thorough pass. All five are addressed; mapping each to the change: 1. GSI create/backfill (
2. TOCTOU on the index read ( 3. Idempotency token keyed globally ( 4. GSI worker poison-row stall ( 5. Hot-path metadata read ( Also added a data-plane parity fix: ops on a Tests: idempotency (replay/mismatch/cross-account), poison-row drop, and reconciler crash-recovery added as in-crate tests. Full verification on the current tip — Python comprehensive suite 1106 passed / 0 failed, Rust integration 372 passed / 0 failed, unit 16 passed, fmt + clippy The branch was also linearized (it carried internal merge commits that the merge queue rejects) and rebased onto current |
The migration registry added pending_data_migrations to the Bootstrapper trait; the SQLite bootstrapper was missing it. SQLite applies its full schema in run_catalog_migrations (single file) with no separately-tracked data migrations, so it reports none pending. Signed-off-by: Lee Hannigan <lhnng@amazon.com>
clippy useless_borrows_in_formatting on the sqlx log-filter format arg. Signed-off-by: Lee Hannigan <lhnng@amazon.com>
Bring the SQLite storage backend into the workspace as crates/storage-sqlite, per RFC-0002 (backends as in-tree crates). PostgreSQL remains the default; SQLite is selected by Cargo feature. Targets local development, CI, and embedded/air-gapped use.
Consolidation:
sqliteandsqlite-memory(zero-config ephemeral in-memory).link()and the bin'sbackends::link_all()references every enabled backend, so inventory registrations are never dead-stripped.Developer mode (build-time
dev-modefeature):dev-mode+ the Postgres backend fail to build, so an insecure production binary cannot exist.Fixes found bringing the backend to parity:
Testing:
Checklist
cargo test --workspace)cargo fmt --check)cargo clippy -- -W clippy::pedantic)Storagetrait, auth model, on-diskformat, or public CLI surface, an RFC has been accepted or is linked
below. Otherwise, an ADR captures the decision (link below).
ADR / RFC:
Breaking changes
By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache License 2.0 and I agree to the Developer Certificate of
Origin (DCO). See CONTRIBUTING.md for details.