Skip to content

fix(backup): restore sort keys, indexes, and throughput; bound memory; recover abandoned restores - #384

Open
robinnsc wants to merge 1 commit into
mainfrom
fix/backup-restore-fidelity
Open

robinnsc wants to merge 1 commit into
mainfrom
fix/backup-restore-fidelity

Conversation

@robinnsc

@robinnsc robinnsc commented Oct 5, 2026 •

Copy link
Copy Markdown
Collaborator

What

PostgreSQL and SQLite backup and restore now reproduce the table that was backed up, with bounded memory and crash recovery. One commit, rebased on current main (which includes #391).

Scope against the readiness review's P0-2. This closes the two restore defects that review said needed fixes of their own (PostgreSQL loses sort keys and hangs the target in CREATING; both backends drop indexes) and adds the restore-fidelity test it asked for. It does not add point-in-time recovery (UpdateContinuousBackups and RestoreTableToPointInTime are still refused; #335 reports PITR honestly) or off-host backup storage (backups still live in the database; #336 and #337). Those remain open.

Sort keys. create_backup and restore_table_from_backup checked for a column named sk, which data tables do not have (sk_s, sk_n, sk_b). Backups of every table with a sort key lost their sort keys, and restoring one left the target in CREATING for good. Backups now store pk and item_data only; restore derives every key column from the item. Backups written by earlier binaries carry the whole item in item_data and restore correctly.

Restored, from a new per-backup table definition: global and local secondary indexes with key schemas, projections, and GSI throughput, filled during the copy; billing mode and provisioned throughput (it was always 5/5); table class, SSE specification, on-demand throughput. Streams, TTL, tags, and deletion protection are not restored, as on the service. A definition captured from a PAY_PER_REQUEST table carries no provisioned throughput for the table or its indexes, even when the catalog still holds values from before a switch from PROVISIONED.

RestoreTableFromBackup overrides. BillingModeOverride and ProvisionedThroughputOverride are implemented on all three backends. They reach storage as an explicit overrides: RestoreTableOverrides parameter on BackupEngine::restore_table_from_backup (a Storage trait change, see below). BackupTableDefinition::apply_to applies them before the PAY_PER_REQUEST normalization and refuses, before any target is created, a PROVISIONED restore of a backup whose GSIs carry no throughput (the service needs GlobalSecondaryIndexOverride there). GlobalSecondaryIndexOverride, LocalSecondaryIndexOverride, SSESpecificationOverride, OnDemandThroughputOverride, and VectorIndexOverride are refused with ValidationException before any storage read; previously every override was silently ignored. Throughput values reuse CreateTable's validator, so the error text matches CreateTable's.

Catalog 0.0.4:

  • crates/storage-postgres/migrations/003_backup_definitions.sql and the SQLite schema add backup_definitions (one row per backup; the definition as JSON behind a version marker, crates/storage/src/backup_definition.rs) and table_restores (one row per restored table; not a foreign key to backups, so the summary outlives the backup as on the service).
  • SQLite's backup_items gains id INTEGER PRIMARY KEY; the restore cursors on it rather than the implicit rowid, which a VACUUM between separately committed batches could renumber. An existing backup_items is rebuilt in place by extenddb migrate (copy in rowid order, idempotent). backup_items.pk now holds the real partition key on SQLite, as on PostgreSQL.
  • Backups and restores from before the upgrade have no definition rows and behave as before: keys and items, no secondary indexes, 5/5 throughput for a provisioned table, no RestoreSummary.
  • The migration runners write the compiled catalog version after every walk (SET_CATALOG_VERSION_SQL), the same change fix(backup): report point-in-time recovery as unsupported #335 makes. They refuse to write it when the stored version is newer, and extenddb migrate refuses a catalog newer than the binary outright, so an older binary can no longer "migrate" a newer catalog, stamp its version onto it, and then pass its own startup gate.
  • fix(backup): report point-in-time recovery as unsupported #335 also claims 0.0.4 and migration 003; whichever of the two merges second renumbers to 0.0.5 and 004.

Consistency:

  • PostgreSQL CreateBackup holds the source table row FOR SHARE from its first catalog read until its data snapshot is taken, and the REPEATABLE READ snapshot is pinned by a real read of the table before the row is released. The first revision relied on LOCK TABLE for this; it is a utility statement and does not establish the snapshot, so the definition and the items could come from different instants (reviewer finding, reproduced). UpdateTable and DeleteTable take the row FOR UPDATE, so the definition recorded is the one in force when the items were read.
  • SQLite CreateBackup takes the engine write lock only to read the definition, insert the backup row as CREATING, and open the item reader's WAL snapshot; items are then streamed without the lock and written in batches under short lock holds, so other writers wait for a batch, not the whole backup (the first revision stalled every write for the duration: about twelve seconds for 100,000 items; reviewer finding). The copy runs detached from the request, so a client that disconnects does not leave a half-written backup; one left by a crash is removed at startup. A CREATING backup is visible to ListBackups/DescribeBackup, as on the service; DeleteBackup refuses it and RestoreTableFromBackup reports it missing. The long reader pins WAL checkpointing for the backup's duration; documented. A file-backed SQLite pool is clamped to at least two connections so the reader and the batch writer can coexist.
  • A restore registers itself in the same transaction that creates its target on PostgreSQL (backup row FOR SHARE and the table_restores row inserted with the tables row) and on SQLite, so a concurrent DeleteBackup orders strictly before (the restore sees no backup, no target is created) or after (it sees the restore and returns BackupInUseException). MongoDB has no cross-collection transaction and uses a claim marker on the backup's metadata: a restore claims before reading and releases once its CREATING target exists; DeleteBackup claims the same way before dropping data; a claim left by a crashed process is cleared at the next startup.
  • DeleteTable on a table that is being restored returns ResourceInUseException on all three backends.
  • DescribeTable reports RestoreSummary for a restored table; RestoreInProgress is true while CREATING. The restore response and later DescribeTable calls carry the same RestoreDateTime.

Memory and concurrency: backup and restore stream items and buffer at most 500 items or 4 MiB of stored JSON. PostgreSQL runs up to four restores at once per process; a fifth waits up to 30 s and is refused with LimitExceededException.

Failure and crash:

  • A failed copy removes its target by table id, claiming it CREATING → DELETING in one statement so removal and the flip to ACTIVE cannot both happen; MongoDB removes a partial target the same way.
  • PostgreSQL: a restore holds a session advisory lock keyed by account and target name on a dedicated connection; its SET idle_session_timeout is best-effort (managed instances may refuse it). The control-plane pass removes targets that are CREATING with no scheduled transition, older than 60 s, whose lock is free; with the pass's 60 s idle timeout that is between 60 and about 120 s after abandonment.
  • SQLite: the same targets are removed at startup, which is safe because one server at a time holds the file (fix(sqlite): one extenddb process per database file #391, merged).
  • MongoDB has no abandoned-restore sweep; a crashed restore leaves its target CREATING for an operator to remove. Documented.
  • The control-plane worker's DeleteTable drop runs under SET LOCAL lock_timeout = '3s' and drops data tables before deleting the catalog row, so a drop blocked behind a backup's ACCESS SHARE lock fails fast and the row stays DELETING for the next pass. The previous order deleted the catalog row first, so a failed drop was never retried. The stale-vector-index recovery now skips indexes whose parent table is DELETING and cleans up if the parent vanishes under it, so the reordered drop cannot leave an orphan vector table.

Refused rather than restored wrongly: backups of tables with vector indexes (both backends; SQLite previously restored the table silently without them); backups of multi-part-key tables; table definitions with an unknown version; and the unsupported overrides above. RestoreTableFromBackup validates TargetTableName. New DynamoDbError::BackupInUseException and StorageError::BackupInUse.

Docs: docs/dynamodb-limits.md no longer says backup and restore are unsupported, and its summary row is recounted; docs/differences-from-dynamodb.md gains a Backup and Restore section with every row qualified by backend; the upgrade manual has a 0.0.4 section that warns PostgreSQL operators that targets the old bug left in CREATING are removed on the first control-plane pass after the upgrade, describes the SQLite lock, and notes WAL headroom; the admin guide's version literal and migrate guidance are current.

Why

No issue filed. The 1.0 readiness review's P0-2 found:

  • PostgreSQL backups of composite-key tables could not be restored.
  • PostgreSQL and SQLite restores dropped GSIs, LSIs, throughput, table class, and encryption settings.
  • Restores held every item in memory.
  • A crash mid-restore left the target CREATING with its name taken.

The backup-store design in #348 replaces this path. Until it lands, this is the path users have.

Testing done

tests/test_backup_restore_fidelity.py (11, any backend): S/N/B hash and sort keys compared item by item by Scan and GetItem; a provisioned table with four GSIs and an LSI (projections ALL/INCLUDE/KEYS_ONLY, S/N hash and N/B range keys, sparse items, table and GSI throughput, IndexStatus, every index's contents, projected attribute sets, ordered ranged reads both directions); RestoreSummary on the response and on DescribeTable; a PROVISIONED table switched to PAY_PER_REQUEST restoring with zero throughput on the table and its GSI; BillingModeOverride in both directions with ProvisionedThroughputOverride; a refused PROVISIONED override on a PAY_PER_REQUEST backup with a GSI, with DescribeTable then returning ResourceNotFoundException (no target left). 11/11 on PostgreSQL and on SQLite against servers built from this branch.

crates/storage-postgres/tests/backup_restore.rs (21, in the PostgreSQL storage-level CI step): old backup formats; failure cleanup; duplicate rows; the multi-part refusal; restored indexes, throughput, table class, SSE, on-demand throughput; more than 1,000 rows including 300 KB items; the abandoned-restore sweep (inside the grace period, lock held, unowned, lock held by another); the backup barrier against an uncommitted UpdateTable; a production-path snapshot test that blocks create_backup after its snapshot is pinned, commits an item, and asserts the backup excludes it (verified to fail when the pinning read is removed), plus the two transaction shapes that show why LOCK TABLE alone was not enough; a half-built GSI; DeleteTable during a restore; DeleteBackup racing a restore in both orders; the drop lock_timeout leaving a row DELETING and the next pass finishing it; RestoreSummary and DeleteBackup (absent, in progress, done, refused then allowed, kept after the backup is deleted). The fixture appends /postgres itself, so EXTENDDB_TEST_PG_CONNECTION_STRING is the server, not a database.

SQLite restore_tests (16): indexes and throughput; the startup sweeps for restores and backups; failed-copy cleanup; the multi-part refusal; table class, SSE, on-demand throughput; 730 rows across batch boundaries compared to what the write path produces; a database with the exact 0.0.3 schema holding a backup in the 0.0.3 row format, migrated and restored with id present and rows in order; byte-bounded batch cuts; DeleteTable during a restore; the restore intent committed with its target; an unrelated writer keeping progress during a 5,000-item backup (fails on the whole-lock shape); items written after the snapshot excluded; a cancelled CreateBackup finishing with every item; the backup_items rebuild being idempotent and order-preserving.

Unit tests: definition normalization and the override preflight (12); the engine's override parsing and refusals including every unsupported member and the CreateTable throughput text (15); MongoDB's claim predicates, GSI throughput handling, and the two-phase claim filters (7); CatalogVersion ordering; the newer-catalog refusal in extenddb migrate and in both schema runners; the vector-recovery selection predicate.

By hand, against release builds from this branch: the override matrix on PostgreSQL (throughput-only override on a provisioned backup; BillingModeOverride=PROVISIONED without throughput refused; invalid enum text; OnDemandThroughputOverride/VectorIndexOverride refused; PROVISIONED+GSI → PAY_PER_REQUEST reports 0/0); a cancelled CreateBackup on SQLite observed CREATING then AVAILABLE with the full count; a 20,000-item SQLite backup with an unrelated writer at a 126 ms maximum gap (previously the whole 0.9 s); a 0.0.3 SQLite file migrated by the binary with backup_items.id present and a second migrate a no-op. Full tests/ on PostgreSQL against this branch and main: identical 8 environment-dependent failures on both (this host's botocore), no regression; the branch adds 18 passing tests.

cargo fmt --all -- --check
cargo +1.97.0 clippy --workspace --all-targets -- -D warnings   # the CI toolchain
cargo +1.97.0 check --locked --no-default-features --features mongodb
cargo test --workspace                                          # 1,323 passed
cargo +1.88.0 check --workspace --locked
EXTENDDB_TEST_PG_CONNECTION_STRING=... cargo test --release -p extenddb-storage-postgres \
  --test backup_restore --test vector_control_plane              # 21 + 42 passed

Review: two rounds of independent adversarial review (four and two reviewers) on this revision after the human review; every blocking and major finding from both is fixed above. MongoDB's claim protocol was checked by an exhaustive interleaving model (20 orderings, no dual claim) but not against a live replica set on this host.

Not changed here

Checklist

  • I have read CONTRIBUTING.md
  • All tests pass (cargo test --workspace)
  • Code is formatted (cargo fmt --check)
  • Clippy is clean (cargo clippy -- -W clippy::pedantic)
  • I have added or updated tests for new functionality
  • I have updated documentation if behavior changed
  • Breaking changes are noted below (if any)
  • If this changes the wire protocol, Storage trait, auth model, on-disk format, or public CLI surface, an RFC has been accepted or is linked below. Otherwise, an ADR captures the decision (link below).

ADR / RFC: #348 (backup and restore design) describes the target this moves toward. Storage trait: BackupEngine::restore_table_from_backup gains an overrides: RestoreTableOverrides parameter (Default means none); the overrides are request input the service defines, and an explicit parameter was preferred over a task-local so every backend author sees it. On-disk: two new catalog tables and one new SQLite column. No wire, auth, or CLI changes beyond honouring request members the API already defines.

Breaking changes

  • Catalog 0.0.4. Every PostgreSQL and SQLite deployment must run extenddb migrate; the server refuses to start against 0.0.3. extenddb migrate refuses a catalog newer than the binary.
  • BackupEngine::restore_table_from_backup takes an additional overrides parameter; out-of-tree backends must add it.
  • RestoreTableFromBackup refuses backups of multi-part-key tables (previously restored with the wrong layout), SQLite refuses backups of tables with vector indexes (previously restored without them), and requests carrying GlobalSecondaryIndexOverride, LocalSecondaryIndexOverride, SSESpecificationOverride, OnDemandThroughputOverride, or VectorIndexOverride are refused (previously ignored).
  • DeleteTable on a table being restored returns ResourceInUseException.
  • DeleteBackup on a backup being restored from, or on a SQLite backup still CREATING, returns BackupInUseException.
  • A file-backed SQLite pool has at least two connections regardless of pool_size.

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.

Comment thread crates/storage-postgres/src/backup_engine.rs Fixed
Comment thread crates/storage-postgres/src/backup_engine.rs Dismissed
Comment thread crates/storage-postgres/src/backup_engine.rs Fixed
@robinnsc robinnsc changed the title fix(postgres): back up and restore tables with a sort key fix(backup): restore sort keys, indexes, and throughput; bound memory; recover abandoned restores Oct 5, 2026
Comment thread crates/storage-postgres/src/backup_engine.rs Dismissed
Comment thread crates/storage-sqlite/src/backup.rs Dismissed
Comment thread crates/storage-sqlite/src/backup.rs Dismissed
Comment thread crates/storage-sqlite/src/backup.rs Dismissed
robinnsc added a commit that referenced this pull request Oct 6, 2026
Nothing stopped two `extenddb serve` processes from opening the same SQLite
file. The backend serializes writers with a lock inside the server process,
so two servers write concurrently and can hit SQLITE_BUSY mid-transaction;
and startup recovery (control-plane transitions, GSI and vector index
rebuilds, and, with #384, abandoned restores) assumes no other server is
running, so a second server starting up would undo the first one's work in
progress. Nothing stopped `destroy` from unlinking the file under a running
server either, which left that server serving an unlinked inode while a
later `init` created a fresh database at the same path.

`extenddb serve` on a file database now takes an exclusive flock(2) on
`<database>.lock` before opening the database and holds it for the life of
the process (the lock lives in the engine, so it is held while any clone
is). `init` and `migrate` take the same lock for their duration through the
bootstrapper's migration-lock hook, and `destroy` takes it before it
removes the file; each refuses, rather than waits, when a server holds it.
Read-only commands (`settings`, `manage`, `verify`, `status`) take no lock.
A second holder fails with:

  another extenddb process is already using <db> (lock held on <db>.lock);
  stop it first, or point this command at a different database

The lock file is named after the file SQLite actually opens, not the
configured string: the location is parsed with sqlx's own
SqliteConnectOptions (so `sqlite:` URL forms, percent-encoding such as
`a%20b.sqlite`, and `file:` URIs resolve as the engine resolves them), and
the path is canonicalized, so two spellings of one file, a `..` path, or a
symlink and its target all share one lock. A disk file whose name happens
to contain `mode=memory` is a file. In-memory databases take no lock.

The kernel releases the lock when the process exits, including on kill -9,
so there is no stale-lock cleanup. On non-Unix platforms no lock is taken
and a warning is logged. flock is advisory and its behaviour on network
filesystems depends on the server and mount; the docs say to keep SQLite
databases on local disk. The directory holding the database must be
writable so the lock file can be created.

flock through libc rather than std's File::try_lock, which needs Rust 1.89;
the workspace MSRV is 1.88. libc is already a dependency of the workspace
and listed in every license notices file.

Docs: troubleshooting entry for the error with its scope; README and the
deployment and design guides now state that one instance per catalog is
the supported deployment, that SQLite enforces it, and that PostgreSQL and
MongoDB do not (per-instance caches without cross-instance invalidation,
every worker on every instance, a liveness-only /health). The deployment
guide previously said multiple PostgreSQL instances were consistent because
there was no in-process cache, which was not true.

Tests: database_file resolution for plain paths, sqlite: URLs,
percent-encoding, file: URIs, in-memory forms, and a disk file named like
a memory parameter; ServeLock refused on the same file, independent across
files, shared across `..` spellings and across file and directory symlinks;
the server factory refusing a second server on one file; destroy and
migrate refused while a server holds the file and a server refused while
migrate holds it.

Signed-off-by: Scott Robinson <robinnsc@amazon.com>
@robinnsc
robinnsc force-pushed the fix/backup-restore-fidelity branch 3 times, most recently from 13c7f24 to dc980eb Compare October 6, 2026 19:43

@yesyayen yesyayen left a comment •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Blocking on two things, both inline:

  1. the Postgres CreateBackup barrier doesn't pin the snapshot the way the code and docs say it does, and
  2. SQLite CreateBackup now holds the write lock for the whole read, which is a server-wide write stall on a big table. The rest are non-blocking.

.execute(&mut *snapshot)
.await
.map_err(db_err)?;
meta.commit().await.map_err(db_err)?;

@yesyayen yesyayen Oct 7, 2026 •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Blocking. This barrier doesn't hold. LOCK TABLE is a utility statement and doesn't take a snapshot in Postgres. The REPEATABLE READ snapshot is taken by the first SELECT, which here runs after meta.commit() has already released the FOR SHARE row. So the definition is captured at one instant and the items at a later one, with UpdateTable free to commit in between. I checked this on PG 15: BEGIN ... REPEATABLE READ; LOCK TABLE t ...; then an insert from another session, then SELECT count(*) sees the new row.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yeah good catch, swapped that to BEGIN RR RO; LOCK TABLE … ACCESS SHARE; SELECT 1 FROM <data> LIMIT 1; and only then releases the FOR SHARE row

Comment thread crates/storage-sqlite/src/backup.rs Outdated
// instant, including against a concurrent UpdateTable. Items are
// read in rowid batches on the same connection, so memory is
// bounded by the batch size.
let _writer = self.write_lock.lock().await;

@yesyayen yesyayen Oct 7, 2026 •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Blocking. This now holds the engine write lock for the whole read and write of the table. On main the item read ran before the lock, only the inserts were under it. The pool is WAL mode, so a deferred read transaction on a second connection already gives a stable snapshot without blocking writers.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

FIxed, the lock is now held only to read the definition, insert the backup row as CREATING, and open the deferred read transaction (snapshot fixed under the lock, so definition and items agree), items stream unlocked and are written in batches under short holds

Comment thread crates/engine/src/backup.rs Outdated
"SSESpecificationOverride",
];

fn validate_restore_table_overrides(body: &Value) -> Result<(), DynamoDbError> {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Now that the definition is in hand, BillingModeOverride and ProvisionedThroughputOverride are a few lines on CreateTableInput. Refusing them is a wire regression for clients that pass them (the console does). I'd implement at least those two and refuse only the index and SSE overrides.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done

Comment thread docs/differences-from-dynamodb.md Outdated
| `RestoreTableFromBackup` overrides | `BillingModeOverride`, `GlobalSecondaryIndexOverride`, `LocalSecondaryIndexOverride`, `ProvisionedThroughputOverride`, `SSESpecificationOverride` apply to the restored table | Not supported; a request carrying any of them returns `ValidationException` rather than restoring a table that differs from what was asked for |
| Multi-part base table keys (preview) | Not supported | A backup of a table created with `enable_multipart_keys` is refused at restore with `ValidationException`; the item paths address such tables by their first HASH and RANGE attribute only |
| `DeleteBackup` during a restore from that backup | `BackupInUseException` | Matches: `BackupInUseException` (HTTP 400) until the restore completes or fails |
| `DeleteTable` on a table being restored | `ResourceInUseException` | Matches on a restore target. An ordinary table in `CREATING` can still be deleted during its control-plane delay, where the service refuses |

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This says "Matches on a restore target" without a backend qualifier, but the MongoDB diff only adds RestoreSummary and the DeleteBackup refusal. DeleteTable during a Mongo restore is not refused, and Mongo has no abandoned-restore sweep. Either qualify these rows or add the behaviour.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Qualified every row by backend, and implemented the Mongo DeleteTable, and clarified that Mongo still has no abandoned restore sweep

## Version History

### Catalog 0.0.3 (Current)
### Catalog 0.0.4 (Current)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Worth a sentence here: any Postgres target the old bug left in CREATING matches the sweep predicate and gets removed on the first control-plane pass after upgrade. Right outcome, but operators will see tables disappear from ListTables and should know why.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Added

Comment thread crates/storage-sqlite/src/backup.rs Outdated
}
let rows_sql = format!(
"SELECT rowid, item_data FROM {table} \
WHERE {cond} AND rowid > ? AND rowid <= ? ORDER BY rowid"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The restore cursor is the implicit rowid of backup_items, read across separately committed batches. backup_items has no INTEGER PRIMARY KEY, so a VACUUM between batches can renumber rowids and the restore skips or duplicates rows. Nothing in-tree runs VACUUM and #391 keeps a second extenddb out, but not a sqlite3 CLI. Give backup_items an INTEGER PRIMARY KEY or cursor on something stable.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done, backup_items has id INTEGER PRIMARY KEY and the restore cursors on it. An existing table is rebuilt in place by migrate, and the fixture test now checks id is present with rows in order.

// This session sits idle for the whole copy. A server-side
// idle_session_timeout would end it and release the lock while the
// restore is still running, so it is disabled for this session only.
sqlx::query("SET idle_session_timeout = 0")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If this SET fails (managed instance that rejects it), every restore becomes a 500. Make it best-effort with a warning.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done, now best-effort with a warning

// mid-copy leaves its target CREATING with no scheduled transition,
// which nothing above would ever move on. Logged and skipped on
// failure, so it cannot hold up the transitions above.
match self.sweep_abandoned_restores().await {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This pass is notify-driven with a 60 s idle timeout, so an abandoned target is removed between 60 and ~120 s, not "after 60 s". Each pass also opens a fresh out-of-pool connection per still-locked candidate. Fine, but say so in the comment.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Corrected the comment

.map_err(|e| StorageError::Internal(format!("Database error: {e}")))?;
.map_err(db_err)?;
sqlx::query(&format!(
"LOCK TABLE {} IN ACCESS SHARE MODE",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This lock is now held across the read and the interleaved catalog writes. A DeleteTable issued during the backup reaches the control-plane worker, whose DROP TABLE blocks behind it and stalls the whole worker pass until the backup finishes. Main had the same shape for the read only. Either SET LOCAL lock_timeout in the worker's drop or document it.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Went with using SET LOCAL lock_timeout

Comment thread crates/storage-sqlite/src/backup.rs Outdated
.unwrap_or(i64::MAX);
sqlx::query(
"INSERT INTO backup_items (backup_arn, pk, sk, item_data) \
VALUES (?, '', NULL, ?)",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

SQLite stores pk = '' while Postgres stores the real pk. Harmless today but the #348 migration-from-rows will want it. Also COPY_BATCH_ITEMS is imported but this file uses its own COPY_BATCH.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Swapped to storing the Real pk

robinnsc added a commit that referenced this pull request Oct 7, 2026
Nothing stopped two `extenddb serve` processes from opening the same SQLite
file. The backend serializes writers with a lock inside the server process,
so two servers write concurrently and can hit SQLITE_BUSY mid-transaction;
and startup recovery (control-plane transitions, GSI and vector index
rebuilds, and, with #384, abandoned restores) assumes no other server is
running, so a second server starting up would undo the first one's work in
progress. Nothing stopped `destroy` from unlinking the file under a running
server either, which left that server serving an unlinked inode while a
later `init` created a fresh database at the same path.

`extenddb serve` on a file database now takes an exclusive flock(2) on
`<database>.lock` before opening the database and holds it for the life of
the process (the lock lives in the engine, so it is held while any clone
is). `init` and `migrate` take the same lock for their duration through the
bootstrapper's migration-lock hook, and `destroy` takes it before it
removes the file; each refuses, rather than waits, when a server holds it.
`init` holds it through the encryption key, default account, and admin
user, not only the schema, so a server cannot start in the window where
the schema exists but the key does not. Read-only commands (`settings`,
`manage`, `verify`, `status`) take no lock. A second holder fails with:

  another extenddb process is already using <db> (lock held on <db>.lock);
  stop it first, or point this command at a different database

The lock file is named after the file SQLite actually opens, not the
configured string: the location is parsed with sqlx's own
SqliteConnectOptions (so `sqlite:` URL forms, percent-encoding such as
`a%20b.sqlite`, and `file:` URIs resolve as the engine resolves them), and
the path is canonicalized, so two spellings of one file, a `..` path, or a
symlink and its target all share one lock. A dangling symlink (the target
not created yet) is followed by hand, since canonicalize refuses it. A disk
file whose name happens to contain `mode=memory` is a file. In-memory
databases take no lock.

The lock file is created 0600 and an existing one is tightened to 0600 on
open, matching the database and its sidecars: flock needs only a read
handle, so a world-readable lock file would let any local user hold the
lock and keep the server from starting.

The kernel releases the lock when the process exits, including on kill -9,
so there is no stale-lock cleanup. On non-Unix platforms no lock is taken
and a warning is logged. flock is advisory and its behaviour on network
filesystems depends on the server and mount; the docs say to keep SQLite
databases on local disk. The directory holding the database must be
writable so the lock file can be created.

flock through libc rather than std's File::try_lock, which needs Rust 1.89;
the workspace MSRV is 1.88. libc is already a dependency of the workspace
and listed in every license notices file.

Docs: troubleshooting entry for the error with its scope; README and the
deployment and design guides now state that one instance per catalog is
the supported deployment, that SQLite enforces it, and that PostgreSQL and
MongoDB do not (per-instance caches without cross-instance invalidation,
every worker on every instance, a liveness-only /health). The deployment
guide previously said multiple PostgreSQL instances were consistent because
there was no in-process cache, which was not true.

Tests: database_file resolution for plain paths, sqlite: URLs,
percent-encoding, file: URIs, in-memory forms, and a disk file named like
a memory parameter; ServeLock refused on the same file, independent across
files, shared across `..` spellings, across file and directory symlinks,
and across a dangling symlink and its future target (one and two links);
the lock file created 0600 and an existing 0644 one tightened; the server
factory refusing a second server on one file; destroy and migrate refused
while a server holds the file and a server refused while migrate holds it.

Signed-off-by: Scott Robinson <robinnsc@amazon.com>
yesyayen pushed a commit to yesyayen/extenddb that referenced this pull request Oct 8, 2026
Nothing stopped two `extenddb serve` processes from opening the same SQLite
file. The backend serializes writers with a lock inside the server process,
so two servers write concurrently and can hit SQLITE_BUSY mid-transaction;
and startup recovery (control-plane transitions, GSI and vector index
rebuilds, and, with ExtendDB#384, abandoned restores) assumes no other server is
running, so a second server starting up would undo the first one's work in
progress. Nothing stopped `destroy` from unlinking the file under a running
server either, which left that server serving an unlinked inode while a
later `init` created a fresh database at the same path.

`extenddb serve` on a file database now takes an exclusive flock(2) on
`<database>.lock` before opening the database and holds it for the life of
the process (the lock lives in the engine, so it is held while any clone
is). `init` and `migrate` take the same lock for their duration through the
bootstrapper's migration-lock hook, and `destroy` takes it before it
removes the file; each refuses, rather than waits, when a server holds it.
`init` holds it through the encryption key, default account, and admin
user, not only the schema, so a server cannot start in the window where
the schema exists but the key does not. Read-only commands (`settings`,
`manage`, `verify`, `status`) take no lock. A second holder fails with:

  another extenddb process is already using <db> (lock held on <db>.lock);
  stop it first, or point this command at a different database

The lock file is named after the file SQLite actually opens, not the
configured string: the location is parsed with sqlx's own
SqliteConnectOptions (so `sqlite:` URL forms, percent-encoding such as
`a%20b.sqlite`, and `file:` URIs resolve as the engine resolves them), and
the path is canonicalized, so two spellings of one file, a `..` path, or a
symlink and its target all share one lock. A dangling symlink (the target
not created yet) is followed by hand, since canonicalize refuses it. A disk
file whose name happens to contain `mode=memory` is a file. In-memory
databases take no lock.

The lock file is created 0600 and an existing one is tightened to 0600 on
open, matching the database and its sidecars: flock needs only a read
handle, so a world-readable lock file would let any local user hold the
lock and keep the server from starting.

The kernel releases the lock when the process exits, including on kill -9,
so there is no stale-lock cleanup. On non-Unix platforms no lock is taken
and a warning is logged. flock is advisory and its behaviour on network
filesystems depends on the server and mount; the docs say to keep SQLite
databases on local disk. The directory holding the database must be
writable so the lock file can be created.

flock through libc rather than std's File::try_lock, which needs Rust 1.89;
the workspace MSRV is 1.88. libc is already a dependency of the workspace
and listed in every license notices file.

Docs: troubleshooting entry for the error with its scope; README and the
deployment and design guides now state that one instance per catalog is
the supported deployment, that SQLite enforces it, and that PostgreSQL and
MongoDB do not (per-instance caches without cross-instance invalidation,
every worker on every instance, a liveness-only /health). The deployment
guide previously said multiple PostgreSQL instances were consistent because
there was no in-process cache, which was not true.

Tests: database_file resolution for plain paths, sqlite: URLs,
percent-encoding, file: URIs, in-memory forms, and a disk file named like
a memory parameter; ServeLock refused on the same file, independent across
files, shared across `..` spellings, across file and directory symlinks,
and across a dangling symlink and its future target (one and two links);
the lock file created 0600 and an existing 0644 one tightened; the server
factory refusing a second server on one file; destroy and migrate refused
while a server holds the file and a server refused while migrate holds it.

Signed-off-by: Scott Robinson <robinnsc@amazon.com>
…; recover abandoned restores

PostgreSQL and SQLite backup and restore now reproduce the table that was
backed up, with bounded memory and crash recovery. This closes the two
restore defects of the 1.0 readiness review's P0-2 (sort keys lost on
PostgreSQL, indexes dropped on both). It does not add point-in-time
recovery or off-host backup storage; those stay with #335, #336, and #337.

Sort keys. create_backup and restore_table_from_backup on PostgreSQL
checked for a column named `sk`, which data tables do not have (sk_s,
sk_n, sk_b). Every backup of a table with a sort key lost its sort keys
and restoring one left the target in CREATING for good. Backups now store
pk and item_data only; restore derives every key column from the item.
Backups written by earlier binaries carry the whole item in item_data and
restore correctly.

Restored, from a new per-backup table definition: global and local
secondary indexes with key schemas, projections, and GSI throughput, filled
during the copy; billing mode and provisioned throughput (was always 5/5);
table class, SSE specification, on-demand throughput. Not restored, as on
the service: streams, TTL, tags, deletion protection. A definition captured
from a PAY_PER_REQUEST table carries no provisioned throughput for the
table or its indexes, even when the catalog still holds values from before
a switch from PROVISIONED, so a restored on-demand table reports what a
freshly created one does.

RestoreTableFromBackup overrides. BillingModeOverride and
ProvisionedThroughputOverride are implemented on all three backends and
passed to storage as an explicit `overrides: RestoreTableOverrides`
parameter on BackupEngine::restore_table_from_backup (a trait change).
BackupTableDefinition::apply_to applies them before the PAY_PER_REQUEST
normalization and refuses, before any target is created, a PROVISIONED
restore of a backup whose GSIs carry no throughput (the service needs
GlobalSecondaryIndexOverride there). GlobalSecondaryIndexOverride,
LocalSecondaryIndexOverride, SSESpecificationOverride,
OnDemandThroughputOverride, and VectorIndexOverride are refused with
ValidationException before any storage read; they were silently ignored.
Throughput values reuse CreateTable's validator so the error text matches.

Catalog 0.0.4. PostgreSQL migration 003 and the SQLite schema add
backup_definitions (one row per backup, the definition as JSON behind a
version marker; extenddb_storage::backup_definition) and table_restores
(one row per restored table: source backup ARN and restore time, not a
foreign key to backups so the summary outlives the backup, as on the
service). SQLite's backup_items gains `id INTEGER PRIMARY KEY`; the restore
cursors on it rather than the implicit rowid, which a VACUUM between
batches could renumber. An existing backup_items is rebuilt in place
(copy in rowid order, idempotent). backup_items.pk now holds the real
partition key on SQLite as it does on PostgreSQL. Backups and restores from
before the upgrade have no definition rows and behave as before. The
migration runners write the compiled catalog version after every walk, so
a replayed earlier migration cannot leave the version behind the schema;
they refuse to write it when the stored version is newer, and `extenddb
migrate` refuses a catalog newer than the binary outright, so an older
binary can no longer stamp its version onto a newer schema and then pass
its own startup gate. #335 also claims 0.0.4 and migration 003; whichever
merges second renumbers.

Consistency. PostgreSQL CreateBackup holds the source table row FOR SHARE
from its first catalog read until its data snapshot is taken, and the
snapshot is pinned by a real read of the table (LOCK TABLE is a utility
statement and does not establish a REPEATABLE READ snapshot; the first
revision relied on it and the definition and the items could come from
different instants). UpdateTable and DeleteTable take the row FOR UPDATE,
so the definition recorded is the one in force when the items were read.
SQLite CreateBackup takes the engine write lock only to read the
definition, insert the backup row as CREATING, and open the item reader's
WAL snapshot; the items are then streamed without the lock and written in
batches under short lock holds, so other writers wait for a batch rather
than the whole backup (a 100,000-item backup previously stalled every write
for about twelve seconds). The copy runs detached from the request, so a
client that disconnects does not leave a half-written CREATING backup;
one left by a crash is removed at startup. A CREATING backup is visible to
ListBackups and DescribeBackup, as on the service, and DeleteBackup refuses
it. The long reader pins WAL checkpointing for the backup's duration. A
file-backed SQLite pool is clamped to at least two connections so the
reader and the batch writer can coexist.

A restore registers itself in the same transaction that creates its target
on PostgreSQL (backup row FOR SHARE, table_restores inserted with the
tables row) and SQLite, so DeleteBackup orders strictly before (the restore
then sees no backup) or after (it sees the restore, BackupInUseException).
MongoDB has no cross-collection transaction and uses a claim marker on the
backup's metadata instead: a restore claims before reading and releases
once its CREATING target exists; DeleteBackup claims the same way before
dropping data; stale claims from a crashed process are cleared at startup.
DeleteTable on a table being restored returns ResourceInUseException on all
three backends. DescribeTable reports RestoreSummary for a restored table,
in progress while CREATING; the restore response and later DescribeTable
calls carry the same RestoreDateTime.

Memory and concurrency: backup and restore stream items and buffer at most
500 items or 4 MiB of stored JSON. PostgreSQL runs up to four restores at
once per process; a fifth waits up to 30 s and is refused with
LimitExceededException.

Failure and crash: a failed copy removes its target by table id, claiming
it CREATING -> DELETING in one statement so removal and activation cannot
both happen; MongoDB removes a partial target the same way. PostgreSQL
restores hold a session advisory lock keyed by account and target name on
a dedicated connection (its idle_session_timeout SET is best-effort, for
managed instances that refuse it); the control-plane pass removes CREATING
targets with no scheduled transition, older than 60 s, whose lock is free,
which with the pass's 60 s idle timeout means between 60 and about 120 s.
SQLite removes the same targets at startup, which is safe because one
server at a time holds the file (#391). The control-plane worker's
DeleteTable drop runs under a 3 s lock_timeout and drops data tables before
deleting the catalog row, so a drop blocked behind a backup's ACCESS SHARE
lock fails fast and the row stays DELETING for the next pass (the previous
order deleted the row first, so a failed drop was never retried). The
stale-vector-index recovery skips indexes whose parent table is DELETING
and cleans up if the parent vanishes under it, so the reordered drop cannot
leave an orphan vector table.

Refused rather than restored wrongly: backups of tables with vector indexes
(both backends now refuse; SQLite previously restored without them),
backups of multi-part-key tables, definitions with an unknown version, and
the unsupported overrides above. RestoreTableFromBackup validates
TargetTableName. New DynamoDbError::BackupInUseException and
StorageError::BackupInUse.

Docs: docs/dynamodb-limits.md no longer says backup and restore are
unsupported and its summary row is recounted; docs/differences-from-
dynamodb.md gains a Backup and Restore section with every row qualified by
backend (what is restored, the seven overrides, the refusals, CREATING
visibility, the delete race, crash recovery, where backups live, the
consistency model and WAL cost); the upgrade manual has a 0.0.4 section
that warns PostgreSQL operators that targets the old bug left in CREATING
are removed on the first control-plane pass after the upgrade, describes
the SQLite lock, and notes WAL headroom; the admin guide's version literal
and migrate guidance are current.

Tests: tests/test_backup_restore_fidelity.py (11, any backend: S/N/B hash
and sort keys compared item by item; a provisioned table with four GSIs and
an LSI including projections, throughput, and ordered index reads;
RestoreSummary; a PROVISIONED table switched to PAY_PER_REQUEST restoring
with zero throughput; BillingModeOverride in both directions; a refused
PROVISIONED override on a PAY_PER_REQUEST backup with a GSI leaving no
target). crates/storage-postgres/tests/backup_restore.rs (21, in the
PostgreSQL storage-level CI step: old formats, failure cleanup, the
consistency barrier including a production-path test that fails if the
snapshot-pinning read is removed and the two transaction shapes that show
why, half-built GSIs, DeleteTable and DeleteBackup racing a restore in both
orders, the abandoned-restore sweep, the drop lock_timeout leaving a row
DELETING for retry, RestoreSummary). SQLite restore_tests (16, incl. a
writer that keeps making progress during a backup, items written after the
snapshot excluded, a cancelled request finishing with every item, the
startup sweeps, a 0.0.3 database in the old row format, the backup_items
rebuild). Unit tests for the definition normalization and override
preflight (12), the engine's override parsing and refusals (15), Mongo's
claim predicates and GSI throughput handling, CatalogVersion ordering, the
newer-catalog refusals, and the vector-recovery predicate.

Signed-off-by: Scott Robinson <robinnsc@amazon.com>
@robinnsc
robinnsc force-pushed the fix/backup-restore-fidelity branch from dc980eb to 8f83169 Compare October 9, 2026 06:26
@robinnsc
robinnsc requested a review from yesyayen October 9, 2026 22:30

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants