Repository navigation
Conversation
robinnsc
requested review from
LeeroyHannigan,
amrith,
c33howard,
jcshepherd,
pdf-amzn and
yesyayen
as code owners
October 16, 2026 00:53
robinnsc
force-pushed
the
fix/pg-online-gsi-backfill
branch
2 times, most recently
from
October 16, 2026 07:09
a387a60 to
a979c6f
Compare
…s continue Adding a GSI to a PostgreSQL table that was taking writes left the index with missing, extra and stale rows, or failed UpdateTable with a duplicate key. The index row was committed ACTIVE before its data table existed, the backfill paged with OFFSET under concurrent writes, and queue rows applied the image they carried, so a late row overwrote newer state. UpdateTable now creates the data table first, publishes the index CREATING, waits 1 s, takes a brief SHARE lock on the base table to wait out in-flight writes, backfills in keyset pages under FOR SHARE SKIP LOCKED, sweeps orphan rows, and flips the index ACTIVE. Writes maintain a CREATING index like an ACTIVE one. The synchronous write path, the queue and the backfill all write an item's current state, read under its row lock. A write that took more than 500 ms from its catalog read re-reads the catalog and queues a repair for any GSI it did not know about; that and the wait cover writes that read the catalog just before the index was created, which no transaction can see because the catalog and the data are separate databases. A build that fails for a transient reason stays CREATING and a background worker finishes it; one that would fail every time removes the index. On both PostgreSQL and SQLite the write path takes index attribute definitions from its own catalog read instead of the cached key info, which predated a new GSI's key attributes until UpdateTable returned, so a GSI with a sort key on a new attribute got rows without that sort key. The new pytest is xfail on MongoDB (concurrent creates of one item return HTTP 500 there regardless of indexes) and skipped on Cassandra (contended deletes return HTTP 500 and the build did not finish in CI). Signed-off-by: Scott Robinson <robinnsc@amazon.com>
robinnsc
force-pushed
the
fix/pg-online-gsi-backfill
branch
from
October 16, 2026 08:06
a979c6f to
e9e0736
Compare
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Adding a GSI with UpdateTable on PostgreSQL now produces a correct index when writes keep going during the build. Before this change the index ended up wrong or the call failed. The same cached-definitions bug on SQLite is fixed alongside.
The new module
crates/storage-postgres/src/data/gsi_build.rsdocuments the ordering argument. The build:update_table.rscreates the GSI's data table before it commits theindexesrow, and commits that row asCREATINGinstead ofACTIVE. Every write reads the catalog, so once the row commits, writes maintain the index exactly as they maintain anACTIVEone, inline or through the propagation queue.LOCK TABLE <base> IN SHARE MODEand releases it at once; the lock waits for writes already in flight. Writes arriving meanwhile queue behind the request, each holding a data pool connection, so the request has a lock timeout (one second overdeadlock_timeout, which is what lets it cancel an autovacuum) and is retried up to six times with growing pauses: with the defaultdeadlock_timeout, about 12 s of write stalls spread over 20 s in the worst case before the build is left to recovery.FOR SHARE SKIP LOCKEDand writes each item's current state into the index. Rows a writer held are then done one at a time with a waiting lock, so a page never holds row locks while it waits on another one. Pages are retried on 40P01 and 40001.ctidwith a fresh snapshot, so a writer re-creating the item at that moment keeps its row.ACTIVE. If the build fails for a transient reason (a conflict, a lock timeout, a lost connection or an empty pool, a shutdown), the index staysCREATINGand the request still succeeds. A new background worker (recovery_worker, every 60 s, with backoff per index) finishes such indexes, and also those left behind by a crash. Any other error, including a row that does not decode, counts as one that would recur: the index is removed and the request fails, as before, and the recovery worker does the same when it meets one. The removal only touches a row stillCREATING, so a second build of the same index (the recovery worker next to an UpdateTable that could not take build ownership) cannot remove an index the first one has published. The data table is dropped a second after the catalog row, and the synchronous write path treats a table that is gone as an index that is gone, so a write that read the catalog just before the removal does not fail.Queue and write path:
gsi_queue.rs: a queued GSI update now applies the item's current state, read under its row lock (resync_base_key), instead of the image the row carried. A row applied late, twice, out of order by a second server process, or after the backfill already wrote a newer state can no longer leave the index behind the table.index.rs: the inline (synchronous) path writes a GSI the way the backfill and the queue do (write_state): delete the item's rows by base key, then upsert the current state on the index primary key. LSIs keep the old image-based delete.delete_table.rs: the base data table is dropped before the index tables. The other order could deadlock with a backfill page (share lock on base rows, then index writes) and, with DeleteTable as the victim, leave data tables behind after the catalog rows were gone.tables, instead of the cachedTableKeyInfo. The engine drops that cache only when UpdateTable returns. Until then, writes built rows for a new GSI with a RANGE key on a new attribute without the sort key column, and skipped the type check on that attribute.The remaining gap. The catalog and the data are separate databases, so a write that read the catalog just before the index row committed cannot learn about the index in its own transaction. Two things cover that write:
A write whose base-table statements all returned within 500 ms holds the table's
ROW EXCLUSIVElock from the first of them until it commits, and the barrier comes at least 1 s after the catalog commit, so the barrier waits for it and the backfill sees it. A slower write repairs itself. Every source of measurement error makes a write look slower or the wait longer, which is the safe direction.A write with no transaction (a plain put or delete on a table the write believed had no index) takes the transaction path instead if it is already past 500 ms when it decides. One that becomes slow only after that queues its repair in a separate transaction after the write; if the process dies between the two, that item stays out of the index (or, for a delete, its old row stays in) until the item is next written. The module doc records this.
SQLite: SQLite took a new GSI's attribute definitions from the cached
TableKeyInfoin the same way. A write landing after the build but before the cache was dropped wrote the index row without its sort key column, so a Query on the full index key missed the item. CI found this with the new pytest. SQLite now reads the definitions with its index read (fetch_indexes_for_tablejoinstables) and uses them to build rows, queue rows, and validate keys. SQLite already reads its index set under its write lock, so it does not have the cross-database window.Other changes:
backfill_gsiintable_helpers.rsis removed.docs/design/04-component-storage.md§6.2 describes the build as implemented.integration.yml.Why
No issue filed. Found in the 1.0 readiness review (P0-4: adding a GSI under concurrent writes fails on PostgreSQL). The readiness document puts the cause in the index table's primary key. That is not it: the PK already includes the base key (
ddl.rs,(pk, base_pk, base_sk...)). The causes on main:indexesrow committedACTIVEbefore the data table existed. Writes in between failed with 42P01 (synchronous GSIs). Asynchronous ones queued rows that the worker skipped as "table dropped", so those rows were lost.backfill_gsipagedORDER BY key LIMIT 500 OFFSET nwhile writes landed. Rows were read twice, which hitduplicate key value violates unique constraintand failed UpdateTable with InternalServerError, or were skipped.Measured with a script that loads 2,000 items, runs 4 writer threads (put, update of the key, REMOVE of the key, delete, TransactWriteItems), and adds a GSI mid-traffic. On main, 3 of 3 runs ended ACTIVE but wrong: 15 to 18 rows missing, 33 to 38 extra and 40 to 50 stale out of about 1,770. With a RANGE key on a new attribute, UpdateTable failed with InternalServerError. On this branch every run matches the table: hash-only and hash+range GSIs, asynchronous and synchronous propagation, 4 and 8 writers. SQLite is not affected, because it holds its write lock for the whole backfill, and it passes the same test on main.
Not closed here
CREATINGGSI are not refused; DynamoDB refuses them with a ValidationException. Main returned the partial index while reporting itACTIVE. This change makes the status honest but does not add the read gate.CREATING, as before._id). That also happens on a table with no index at all, so it is the MongoDB write-race issue thattest_put_item_create_race.pyalready marks, not this change.TableKeyInfoon that node is not invalidated, because that happens after the storage call returns. This was already true on main.Cost
FOR SHARE(three statements for a present item instead of one or two). That lock means a queued update and a concurrent write to the same item now wait for each other, where before the worker only touched the index table; a long foreground transaction holding a row holds up its queue partition for that long.Testing done
New
tests/test_gsi_online_backfill.py: adds a GSI while four writer threads run puts, key updates, key removals, deletes and TransactWriteItems, then compares a Scan of the index with a Scan of the table (missing, extra, stale, and items indexed twice), for a hash-only GSI and a hash+range GSI on a new attribute. On main both fail on PostgreSQL. On this branch both pass on PostgreSQL and SQLite. On main, SQLite fails the hash+range case only intermittently (it did in CI); the deterministic test below covers that case. On MongoDB both are xfail and on Cassandra both are skipped (see above).New
crates/storage-postgres/tests/gsi_online_build.rs. On main every test fails except the recovery test, which needs code this PR adds.queue_rows_applied_out_of_order_converge_to_the_item: on main, the item is indexed under both its old and new key.queue_row_for_a_row_the_backfill_wrote_is_applied: on main, 23505 and the queue never drains.sync_write_over_an_existing_index_row_succeeds: on main, the put fails with 23505.sync_write_survives_a_gsi_whose_table_is_gone: two synchronous GSIs, one whose data table is gone; the write succeeds and the other index is right.concurrent_writes_during_a_build_leave_the_index_equal_to_the_table: 1,200 items and 4 writers, synchronous then asynchronous. On main, writes fail with 42P01.recovery_finishes_a_gsi_left_creating: missing, stale and orphan rows are put right, and the index becomesACTIVE.write_with_key_info_older_than_a_new_gsi_indexes_its_sort_key: a put carrying key info from before the GSI was added. On main, the index row has no sort key. The same test is added for SQLite incrates/storage-sqlite/src/update_table.rs, where it also fails without this change.New
crates/storage-postgres/tests/gsi_stale_writer.rs: a hidden test hook (pause_writes_after_index_read_for_tests) makes writes pause 3 s after their catalog read, so they write after the build has finished. It covers a new item, a key change and a delete, on the autocommit path and the transaction path. With the repair turned off by hand, the test fails: all three items are wrong in the index.Full PostgreSQL pytest run (
tests/,-n 6) against servers built from this branch and from main: no test fails on the branch that passes on main. The failures that are common to both come from the local environment (CLI lifecycle without a connection string, import/export paths not configured). The same comparison for the full SQLite pytest run: no new failures. The Rust integration suite (tests/rust,--test-threads=1) has the same 3 failures on both.Seven rounds of independent review of the design and the implementation. Each round's findings are fixed here or listed under "Not closed here".
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: n/a. There is no schema migration, no wire change and no trait change.
Breaking changes
None on the wire. On PostgreSQL, DescribeTable reports a GSI that is being built as
CREATINGinstead ofACTIVE.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.