Repository navigation
Store U+0000 in strings on PostgreSQL and MongoDB - #368
LeeroyHannigan wants to merge 6 commits into
Conversation
robinnsc
left a comment
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.
There was a problem hiding this comment.
Fixed in 4367628. refuse_if_other_clients_connected_anywhere runs the check on the data database and, when (inet_server_addr, inet_server_port, current_database) differs, on the catalog database as well; a same-database deployment is checked once. The refusal names which database is held. It now also runs before the command's first write (Bootstrapper::check_exclusive_access_for_pending_migrations, called from extenddb migrate after --yes), so a refusal leaves the catalog version where it was; the in-migration check stays for any other caller.
While testing this end to end I found a second gap in the same guard: extenddb migrate connects as the application role, and for other roles' sessions pg_stat_activity hides backend_type from a non-superuser, so the backend_type = 'client backend' filter dropped an operator's psql or a server running under another role. Autovacuum workers show the same hidden type but have no usename. The filter now counts any session with a role whose type is client backend or hidden. The existing live tests did not catch it because every pool in them is the superuser from EXTENDDB_TEST_PG_CONNECTION_STRING.
Live tests: refuses_004_while_another_client_holds_a_separate_catalog (two scratch databases, a session on the catalog alone is refused with the catalog named, nothing recorded, runs once it closes) and guard_counts_other_roles_sessions_from_a_non_superuser_pool (a plain login role runs the guard against a superuser session with no application name). Each fails against the filter it replaced. The upgrade path was also run against a deployment initialised by the main binary with separate catalog and data databases; the sequence is in the PR body under Gates.
There was a problem hiding this comment.
Still holds at 21998b5, with two changes since the 13 Oct reply. The merge of main in 0d160ad keeps the check on both databases unchanged. Testing the merged branch against an upgrade from 0.1.14 found that migrate --yes refused itself: the command opens catalog management and settings stores for the resource-tag step through the configured connection string, those sessions carried no application name, and the check counted them as foreign. c521f0d sets application_name=extenddb-migrate on that connection string when it is a PostgreSQL URL, and 21998b5 replaces an application_name the operator already set instead of keeping it, so the command's own sessions are never counted. The unit test for an existing name fails on the previous helper.
| /// 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.
There was a problem hiding this comment.
Done in 4367628: catalog 0.0.4 via migrations/003_control_chars_encoding.sql, which changes no schema and only moves the version. A 0.0.3 binary now refuses a migrated deployment with Catalog version mismatch: expected 0.0.3, found 0.0.4. Run 'extenddb migrate' (checked with the main binary against a deployment migrated by this branch), and the data-database gate still refuses a 0.0.4 binary on an unmigrated data database, so both directions are closed by checks the binaries already perform. The manual's version history has the 0.0.4 entry and states that rollback restores both databases. SQLite stays at 0.0.3, with the architecture guide saying so.
Numbering: #384 and #335 also claim catalog 003 / 0.0.4. Whichever of the three merges later renumbers; a one-statement file makes that a rename plus a constant.
There was a problem hiding this comment.
Still holds, with new numbers after the merge of main. Main's #384 added 003_backup_definitions.sql as catalog 0.0.4, so the encoding marker is now migrations/004_control_chars_encoding.sql and moves the catalog to 0.0.5 (CATALOG_VERSION in crates/storage-postgres/src/lib.rs). The gate is the same check the 13 Oct reply measured, now comparing 0.0.4 with 0.0.5: a binary that expects 0.0.4 (main since #384) finds 0.0.5 and refuses with Catalog version mismatch, and this branch refuses an unmigrated data database. I have not re-run the 0.0.4 binary against a migrated deployment since the merge.
|
|
||
| /// 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.
There was a problem hiding this comment.
I went with the code-site statement rather than a startup marker. The exposure needs a legacy attribute name containing U+0001 immediately followed by U+0001 or U+0002, and a marker here would have nothing to rewrite, so it would add an acknowledgement step to every MongoDB upgrade for that case. item_data_to_json now carries the tradeoff and points at the upgrade manual's Data Migrations section; the PR body says the same under Not in this change. If you would rather have the gate anyway, say so and I will add it as a settings document that extenddb init and extenddb migrate --yes write.
There was a problem hiding this comment.
Unchanged since the 13 Oct reply: item_data_to_json (crates/storage-mongodb/src/data/mod.rs) still states the tradeoff and points at the upgrade manual's Data Migrations section, and there is no startup marker. The offer to add one as a settings document written by extenddb init and extenddb migrate --yes stands if you want it.
b5f7204 to
ef6f7d2
Compare
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) and extenddb init records
it on fresh deployments.
Catalog 0.0.4 (migrations/003_control_chars_encoding.sql) changes no schema; it
moves the version so that a binary from an earlier release, which reads escaped
rows as literal data and reports nothing, refuses a migrated deployment through
the version check it already performs. The two gates close both directions.
extenddb migrate refuses to run 004 while any other session holds the data
database or, when the catalog is a separate database, the catalog database
(backup_items lives there). The check runs before the first write, so a refusal
leaves the catalog version where it was, and again inside the migration. The
command connects as the application role, for which pg_stat_activity hides
backend_type on other roles' sessions; the filter counts any session with a role
whose type is client backend or hidden, so an operator's psql or a server under
another role is counted and autovacuum workers are not.
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. Legacy
field names are read through the decoder without a migration; the decoder's
documentation states the exposure (a name containing U+0001 followed by U+0001
or U+0002) and points at the upgrade manual.
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, the migration count and version; live migration tests for
re-encoding, the second-run skip, the standard_conforming_strings setting, the
startup gate, the connection guard on the data database, on a separate catalog
database, and from a non-superuser pool against another role's session;
tests/test_cli_vector_catalog_migration.py covers 0.0.2 to 0.0.4 and the gate
in both directions. Docs: storage design, upgrade manual, version literals.
Closes #364
ef6f7d2 to
4367628
Compare
Conflicted files and how each was resolved: - crates/storage-postgres/migrations/003_control_chars_encoding.sql: renamed to 004_control_chars_encoding.sql. Main's #384 added 003_backup_definitions.sql (catalog 0.0.4), so the encoding marker becomes migration 004 and moves the catalog to 0.0.5. The data code migration keeps its name, 004_escape_control_chars: main added no data migration since the base, and the data ledger is separate from the catalog ledger. - crates/storage-postgres/src/migrations.rs: CATALOG_MIGRATIONS lists both 003_backup_definitions.sql and 004_control_chars_encoding.sql; the count and version tripwire now expects 4 files and 0.0.5. Main's runner changes (version convergence after the walk, pending catalog files, shape steps) and this branch's 004_escape_control_chars code both kept. - crates/storage-postgres/src/lib.rs: CATALOG_VERSION is 0.0.5. Both check_data_migrations_applied (this branch) and prepare_catalog_for_serve (main) kept; the serve path runs the catalog gate first, then the data migration check. - crates/storage-postgres/src/data/index.rs: main's multi-row insert_index_rows_multi kept, with the projected item serialized through item_to_json so index rows carry the escaped form. - crates/storage/src/util/mod.rs: both sides' modules and re-exports kept (backup_id, catalog_version, control_chars). - tests/test_cli_vector_catalog_migration.py: main's structure kept; ENCODING_MIGRATION is 004_control_chars_encoding.sql, the current version is 0.0.5 and the future version 0.0.6; the rollback to 0.0.2 deletes all three later ledger rows. The replay test deletes only the vector row, as on main, since the runner converges the version after the walk. - docs/manuals/07-upgrade-manual.md: Catalog 0.0.5 (Current) is this branch's entry, PostgreSQL only, placed above main's Catalog 0.0.4 entry; the Current Status names 0.0.5 on PostgreSQL and 0.0.4 on SQLite and Cassandra; the migration list and the Data Migrations section name the new numbers. - docs/getting-started.md, docs/manuals/01-architecture-guide.md, docs/manuals/04-quickstart-setup-guide.md: main's 0.1.14 sample output with catalog 0.0.5 for PostgreSQL; the architecture guide keeps SQLite and Cassandra at 0.0.4 with the doc guard marker. Follow-on edits for the version bump: sample output in the install manuals and the admin guide reads 0.0.5; three rows in differences-from-dynamodb.md that name catalog 0.0.4 as the version that introduced a behaviour carry the doc guard marker; catalog_converge.rs builds its scratch catalog through 004 and records 004_escape_control_chars in the data ledger, the way init leaves a deployment.
…m the decoded item Main's restore (#384) reads each backup_items document as text, parses it into an Item and writes the text into the target's item_data unchanged. On this branch that text carries the control-character escape, and the key columns and index rows are built by helpers that escape their input, so the parsed item was escaped a second time: a restored table whose items hold U+0000 or U+0001 got key columns and index rows in a form the read path does not produce. The item is now decoded with json_to_item before the key helpers see it, and the key helpers are the escaping ones from key_text, so every column is escaped exactly once; the item_data text is still copied as it is. The backup copy sizes each item from the decoded form for the same reason. The parallel Scan cross-segment check (#203) hashes the candidate key's text and compares it with the hash of the stored pk column; it now uses the escaping pk_to_text so both sides hash the same text for keys with control characters. A storage-level test restores a table whose keys, index keys, attribute names and nested values hold U+0000 and U+0001 and compares the stored rows of the base table and both index tables with the source's, then reads each item back.
… check ignores them On PostgreSQL, `migrate --yes` with 004_escape_control_chars pending refused every upgrade from 0.1.14: "3 other connection(s) hold the catalog database: extenddb from ... (no application name)". The migrate command opens catalog management and settings stores for the resource-tag step through the configured connection string, and those sessions carry no application name, so the check for foreign sessions that the data code migration runs before rewriting rows counted the command's own connections. The bootstrapper's pools carry the name and were ignored. The connection string handed to the catalog stores now carries application_name=extenddb-migrate when it is a PostgreSQL URL. Other backends' connection strings and a URL that already names an application pass through unchanged. The CLI test file gains the upgrade path a fresh init never reaches: the data ledger row for 004_escape_control_chars removed and the catalog rolled back, then migrate without --yes lists it, migrate --yes applies it and the deployment serves. The existing pre-vector test did not cover this because init records the data migration, so the exclusive-access check never ran.
…module Pure move of with_migrate_application_name, its constant and its four tests from cmd_migrate.rs into migrate_connection.rs. No behaviour change. cmd_migrate.rs goes back from 856 to 793 lines.
…on string The exclusive-access check for 004_escape_control_chars skips only sessions named extenddb-migrate. A connection string that already set application_name kept the operator's name, so migrate --yes counted its own catalog sessions as foreign and refused the upgrade. The helper now drops any application_name parameter and appends extenddb-migrate. The unit test for an existing name fails on the previous helper and passes now.
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
\^@; 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\^@escape 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 or, when the catalog is a separate database, to the catalog database (backup_itemslives there), and lists them (a server still writing during the rewrite would leave rows in the wrong encoding). The check runs before the command's first write, so a refusal leaves the catalog version where it was, and again inside the migration for any other caller. Its own connections carryapplication_name = extenddb-migrateand are excluded, andEXTENDDB_MIGRATE_IGNORE_CONNECTIONS=1skips the check for idle pooler connections. The command connects as the application role, which is not a superuser, and PostgreSQL hidesbackend_typeon other roles' sessions from it; the filter counts any session with a role whose type isclient backendor hidden, so an operator'spsqlor a server under another role is counted while autovacuum workers are not.migrations/003_control_chars_encoding.sql): changes no schema. It moves the catalog version because the stored form of strings changed, so a binary from an earlier release, which would read escaped rows as literal data and report nothing, refuses a migrated deployment through the version check it already performs. With the startup gate on the data database this closes both directions: a 0.0.3 binary refuses a 0.0.4 catalog, and a 0.0.4 binary refuses a data database without004_escape_control_chars. SQLite stays at 0.0.3; its stored form did not change. fix(backup): restore sort keys, indexes, and throughput; bound memory; recover abandoned restores #384 and fix(backup): report point-in-time recovery as unsupported #335 also claim catalog 003 / 0.0.4; whichever of the three merges later renumbers. The upgrade manual documents the procedure, the rollback (both databases from pre-migration backups), 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.\^Atext 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, confirm the connection guard refuses while an unnamed connection is open and allows once it closes, refuses when a session holds only a separate catalog database and names it, and, run from a non-superuser pool, counts a session of another role; each guard test fails against the filter it replaced.tests/test_cli_vector_catalog_migration.pycovers a 0.0.2 deployment upgrading to 0.0.4, a replayed migration, and the version gate in both directions against a built binary.-D warnings, workspace), 1,294 workspace tests with the live PostgreSQL tests enabled, MSRV 1.88, the CLI lifecycle and migration suites against the built binary, and the data-plane Python suites on servers built from this branch for all three backends (the MongoDB server failstest_put_item_create_race.pythree times; a server built from main fails the same three, which is [Bug] MongoDB: concurrent same-key PutItem surfaces E11000 as InternalServerError #356).migrate --yeswith the old server running is refused before any write and the catalog stays 0.0.3; apsqlsession holding only the catalog database is refused with the catalog named; with everything stopped the migration moves the catalog to 0.0.4, records 004, and re-encodes a legacy row holding raw U+0001; the main binary then refuses withCatalog version mismatch: expected 0.0.3, found 0.0.4; the new binary serves the legacy row unchanged and round-trips an item with U+0000 in its sort key and an attribute name.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.item_data_to_json) and in the upgrade manual instead.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 migrate command refuses before writing anything while another session holds either database. The new server refuses to start until the catalog is at 0.0.4 and the migration is recorded; a server from an earlier release refuses a migrated catalog. Downgrading means restoring both databases from pre-migration backups. A rolling deployment mixing the two releases on one data database is not supported for this release. Fresh deployments are unaffected.