Repository navigation
fix(storage-postgres): store attribute names containing U+0000 in the catalog - #833
Open
LeeroyHannigan wants to merge 3 commits into
Open
LeeroyHannigan wants to merge 3 commits into
LeeroyHannigan wants to merge 3 commits into
Conversation
… catalog DynamoDB accepts U+0000 in a key attribute name, an index key attribute name and a projected attribute name. DescribeTable returns each name byte for byte (measured on 2026-09-18 and 2026-10-24, and again on 2026-11-04). PostgreSQL jsonb refuses U+0000, so CreateTable with such a name answered 500 on PostgreSQL. catalog_json stores U+0000 in the jsonb documents that carry attribute names as U+0001 U+0001, and U+0001 as U+0001 U+0002. Each read applies the inverse. A name without either character keeps its stored form. The documents are tables.key_schema, tables.attribute_definitions, indexes.key_schema, indexes.projection, backups.key_schema, backups.attribute_definitions, backups.vector_indexes, backup_definitions.definition, vector_indexes.vector_attribute, vector_indexes.search_schema and vector_indexes.projection in the catalog database, and gsi_pending.index_context in the data database. Item data does not go through the escape. Migration 6, attribute_name_escape, applies the escape once to each stored PostgreSQL document that holds U+0001. It has a catalog body and a data body, and each body runs in one transaction with its ledger row. It writes one note for each column that it changed. It is breaking, because a binary without the decoder reads an escaped name as another name. It is not convergent, because an escaped U+0001 and a raw U+0001 U+0002 are the same bytes. It does not apply to SQLite and Cassandra, which store these documents as JSON text, or to MongoDB, whose BSON strings hold U+0000. The upgrade manual, ADR-0023, CONTRIBUTING.md and the sample outputs name migration 6. The CONTRIBUTING.md example becomes migration 7. The no-pgvector CI job also runs the new migrations_name_escape tests. Closes #367
LeeroyHannigan
requested review from
amrith,
c33howard,
eliseong,
hitesh96db,
jcshepherd,
navareamol,
pdf-amzn and
robinnsc
as code owners
November 4, 2026 23:52
This was referenced Nov 4, 2026
Main added migration 6, backup_catalog_index, which this branch had used for attribute_name_escape. The merge keeps main's migration 6 as it is and moves attribute_name_escape to id 7 in the registry, in the PostgreSQL module (the file, module, SOURCE and FINGERPRINT are now m0007_attribute_name_escape), in the golden fixture, which is now 0007_attribute_name_escape.txt, and in the SQLite, MongoDB and Cassandra NotApplicable arms. The RELEASED_THROUGH comment names 7. crates/storage/src/migrations/registry.rs: both sides added an entry at id 6. The result keeps main's BackupCatalogIndex at 6 and adds AttributeNameEscape at 7. crates/storage-postgres/src/migrations/mod.rs: both sides added an arm in migration() and an id in the transactional arm of runs_in(). The result has both arms and both ids. crates/storage-sqlite, storage-mongodb and storage-cassandra src/migrations/mod.rs: both sides added an arm. The result has main's BackupCatalogIndex arm followed by this branch's NotApplicable arm. crates/storage-mongodb/tests: main replaced the literal catalog version with MigrationId::LATEST.number(), which this branch had moved to 6. The result keeps main's form. migrations_fresh.rs lists id 7 as a not-applicable step and ledger row and names it in the reconverge comment, the later-binary rows use id 8, and the step counts in migrations_locks.rs and migrations_released.rs are 8. CONTRIBUTING.md: both sides renumbered the table_note example to 7. The result renumbers it to 8, the next free id after attribute_name_escape, and lists both new ids in the runs_in() example. docs/manuals/07-upgrade-manual.md: the result keeps main's sentence on migration 6 and its section on the backup catalog index, this branch's sentence that each migration is breaking on every backend it applies to, and this branch's description of the escape as migration 7. The sample outputs in the getting started guide, the quickstart, the admin guide and the install guides, and the row in the differences table, name migration 7. crates/storage-postgres/tests/catalog_nul_names.rs and migrations_name_escape.rs: main replaced BackupEngine with LegacyBackupReader, whose create and restore take a JobPermit. The two tests call create, describe, restore_source and restore with JobPermit::detached().
5 of 8 tasks
test_a_migration_6_that_fails_in_its_backfill_stays_pending_and_resumes expected migration 6 to be the only pending migration of a catalog at migration 5. This branch adds migration 7, so such a catalog has 6 and 7 pending. The check now asks for migration 6 among the pending migrations and for no pending migration below it.
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
This PR lets a PostgreSQL deployment store attribute names that hold U+0000 in its catalog. DynamoDB accepts U+0000 in a key attribute name, an index key attribute name and a projected attribute name.
DescribeTablereturns each name byte for byte (measured on 2026-09-18 and 2026-10-24, and again on 2026-11-04). PostgreSQLjsonbrefuses U+0000. So on main at 25ee109,CreateTablewith such a name answers 500 on PostgreSQL.The stored form
crates/storage-postgres/src/catalog_json.rsdefines the stored form of thejsonbdocuments that carry attribute names. The server stores U+0000 as U+0001 U+0001, and U+0001 as U+0001 U+0002. It stores every other character as itself, so a name without these two characters keeps its old stored form. Every write of these documents goes through the escape, and every read goes through the inverse. The documents are:tables.key_schema,tables.attribute_definitions,indexes.key_schema,indexes.projection,backups.key_schema,backups.attribute_definitions,backups.vector_indexes,backup_definitions.definition,vector_indexes.vector_attribute,vector_indexes.search_schemaandvector_indexes.projectiongsi_pending.index_context, which holds the index context of the queued index updatesThe item documents do not go through the escape. #368 covers U+0000 in item data.
Migration 7,
attribute_name_escapeThe migration uses the framework of ADR-0023. On PostgreSQL it applies the escape once to each stored document that holds U+0001. Releases before migration 7 stored the names raw. Without the migration, a stored name with U+0001 followed by U+0001 or U+0002 would read back as another name.
tables.key_schema: 1 document rewritten.Kind::Breaking. A binary without the decoder reads an escaped name as another name.migrate --yes --reconvergeskips it.\^@. MongoDB stores them as BSON, whose strings hold U+0000.Documentation and CI
docs/manuals/07-upgrade-manual.mddescribes migration 7 in step 4 of the general upgrade procedure.docs/differences-from-dynamodb.mdhas a row for these names.docs/adr/0023-catalog-migrations.mdand the upgrade manual state that each migration is breaking on every backend it applies to. Before this change, both named five migrations.CONTRIBUTING.mdrenumbers its example migration from 7 to 8.catalog migration 7..github/workflows/integration.ymlalso runs the new test binarymigrations_name_escape.This PR replaces the catalog part of #531. #531 added catalog version 0.0.5 with its own migration code. Main migrates through the ADR-0023 framework, so this PR builds its migration on that framework.
#671 took migration 6 on main, so this PR's migration is now 7. #368, #474, #740, #819 and #823 add migrations too. Each pull request that merges after another one of them must take the next free id.
#700's backup job writes the source table, attribute names included, to
backup_index.source_table_details_jsonthroughclaim_backupand its status write. Both bind the JSON to a JSONB column without the escape. On PostgreSQL a backup of a table with a U+0000 attribute name would fail at that insert. Whichever of #700 and this PR merges second adds the escape on those writes and the unescape on the read.The merge with main
The merge of main at 38992eb (5a0de15) moves this PR's migration from 6 to 7. It also ports
catalog_nul_names.rsandmigrations_name_escape.rstoLegacyBackupReader, which replacedBackupEnginein #671. The assertions do not change.Why
Closes #367. The issue reports two faults on PostgreSQL. #830 fixes the table name fault. This PR fixes the attribute name fault.
Testing done
On the merge commit 5a0de15, on 2026-11-05: fmt and clippy pass on
extenddb-storage,extenddb-storage-postgres,extenddb-storage-sqlite,extenddb-storage-mongodbandextenddb-storage-cassandra. Their lib tests pass: 228, 81, 257, 166 and 128. The MongoDBinterpreter_selftests(18) andpushdown_parity(20) pass. CI runs the PostgreSQL, MongoDB and Cassandra test binaries.Rust, on the branch (base: main at 25ee109):
cargo fmt --all -- --check: clean.cargo +1.97.0 clippy --locked --all-targets -- -D warnings: clean.cargo +1.88.0 build --locked --workspace: ok.cargo test --locked --workspace --exclude extenddb-backup-store, with no external store: 3061 passed, 0 failed, 61 ignored.cargo test --locked -p extenddb-storage-postgres --release --tests, againstpgvector/pgvector:pg16: 347 passed, 0 failed, 2 ignored, in 27 test binaries. The new binaries arecatalog_nul_names(2 passed),catalog_nul_index_writes(3) andmigrations_name_escape(2). In an earlier run on this branch, on a loaded host,vector_control_plane::build_ownership_uses_its_own_session_and_releases_on_dropfailed once. It passed in the two runs after that, with 42 passed each time. The test probes an advisory lock right after a drop. This PR does not change that code or that test.postgres:16without pgvector: 50 passed, 0 failed. By binary:migrations_crash9,migrations_fresh6,migrations_golden7,migrations_guards7,migrations_name_escape2,migrations_released12 andmigrations_runs7.mongo:7replica set),cargo test --locked -p extenddb-storage-mongodb --tests: 242 passed, 0 failed, 46 ignored. By binary: the library 165 (46 ignored),interpreter_selftests18,migrations_fresh8,migrations_guards7,migrations_holders10,migrations_locks10,migrations_released4 andpushdown_parity20. In the last run,migrations_holders::a_primary_that_hangs_before_a_renewal_and_answers_again_keeps_the_registrationfailed once on a loaded host, with "could not renew its registration for 4 s". It passed in the run before and in a rerun of its binary. In that file this PR changes only the expected catalog version of two other tests.cassandra:4.1):--test migrations21 passed, and--test migrations_stopped7 passed.Wire tests through boto3, against a local server per backend, from a release build of the branch. The set is
tests/python/test_nul_attribute_names.py(5 rows),test_tablesandtest_gsi.cassandra:4.1)mongo:7replica set)The 5 new rows against a main 25ee109 PostgreSQL binary: 4 failed with 500
InternalServerErroronCreateTable, and the U+0001 row passed.Against Amazon DynamoDB in us-east-1 on 2026-11-04, from 22:45 to 22:56 UTC: the 5 new rows passed.
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: ADR-0023, catalog migrations. This PR adds migration 7 under it.
Breaking changes
Migration 7 is breaking on PostgreSQL. A binary without migration 7 refuses a PostgreSQL catalog that has it, with exit status 4. To roll back, restore the backup taken before
extenddb migrate --yes. SQLite, Cassandra and MongoDB record migration 7 as not applicable, which is additive.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.