Uh oh!
There was an error while loading. Please reload this page.
fix: prune unreferenced AttributeDefinitions on UpdateTable - #274
Merged
LeeroyHannigan merged 2 commits intoAug 18, 2026
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.
LeeroyHannigan
requested review from
amrith, c33howard, jcshepherd, pdf-amzn and yesyayen
as code ownersAugust 16, 2026 12:21
…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
merged commit Aug 18, 2026
7184c94
into
fix/gsi-add-on-hash-only-table
13 checks passed
Uh oh!
There was an error while loading. Please reload this page.
LeeroyHannigan added a commit
that referenced
this pull request
Aug 18, 2026
…274) (#280) * fix: prune unreferenced AttributeDefinitions on UpdateTable 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. * fix: prune AttributeDefinitions on all three backends, not just PostgreSQL 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. --------- Co-authored-by: Lee Hannigan <amrithie@amrith.org>
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
2 participants
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.
Stacked on #273. Based on
fix/gsi-add-on-hash-only-tableso the diff shows only this change; retarget tomainonce #273 merges.UpdateTablemerged the request'sAttributeDefinitionsinto 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, both visible to any client callingDescribeTableafter anUpdateTable:AttributeDefinitionsupplied alongside a GSI add was stored, where DynamoDB drops it;Fix
The effective set is recomputed as the stored definitions merged with the request's, then pruned to those still referenced.
Merging is still required and is not being undone here: the request carries only the attributes it needs, so replacing the set drops the base table's own
pk/skand degrades keyed reads to a partition-only lookup (#259, fixed in #261). Pruning is the half that was missing.Two details worth a reviewer's attention:
merge_attribute_definitionsrather 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;AttributeDefinitions, because a GSI deletion prunes without the request naming anything. It reads the surviving index key schemas after the create/delete rows are applied in the same catalog transaction, under theFOR UPDATElock already held on the table row.Verification
tests/rust/src/update_table_attribute_definitions.rs, 4 tests, all passing:These use hash-only tables, which is why this stacks on #273: against
mainthey fail at table setup with the unrelated 500 that #273 fixes.cargo fmt --all --check,cargo clippy --all-targets -- -D warnings, andcargo test --workspace(798 passed, 0 failed, 0 filtered out) are clean.