Repository navigation
fix(storage-postgres): store attribute names containing U+0000 in the catalog - #833
Open
LeeroyHannigan wants to merge 6 commits into
Open
LeeroyHannigan wants to merge 6 commits into
LeeroyHannigan wants to merge 6 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.
LeeroyHannigan
added
release-blocker
Collaborator
|
General: you'll want to merge and resolve conflicts, and it may expand the scope of this PR. After merging from main, a restore will read the escaped names as the literal names, so a restored GSI keyed on a U+0001 name will have no items. crates/storage-postgres/src/restore_engine.rs:64-91 and crates/storage-postgres/src/backup_engine.rs:76-97 (on main) #740 added the new restore job, and #700 added the new backup job. They merged after this PR's base. There are three places where escaped columns are read or written "raw": restore_target_shape (restore_engine.rs:64-91 on main) reads tables.key_schema and tables.attribute_definitions with serde_json::from_value. |
Collaborator
|
The migration number is also going to need to be bumped again. Ping me after you merge with main, and I can approve. |
jcshepherd
requested changes
Nov 9, 2026
jcshepherd
left a comment
jcshepherd
left a comment
Collaborator
There was a problem hiding this comment.
Needs to be synced from main and conflicts fixed; additionally main will bring down a couple new areas where escaping will be needed. Once that is taken care of and the migration number refreshed, I can approve.
Main added migrations 7 (stream_pending), 8 (restore_progress) and 9 (warm_throughput). This branch used id 7 for attribute_name_escape. The merge keeps main's ids and moves attribute_name_escape to id 10. The registry, the PostgreSQL module and the SQLite, MongoDB and Cassandra NotApplicable arms use the new id. The module file is now m0010_attribute_name_escape.rs, with SOURCE and FINGERPRINT to match. The golden fixture is now 0010_attribute_name_escape.txt. The RELEASED_THROUGH comment names 10. crates/storage/src/migrations/registry.rs: both sides added an entry at id 7. The result keeps main's entries at 7, 8 and 9 and adds AttributeNameEscape at 10. crates/storage-postgres/src/migrations/mod.rs: both sides added arms in migration() and ids in the transactional arm of runs_in(). The result has main's three arms, then this branch's arm, and all four ids. crates/storage-sqlite, storage-mongodb and storage-cassandra src/migrations/mod.rs: both sides added arms. The result has main's arms, then this branch's NotApplicable arm. crates/storage-sqlite/src/migrations/tests.rs and released_tests.rs: both sides changed two tests to count only the migrations that have a body on SQLite. The result is main's form of each test. In tests.rs, main checks the applied and not_applicable counts, where this branch checked the applied count and the total. In released_tests.rs, both forms count the convergent migrations that have a body on SQLite. The Migration import that this branch added is no longer used, so the result drops it. crates/storage-mongodb/tests/migrations_fresh.rs: the result lists id 10 as a not-applicable step and ledger row, and the reconverge comment names it. The later-binary ledger rows use id 11, and the LATEST assertion is 10. migrations_locks.rs and migrations_released.rs count 11 steps. crates/storage-postgres/src/create_table.rs: main added the warm throughput record of each global secondary index. This branch writes the key schema and projection of that index through to_catalog_json. The result keeps both. crates/storage-postgres/src/backup_engine.rs: main moved the index read of the legacy restore copy into restore_indexes(). This branch read the key schema and projection in that copy through from_catalog_json. The result calls restore_indexes() from the copy, and restore_indexes() reads both documents through from_catalog_json. The import line keeps main's RestoreSource and this branch's catalog_json functions. CONTRIBUTING.md: main renumbered the table_note example to 10. The result renumbers it to 11, the next free id after attribute_name_escape. The runs_in() example lists main's three ids and AttributeNameEscape. docs/manuals/07-upgrade-manual.md: the result keeps main's text on migrations 6 to 9. This branch's sentences on the escape follow the paragraph on migration 9 and name migration 10. The sentence on kinds is main's, with migration 10 added to the breaking migrations. Step 4 of the general upgrade procedure names migration 10. docs/getting-started.md, docs/manuals/04-quickstart-setup-guide.md, 08-install-linux.md and 09-install-macos.md: the sample outputs show migration 10. The admin guide example and the row in the differences table also name migration 10. crates/storage-postgres/tests: main added warm_throughput to GsiInput, so the literals in catalog_nul_names.rs and catalog_nul_index_writes.rs set it to None. migrations_name_escape.rs names migration 10 in its test names, skip messages and assertions.
The backup job (#700) and the restore job (#740) merged after this branch's base. They read and write catalog documents that carry attribute names without the escape of catalog_json. - backup_catalog.rs: claim_backup and set_status bind backup_index.source_table_details_json through escape_json, and row_from_tuple reads it through unescape_json. Without the escape in claim_backup, PostgreSQL refuses a source table with U+0000 in a name, and CreateBackup fails. Without the escape in set_status, the job's last write fails, and the backup stays CREATING. Without the unescape, DescribeBackup returns the stored form of each name. - restore_engine.rs: restore_target_shape reads tables.key_schema and tables.attribute_definitions through from_catalog_json. Without it, a store-backed restore looks for the stored form of each key name on the items, and the restore fails. The merge commit before this one reads indexes.key_schema and indexes.projection through from_catalog_json in restore_indexes(), which main split out of the legacy restore copy. Migration 10 also rewrites backup_index.source_table_details_json, which migration 6 adds. The module docs of catalog_json.rs and of the migration, and step 4 of the upgrade manual, list the column. Tests: - catalog_nul_backup_jobs.rs is new. The first table has a U+0000 name and a U+0001 name in its key schema, and a GSI keyed on a U+0001 name. The backup job writes it to a filesystem store, and the restore job restores it. DescribeBackup and DescribeTable of the restored table return each name byte for byte. - The second table has items. Its key names and the key of one GSI hold U+0001, and the key of a second GSI holds U+0000. After the backup and the restore, a Query on the restored GSI returns the items. The PostgreSQL item columns cannot hold a name with U+0000 yet. So only the first table has U+0000 in its key schema, and it has no items. - The third test writes a backup_index row through claim_backup and set_status, and reads it back unchanged. It checks that the stored text holds the stored form of each name. - migrations_name_escape.rs: the golden test seeds two backup_index rows as a release before migration 10 wrote them. The fixture shows the raw names rewritten, the other row unchanged, and the note "backup_index.source_table_details_json: 1 document rewritten".
Main at 887b6e2 adds two commits since the previous merge. #842 lists what the audit log records. #840 refuses access key import unless allow_credential_import is true. Neither commit adds a migration, so attribute_name_escape stays at id 10. The merge has no conflicts. Both sides edited docs/manuals/07-upgrade-manual.md, docs/differences-from-dynamodb.md, docs/getting-started.md and docs/manuals/05-admin-guide.md. Main changed the text on credential import and audit logging, and added a v0.1.15 behavior change that needs no catalog migration. This branch changed the migration list, the sample outputs and the escape. The changes are in separate paragraphs and table rows. The Current Status section lists migrations 1 to 10. Each sample output and example still names migration 10, so the merge changes no line beyond main's.
Collaborator
Author
|
@jcshepherd I merged main twice, 35edb77 in b3bd377 and 887b6e2 in cdd89ca. The migration is now 10. The three places you named now go through
Migration 10 also rewrites I searched The new test binary |
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,backup_index.source_table_details_json,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 10,
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 10 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.\u0000. MongoDB stores them as BSON, whose strings hold U+0000.The backup and restore jobs
#700's backup job writes the source table, attribute names included, to
backup_index.source_table_details_jsonthroughclaim_backupand its status write.row_from_tuplereads that column for everybackup_indexread. Without the escape, the insert for a source table with a U+0000 attribute name fails on PostgreSQL. #740's restore job readstables.key_schemaandtables.attribute_definitionsinrestore_target_shape, andindexes.key_schemaandindexes.projectioninrestore_indexes. Withrestore_indexesraw, a restored GSI keyed on a U+0001 name has no items. Withrestore_target_shaperaw, the restore of a table with a U+0001 key name removes its target.7739167 escapes both writes and decodes the reads in
row_from_tupleandrestore_target_shape.restore_indexesgets its decode in the merge commit b3bd377. Main moved the index read into that function. This PR decodes that read, so the merge moved the decode with it. Migration 10 rewritesbackup_index.source_table_details_jsontogether with the other catalog documents.Migration 3,
sort_key_definitions, is convergent, somigrate --yes --reconvergeruns it again on a catalog at migration 10. There it reads the stored form. Itsrepair_tablesstep compares and writes stored names, so it stays consistent. Itsrepair_backupsstep looks up a missing sort key name in the item data, which holds raw names. That lookup misses a sort key name that holds U+0000 or U+0001. The case needs a backup that lost its sort key definition to theUpdateTablefault of #259, written to a migrated catalog by an older release. This PR does not change migration 3.Documentation and CI
docs/manuals/07-upgrade-manual.mddescribes migration 10 in step 4 of the general upgrade procedure, withbackup_index.source_table_details_jsonin its list of documents.docs/differences-from-dynamodb.mdhas a row for these names.docs/adr/0023-catalog-migrations.mdstates that each migration is breaking on every backend it applies to. Before this change, it named five migrations. The upgrade manual keeps main's sentence, which now says that migrations 1 to 7, 9 and 10 are breaking.CONTRIBUTING.mdrenumbers its example migration from 10 to 11.catalog migration 10..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.
Main now runs migrations 1 to 9: #671 took 6,
stream_pendingis 7, #740 took 8 and #474 took 9. So this PR's migration is now 10. #368, #819 and #823 add migrations too. Each pull request that merges after another one of them must take the next free id.The merges with main
The merge of main at 38992eb (5a0de15) moved this PR's migration from 6 to 7. It also ported
catalog_nul_names.rsandmigrations_name_escape.rstoLegacyBackupReader, which replacedBackupEnginein #671. The assertions did not change.The merge of main at 35edb77 (b3bd377) moves the migration from 7 to 10. It keeps main's migrations 7 to 9 in the registry, in each backend's migration list, and in the MongoDB and SQLite migration tests. It also sets main's new
GsiInput.warm_throughputfield in this PR's tests. The merge of main at 887b6e2 (cdd89ca) has no conflicts.Why
Closes #367. The issue reports two faults on PostgreSQL. #830, now merged, fixes the table name fault. This PR fixes the attribute name fault.
Testing done
On 2026-11-10. The tip cdd89ca differs from 7739167 only by main's #842 and #840, which change docs and
crates/server. The runs marked "escape commit" ran on 7739167 before its last change, which adds deadlines to two waits incatalog_nul_backup_jobs. After that change the binary passed again, 3 of 3, in apostgres:16container.cargo fmt --all -- --check: clean on each of the three new commits.cargo +1.97.0 clippy --locked --all-targets -- -D warnings: clean on b3bd377 forextenddb-storage,extenddb-storage-sqlite,extenddb-storage-mongodb,extenddb-storage-cassandraandextenddb-storage-postgres. Clean on 7739167 forextenddb-storage-postgres, the one crate that commit changes.cargo check --locked --workspaceon the tip: clean.extenddb-storage240,extenddb-storage-postgres84,extenddb-storage-sqlite283,extenddb-storage-mongodb172 (66 ignored),extenddb-storage-cassandra129 andextenddb-engine636 passed. Two SQLitebackup_catalog::write_lock_testsfailed on the test host. There the filesystem backup store refuses its root, because a parent directory belongs to another user. Both pass in a container.cargo test --locked -p extenddb-storage-postgres --testson the escape commit, againstpgvector/pgvector:pg16: 409 passed and 2 ignored, in the library and 30 test binaries. 5 more tests failed on the test host for the same store root reason: 3store_identitytests inbackup_restorefrom main, and 2 tests of the newcatalog_nul_backup_jobs. Both binaries pass in apostgres:16container, 3 of 3 each.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) on the escape commit:migrations_fresh8,migrations_locks11 andmigrations_released5 passed.The new binary
catalog_nul_backup_jobshas three tests:backup_indexrow keeps such names throughclaim_backupand the status write.One at a time, a raw
restore_target_shape,restore_indexes,row_from_tupleor status write fails at least one of these tests. The code of the merge commit b3bd377 fails all three. The golden test of migration 10 covers abackup_indexrow that holds U+0001.Earlier, on the branch at base 25ee109, before the merges of main:
cassandra:4.1):--test migrations21 passed, and--test migrations_stopped7 passed.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 10 under it.
Breaking changes
Migration 10 is breaking on PostgreSQL. A binary without migration 10 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 10 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.