Uh oh!
There was an error while loading. Please reload this page.
fix: prune unreferenced AttributeDefinitions on UpdateTable (re-land #274) - #280
Merged
Merged
Conversation
UpdateTable merged the request's AttributeDefinitions into the stored set but never removed any, so the stored set only ever grew. Real DynamoDB keeps it equal to the attributes actually referenced by the table key schema plus the key schemas of the live indexes, so two cases diverged: * an unused AttributeDefinition supplied alongside a GSI add was stored, where DynamoDB drops it; * deleting a GSI left behind the definitions only that index referenced. Both are visible to any client that calls DescribeTable after an UpdateTable. The effective set is now recomputed as the stored definitions merged with the request's, then pruned to those still referenced. Merging is still required: the request carries only the attributes it needs, so replacing would drop the base table's own pk/sk and degrade keyed reads to a partition-only lookup (issue #259). Pruning is the half that was missing. The recompute reuses merge_attribute_definitions rather than reimplementing the merge, which keeps the stored definition when both sides name the same attribute, so a conflicting redeclaration cannot silently retype a live index key. It runs whether or not the request carried AttributeDefinitions, because a GSI deletion prunes without the request naming anything, and it reads the surviving index key schemas after the create/delete rows are applied in the same catalog transaction, under the FOR UPDATE lock already held on the table row. Covered by tests/rust/src/update_table_attribute_definitions.rs: an unused definition supplied with a GSI add is dropped, a GSI removal drops only the definitions that index referenced while leaving the rest, a conflicting redeclaration keeps the stored type, and sequential adds accumulate.
…reSQL The prune was implemented only in the PostgreSQL backend, but tests/rust is backend-agnostic and CI runs it against MongoDB too, so mongodb-rust failed on the two tests that assert a definition is dropped. SQLite was equally behind and did not fail only because no CI job runs tests/rust against it: run-integration -sqlite runs the pytest suite, so its failure was silent rather than absent. AttributeDefinitions is DescribeTable output, so a behaviour that differs per backend is a divergence a client can see. Rather than three copies of the rule, the merge-then-prune logic moves into extenddb_storage::util as effective_attribute_definitions, next to merge_attribute_definitions it builds on, with six unit tests covering the dropped-unused case, pruning only the removed index's attributes, the table key never being pruned, stored-type precedence on a conflicting redeclaration, accumulation across sequential adds, and index sort keys counting as references. PostgreSQL is rewired onto the helper with no behaviour change. SQLite gains the prune and, like PostgreSQL, now recomputes whether or not the request carried AttributeDefinitions, because a GSI deletion prunes without the request naming anything. MongoDB needed a different shape. Its UpdateTable merges before the index create/delete loop and the loop consumes the merged set to resolve a new index's key types, so the surviving set is not known at that point. The merge stays where it is and the prune runs after the loop, reading the surviving index key schemas from describe_table_impl, which is what makes a deletion prune. That is a second catalog write; this backend's UpdateTable is non-transactional throughout and the existing merge already documents that window, so nothing is widened. Verified against all three backends rather than inferred: postgres 5/5 (4 prune + the hash-only GSI test) mongodb 4/4 via devtools/run-mongodb-tests, the same path CI uses sqlite 8/8 (prune + hash-only + the index-key validation already present) 804 workspace unit tests pass with 0 filtered out, fmt and clippy --all-targets clean.
LeeroyHannigan
requested review from
amrith, c33howard, jcshepherd, pdf-amzn, robinnsc and yesyayen
as code ownersAugust 18, 2026 09:41
Uh oh!
There was an error while loading. Please reload this page.
LeeroyHannigan
commented
Aug 18, 2026
CollaboratorAuthor
Merged under admin bypass, recorded here so it reads as deliberate rather than as a gap. This PR is a re-land of #274, which merged into The content here is a conflict-free cherry-pick of #274's two commits ( Merged via a temporary admin bypass because the reviewers who could re-approve are in timezones that are hours from being online, and this blocks the v0.1.6 release that carries the import/export fix from #277. Branch protection and the |
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Re-lands #274, which merged into
fix/gsi-add-on-hash-only-tablerather thanmain.#273 merged to
mainat 09:34:34 and #274 merged 30 seconds later, into #273's headbranch, before GitHub retargeted it. That branch had already served its purpose, so
#274's commits never reached
main:effective_attribute_definitionsis absent frommain, andcrates/storage-postgres/src/update_table.rsstill callsmerge_attribute_definitionswith no prune step.The two content commits from #274 (
a71bc7c,9d6aff9) are cherry-picked onto currentmain, which already carries #273 and #277. Both applied without conflict. No contentchange from what was approved on #274.
What this restores
UpdateTable prunes attribute definitions that are no longer referenced by the table key
schema or by a surviving index, instead of accumulating every definition ever supplied.
The merge-then-prune rule lives in
crates/storage/src/util/key.rsaseffective_attribute_definitions, with all three backends wired to it: SQLite andPostgres in
update_table.rs, and MongoDB intable_engine.rs, where the prune runsafter the index create/delete loop because that loop consumes the merged set to resolve a
new index's key types.
Verification
cargo fmt --all -- --checkexit 0,cargo clippy --all-targets -- -D warningsexit 0,cargo test --workspace831 passed with 0 filtered out.