Store U+0000 in strings on PostgreSQL and MongoDB - #368
LeeroyHannigan wants to merge 1 commit into
Conversation
robinnsc
left a comment
There was a problem hiding this comment.
One blocking item (the connection guard's coverage, inline on migrations.rs), one strong question on the downgrade hazard, one non-blocking question on the MongoDB asymmetry. The escape design itself checks out: I traced the order/prefix/collision argument, the migration's non-idempotence handling (per-table transaction plus progress marker, with the startup gate guaranteeing only legacy rows are ever scanned), and the backup path — create-backup copies stored rows verbatim from the data tables, so escaped form flows through backup_items transitively and there is no unescaped write path I could find.
| // (an older release) or double-escaped (this release); see | ||
| // `refuse_if_other_clients_connected`. | ||
| if std::env::var_os(IGNORE_CONNECTIONS_ENV).is_none() { | ||
| refuse_if_other_clients_connected(data_pool).await?; |
There was a problem hiding this comment.
Blocking: this guard runs against the data pool only, but 004 also rewrites backup_items in the catalog database. When catalog and data are separate databases, a session holding only the catalog DB isn't counted, and a writer there during the rewrite leaves backup_items rows in the wrong encoding. The guard should also run against catalog_pool when the two databases differ — the application_name exclusion already covers the migrate command's own connections on both.
| /// migration ran would have its own rows rewritten by it (see | ||
| /// [`escape_legacy_control_chars`]). `PostgresEngine::check_data_migrations_applied` | ||
| /// refuses to start on a data database missing any of these. | ||
| pub(crate) const REQUIRED_DATA_CODE_MIGRATIONS: &[&str] = &["004_escape_control_chars"]; |
There was a problem hiding this comment.
On the downgrade hazard: an older binary started against a migrated database silently reads escaped rows as literal data — the worst failure mode in this change, and the mitigation is a sentence in the manual. The PR description notes a catalog version bump would make old binaries refuse and defers it as a release decision. Since the version-gate mechanism already exists and the failure is silent corruption rather than an error, I'd like the bump to ride in this PR or the stack — or at minimum a tracked issue with an owner, so it doesn't live only in a doc note.
|
|
||
| /// Reverse of [`json_to_item_data`]: BSON read from storage back to JSON with | ||
| /// the object-key escape undone. | ||
| pub(crate) fn item_data_to_json(value: &bson::Bson) -> Result<serde_json::Value, StorageError> { |
There was a problem hiding this comment.
Non-blocking: this decoder changes how a legacy field name containing U+0001 U+0001 or U+0001 U+0002 reads back — silently, with no migration and no startup gate, disclosed only in the upgrade manual. The exposure is genuinely narrow (attribute names containing U+0001), so skipping the migration is defensible, but Postgres got a startup refusal for the same class of hazard and MongoDB got a paragraph. Is a one-time startup marker (refuse until the operator acknowledges, like the Postgres gate but with nothing to rewrite) cheap enough to close the asymmetry? If not, a sentence here pointing at the manual section would at least make the tradeoff visible at the code site.
DynamoDB accepts the character U+0000 anywhere a string appears: keys, index
keys, values, strings in lists, map keys, attribute names (measured against the
service 2026-09-18; it sorts as the byte 0x00 and is a different key from the
six-character text "\u0000"). PostgreSQL TEXT rejects the byte and jsonb
rejects the escape, so the backend answered 500 for every such item and for a
Query whose key value carried it. BSON field names are C strings, so MongoDB
answered 500 when the character appeared in an attribute name or map key.
Shared escape (crates/storage/src/util/control_chars.rs): U+0000 is stored as
U+0001 U+0001, U+0001 as U+0001 U+0002, everything else unchanged. Exhaustive
tests over every string up to length 4 on {U+0000, U+0001, U+0002, a} pin round
trip, UTF-8 byte order, prefix preservation, and distinctness, so COLLATE "C"
comparisons, BETWEEN, begins_with, and the row-comparison page cursors return
the rows the raw text would.
PostgreSQL: item documents go through item_to_json/json_to_item (every
item_data write, stream records, gsi_pending rows and contexts, vector rows);
key column text goes through data/key_text.rs wrappers at every producing site;
the TTL index and sweep and the vector search filters address attributes by the
escaped name. Migration 004_escape_control_chars re-encodes rows written by
earlier releases that contain U+0001, one table per transaction with a
per-table progress marker, only rows containing U+0001 touched, predicate
independent of standard_conforming_strings. The server refuses to start on a
data database that does not record the migration (rows this build writes are
indistinguishable from legacy rows to the migration), extenddb init records it
on fresh deployments, and extenddb migrate refuses to run it while other
sessions hold the data database.
MongoDB: item_to_document and its inverse escape object keys only (BSON string
values already hold the byte); conditions and native updates on names with
these characters use the existing in-process fallback paths, as names with "."
or "$" already do; field paths with no fallback use the escaped name.
Tests: tests/test_nul_strings.py (14 wire cases passing against DynamoDB and
all three backends, plus a stream record case on ExtendDB servers); unit tests
on the escape, the key wrappers, the MongoDB document conversion and
predicates; live migration tests for re-encoding, the second-run skip, the
standard_conforming_strings setting, the startup gate, and the connection
guard. Docs: storage design, upgrade manual.
Closes #364
b5f7204 to
ef6f7d2
Compare
Closes #364.
What changes
DynamoDB accepts the character U+0000 anywhere a string appears. Measured against the service on 2026-09-18 with a scratch table: PutItem accepts it inside a partition key, a sort key, a GSI key, a non-key value, a string in a list, a map key, and a top-level attribute name; GetItem returns each byte-identical; it sorts as the byte 0x00 ahead of every other character; it is a different key from the six-character text
\u0000; range and prefix conditions treat it as an ordinary byte; the GSI holds the item; DeleteItem on the key removes it. The service also accepts it in key schema attribute names, and rejects it in table and index names by the usual pattern.ExtendDB's PostgreSQL backend returned
500 InternalServerErrorfor every one of the item cases (TEXTrejects the byte in key columns;jsonbrejects the\u0000escape initem_data), and for a Query whose key value contains it. The MongoDB backend returned 500 when the character appeared in an attribute name or map key (BSON field names are C strings). SQLite already matched the service.Both backends now store the character and return it unchanged.
Shared escape
crates/storage/src/util/control_chars.rsadds an order-preserving escape: U+0000 is stored as U+0001 U+0001, U+0001 as U+0001 U+0002, every other character unchanged. Strings without either character are stored exactly as before, so ordinary data does not change on disk. Unit tests enumerate every string up to length 4 over the alphabet {U+0000, U+0001, U+0002, a} and check that the escape round-trips, preserves UTF-8 byte order, preserves prefixes, and never collides, soCOLLATE "C"comparisons,BETWEEN,begins_with, and the row-comparison page cursors from #361 and #362 give the same answers on escaped columns as on the raw text.PostgreSQL
item_to_jsonandjson_to_itemindata/mod.rsapply and reverse the escape on every string in the tree (names, map keys, values). Everyitem_datawrite in the crate goes through it (put, update, transactions, index rows, vector index rows), as do stream records, queued index updates and their contexts ingsi_pending, and the vector search reader.data/key_text.rswraps the sharedpk_to_text,composite_pk_to_text, andparse_skso string key components are escaped before binding; every key-producing site in the crate imports the wrappers, so writes, reads, index maintenance, cursors, and the propagation queue agree on the stored form. Composite partition keys escape each part before netstring joining, matching what the Query builder does with a key condition.item_datauses the escaped name: the TTL index and sweep, and vector search filters (name and value).004_escape_control_chars: rows written before this change hold their strings raw, and a raw U+0001 followed by U+0001 or U+0002 would read back differently under the new decoder (a lone U+0001 reads back unchanged). The operator-run migration rewrites rows containing U+0001 in every data, index, and vector table (text key columns anditem_data),gsi_pending,stream_records, andbackup_items, one table per transaction with a per-table progress marker so an interrupted run resumes without rewriting a table twice. Only rows containing U+0001 are touched. It scans each table once, so it takes time proportional to data size. Legacy rows never contain U+0000, since PostgreSQL refused them. The migration cannot tell a row this build wrote from a legacy row (an escaped U+0001 is stored as U+0001 U+0002, which is also a legal legacy sequence), so the server refuses to start on a data database that does not record the migration, with a message naming it and the command to run;extenddb initrecords it on a fresh deployment. That ordering is what makes the migration safe: it only ever sees rows written by older builds. The candidate predicate spells its backslash aschr(92)so it reads the same under either value ofstandard_conforming_strings.extenddb migrate --yesrefuses to run the migration while any other session is connected to the data database and lists them (a server still writing during the rewrite would leave rows in the wrong encoding); its own connections carryapplication_name = extenddb-migrateand are excluded, andEXTENDDB_MIGRATE_IGNORE_CONNECTIONS=1skips the check for idle pooler connections. The upgrade manual documents the procedure, the downgrade hazard (an earlier release started against a migrated database reads escaped rows as literal data), and the MongoDB legacy field-name limitation.MongoDB
BSON string values are length-prefixed and already held the character; field names could not.
item_to_documentand its inverse escape object keys only. Conditions and native updates whose attribute names contain U+0000 or U+0001 fall back to the existing in-process paths, the same way names containing.or starting with$already do. Field paths with no fallback (the TTL index, sweep, and field reads) use the escaped name.Documentation
docs/design/04-component-storage.mddescribes the escape beside key and item storage;docs/manuals/07-upgrade-manual.mddocuments the migration.Tests
tests/test_nul_strings.py: 14 wire tests encoding the measurements above (round trip of an item with the character in every position, partition byte order with U+0000 first, U+0000 distinct from its escape text, distinct partition keys, range and prefix conditions, Limit 1 cursor walk, GSI query on a U+0000 key, filter and condition expressions on a U+0000 attribute name, UpdateItem SET and REMOVE with U+0000 names and values, BatchWriteItem, TransactWriteItems, BatchGetItem, TransactGetItems, Scan count, DeleteItem), plus a stream test (INSERT, MODIFY, REMOVE records for such an item, with Keys, NewImage, and OldImage byte-identical) that runs against ExtendDB servers only, as the existing stream tests do. The 14 pass against DynamoDB itself and all 15 against all three backends on this branch. Before the change: 14 errors on PostgreSQL and MongoDB, 14 passing on SQLite; the stream test fails on both changed backends.\u0001text row and plain rows byte-identical, confirm the second run is skipped, confirm jsonb rows are re-encoded withstandard_conforming_stringsoff, confirm the startup gate reports the migration until it is recorded, and confirm the connection guard refuses while an unnamed connection is open and allows once it closes.-D warnings, workspace), 1,230 workspace tests with the live PostgreSQL tests enabled, MSRV 1.88, and the full Python integration suites on servers built from this branch for all three backends.Not in this change
jsonb. Table and index names cannot contain it on the service either, but PostgreSQL answers a U+0000 table name with500 Internal error during authorizationwhere the service and the other backends answer the patternValidationException. Both are catalog and authorization paths, separate from item storage: [Bug] PostgreSQL: U+0000 in key schema attribute names returns 500; U+0000 in a table name fails in authorization instead of validation #367.Deploy note
PostgreSQL deployments upgrade in this order: stop every server that uses the data database, run
extenddb migrate --yeswith the new binary, start the new servers. The new server refuses to start until the migration is recorded, and a server from an earlier release still writing during the migration would leave unescaped rows behind. A rolling deployment mixing the two releases on one data database is not supported for this release. Fresh deployments are unaffected.