Skip to content

feat(TC-1274): seek KV list ranges on a COLLATE "C" index; hold the legacy-artifact fence scan for 60 s - #374

Merged
samgbafa merged 5 commits into
mainfrom
feat/tc-1274-kv-key-c-index
Oct 19, 2026
Merged

samgbafa merged 5 commits into
mainfrom
feat/tc-1274-kv-key-c-index

Conversation

@samgbafa

@samgbafa samgbafa commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

TC-1274 / TC-1317. Ships in the release after 1.21.2. Two changes from the 2026-10-19 CPU analysis (§3 range select, §4 fence scan).

Operator step before deploy (recommended; the node does not depend on it)

The index is performance-only. The node boots and serves without it; KV lists are slower until it exists. When it is missing on a non-empty current_kv, the node logs current_kv_c_index_missing and keeps serving. Build it before the deploy so lists are fast from the first request. Order: this step first, then the node deploy. Run it on the PlanetScale primary outside any transaction (console, or psql without BEGIN):

CREATE INDEX CONCURRENTLY IF NOT EXISTS idx_current_kv_space_deleted_key_c ON current_kv (space, deleted, (key COLLATE "C"));

Verify; all three columns must be t:

SELECT i.indisvalid, i.indisready, i.indislive FROM pg_index i JOIN pg_class c ON c.oid = i.indexrelid WHERE c.relname = 'idx_current_kv_space_deleted_key_c';

If the build was cancelled or failed, the index stays INVALID and IF NOT EXISTS skips it. Drop it, then redo the first statement:

DROP INDEX CONCURRENTLY idx_current_kv_space_deleted_key_c;
  • Role and timeouts (from the Opus review, operator note): run the build as the role that owns current_kv (a role that cannot create indexes makes the build fail), and in the same session run SET statement_timeout = 0; first so a console or role statement timeout does not cancel the build (a cancelled build leaves an INVALID index: recover with the DROP above). Do not run a non-concurrent DROP INDEX or ALTER TABLE on current_kv while a node boots (it can fail that boot after the 30 s lock wait, like any migration).
  • Writes to current_kv continue during the build (CONCURRENTLY). It waits for transactions open when it starts, without blocking them.
  • Build time: 3.9 s for 1,000,000 rows (112 MB heap, 109 MB index) on an unthrottled local PostgreSQL 16. A CPU-limited primary will be slower. The production row count is not known to me.
  • Rollback: DROP INDEX CONCURRENTLY IF EXISTS idx_current_kv_space_deleted_key_c; is safe at any time; only the range select's plan changes. A binary rollback to a release before this migration also needs DELETE FROM seaql_migrations WHERE version = 'm20261020_000000_current_kv_key_c_index'; first (a node refuses to start with a recorded migration it does not ship, as for feat(TC-1274): add epoch_order (space, parent, child) index migration #341/feat(TC-1274): maintain epoch_head so writes stop scanning epoch history #343). The migration's down keeps the index.
  • Also in docs/releasing.md ("Operator steps before a node deploy").

1. idx_current_kv_space_deleted_key_c

kv_prefix_condition compares key COLLATE "C". idx_current_kv_space_deleted_key is built in the column's own collation, so under a non-C database collation (production: en_US.UTF-8) the key bounds could not be index conditions: the scan read every live key of the space and filtered.

Migration m20261020_000000_current_kv_key_c_index. Performance-only (Naomi's ruling): it never locks current_kv for a build in the boot transaction and never fails the boot because the index is missing or broken. PostgreSQL stores (key COLLATE "C") on a bare column as the column key with the pg_catalog.C collation in indcollation, not as an indexprs expression, so the shape check reads exactly that. Every row below records the migration as applied (normal seaql behaviour), so an index an operator builds later is simply picked up by the planner, with no restart.

State at boot (PostgreSQL, by the index's exact name) Behaviour
Valid, ready, live btree of that name on current_kv, columns space, deleted, key, third column collation pg_catalog.C, space and deleted in their own columns' collations (a different collation on space cannot serve space = $1: checked with EXPLAIN), default opclasses, ascending, no predicate, no expression, no INCLUDE No-op: no DDL, no lock, no warning; the operator index keeps its OID
Missing, current_kv empty (SELECT EXISTS (SELECT 1 FROM current_kv)) CREATE INDEX (instant on an empty table), in a savepoint with a 2 s lock_timeout; if that build fails (a writer appears, or the role may not build), the transaction stays usable and it is the row below with reason=build_failed
Missing, current_kv not empty Skipped. Never CREATE INDEX on a non-empty table. One WARN event current_kv_c_index_missing (reason=table_not_empty); the node boots and serves with slower KV lists
Right shape but INVALID / not ready / not live (a failed or still-running CREATE INDEX CONCURRENTLY) One WARN event current_kv_c_index_invalid, pointing at DROP INDEX CONCURRENTLY + the operator CREATE. Index left alone; boot continues
This exact name with another shape (the existing (space, deleted, key) shape, COLLATE "POSIX", another expression, DESC, other opclass, INCLUDE, partial, another table), or a non-index relation One ERROR-level event current_kv_c_index_wrong_shape, pointing at DROP INDEX CONCURRENTLY idx_current_kv_space_deleted_key_c; then the operator CREATE. Object left alone; boot continues
A different shape under a different name Ignored (the migration looks only at its own name)
SQLite, MySQL No-op

Logging (TC-961). The three events are tracing records with a constant event field and a constant message that names the index and points at docs/releasing.md, section idx_current_kv_space_deleted_key_c; they carry no key, space, DID or value (a test inserts a key and asserts it is absent from the event). Tracing is initialised (tracing_try_init, lib.rs) before TinyCloud::new runs the migrations, so they are emitted at boot. The log shipper (log-shipper/sanitize.vrl) ignores the event field: it maps a WARN/ERROR record to node_warning / node_error by exact message and otherwise to event_code=unclassified. Nothing needs registering for the records to ship (no CI check requires it), so none was added; the named event is visible in the node's own container log.

No query change is needed (confirmed): the planner matches the COLLATE "C" bounds to the new index. postgres_kv_list_range_seeks_on_the_c_collation_index makes current_kv.key ICU-en collated and explains kv_list_statement on 320 rows, with the new index dropped and present (prefix docs/ and bare docs):

without: Bitmap Heap Scan  Filter: (NOT deleted) AND key >= 'docs/' AND key < 'docs0'   Rows Removed by Filter: 300
           -> Bitmap Index Scan on idx_current_kv_space_order  Index Cond: (space = ...)

with:    Bitmap Heap Scan  Heap Blocks: exact=1
           -> Bitmap Index Scan on idx_current_kv_space_deleted_key_c
                Index Cond: (space = ...) AND (deleted = false) AND (key >= 'docs/') AND (key < 'docs0')

No Rows Removed by Filter with the index. The bare prefix is a BitmapOr of two arms on the same index. On this 320-row fixture the planner picks a bitmap scan plus a sort (ORDER BY key is in the column's collation, as before); the RCA measured an Index Only Scan on a larger table. Both are Index Cond on the key bounds. SQLite: with a trailing-/ prefix the existing index already seeks (SEARCH ... (space=? AND deleted=? AND key>? AND key<?), tested), so no change there. A bare prefix on SQLite is an OR with the exact key and is not seeked; that is unchanged and out of scope.

2. Fence-scan TTL 1 s → 60 s (EffectiveFenceCache)

LEGACY_SCAN_TTL is now a named constant; the argument is in its doc comment. Summary:

  • What the boolean gates. true (a SQL/DuckDB artifact with a non-digest name and no database_legacy_artifact row) makes every SQL/DuckDB /invoke, and the share-email SQL store, answer 503 before any database is touched. false lets the request on to resolve, which reads database_legacy_artifact and database_alias live on every request and quarantines or routes a registered legacy name.
  • The operator's fence is never cached. The configured fence and the durable database_identity_fence row are read on every call before the cache, so fence on is immediate. apply, set_alias and clear_alias all require the durable fence, and set_fence(false) runs its own uncached scan.
  • Stale false (the unsafe direction). This binary has no writer of an unregistered legacy artifact: names come from logical_name (an N2 digest) or from an alias, and resolve returns an alias only when its legacy row is registered; the legacy selector is "for N3's inventory only". The answer can turn false→true only if an older release or a restore writes one. For up to 60 s, requests for that path then resolve to the digest name (an empty database) instead of getting a 503. That is the existing 1 s window made longer; it overwrites nothing, and a digest artifact created in the window makes the later apply fail with Collision for the operator to resolve.
  • Stale true. Fail-closed: after the last legacy artifact is registered, requests keep getting a retryable 503 for at most 60 s, and only while the durable fence is off.
  • Restart / migration / instances. The cache lives in the process and starts empty. A migration run or the tinycloud-sql-identity CLI is a different process and does not invalidate it; nothing above needs that. Nodes sharing a database each hold their own entry, each at most 60 s stale. Entries age from the start of their scan.

Note on the RCA's option 1 ("turn the durable fence on"). Do not do this. A durable fence that is on makes EffectiveFenceCache::check return true, which is the 503 for all SQL/DuckDB requests (step 9 of the TC-780 runbook is fence off to resume SQL). The TTL change is the right fix while the fence stays off.

Test: effective_fence_reuses_a_legacy_scan_for_the_ttl_and_rescans_after_it (moves the entry's timestamp; no sleeping): reused at TTL - 5 s, re-run at TTL + 1 s.

Tests

  • cargo test -p tinycloud-core --lib: 505 passed against a private PostgreSQL 16.15 (ICU, en_US.utf8 cluster) and with no PostgreSQL (SQLite only, the PostgreSQL tests skip); the lib suite ran 5 times in a row plus at 96 and 200 test threads without a hang. fmt and clippy -D warnings clean.
  • Migration tests, one per branch above (PostgreSQL): valid right-shape index is a no-op (same OID, no warning, a writer holding ROW EXCLUSIVE does not hold the boot up); fresh database creates it; missing + empty creates it silently and gives the boot transaction's lock_timeout back; missing + empty with a blocking writer warns (build_failed), boots, records the migration; missing + non-empty builds nothing (no lock: a writer holding ROW EXCLUSIVE does not delay the boot), warns once with the constant message and no key, boots and records; an operator build afterwards is picked up quietly; invalid / not ready / not live index warns once and is left alone; a CREATE INDEX CONCURRENTLY still running warns invalid without waiting for it; 14 wrong-shape variants under our name (including another table's index and a table of that name) log one ERROR event; different shapes (and the exact shape) under other names are ignored. I checked the tests catch bugs: a version that always builds, one that never builds, and one that treats an invalid index as valid each fail them.
  • Test isolation (hang fix). CREATE INDEX CONCURRENTLY waits for every open transaction of its database, whatever schema it works in, so a build in the shared test database can queue behind other tests' migration transactions (and they behind it). Per-test schemas do not help. Tests that run it, or hold a transaction open while it runs, now use a database of their own (PostgresTestDatabase): this PR's migration tests, and the two m20261017_000000_epoch_order_space_index tests and the epoch_head upgrade test that also build CONCURRENTLY. I did not reproduce the hang locally on the previous head (3 runs); the isolation removes the cause, not a symptom.
  • Event capture uses one process-wide tracing subscriber installed on first use (a thread-scoped one can miss events while other threads of the parallel lib tests register the same callsites; the repo's own attestation_logging.rs note says the same).
  • Not run: MySQL (its migration chain cannot run, TC-1314); the production PlanetScale version.

Conflicts with open PRs

Checked with git merge-tree against each open PR's head, minus what already conflicts with main:

…egacy-artifact fence scan for 60 s

- m20261020_000000_current_kv_key_c_index: idx_current_kv_space_deleted_key_c ON current_kv (space, deleted, (key COLLATE "C")). The operator builds it with CREATE INDEX CONCURRENTLY first; the migration is a no-op for a valid index of that shape, fails with DROP/CREATE INDEX CONCURRENTLY instructions for an INVALID or differently shaped one, and otherwise builds it inside the boot transaction.
- EffectiveFenceCache: LEGACY_SCAN_TTL 1 s -> 60 s, with the correctness argument.
dryrun added 3 commits October 19, 2026 16:33
The migration no longer builds idx_current_kv_space_deleted_key_c on a
non-empty current_kv and never fails boot over the index. Missing on a
non-empty table, invalid, or wrong-shaped under its name now log a typed
event (current_kv_c_index_missing / _invalid / _wrong_shape) and boot
continues. It still builds the index on an empty table.

Tests that run CREATE INDEX CONCURRENTLY use a database of their own.
…ture names; check leading-column collations

The boot timeouts in the index migration tests now start when the capture
lock is held, not before. Fixture schema/database names carry a process-wide
counter, since two tests can read the same clock instant. The shape check
also requires space and deleted to use their own columns' collations.
@samgbafa

Copy link
Copy Markdown
Contributor Author

APPROVE: see TC-1274 (reviewed exact head a353d20)

@samgbafa

Copy link
Copy Markdown
Contributor Author

APPROVE: see TC-1274 (main-merge with TWO listed differences, waived by Naomi; subset of reviewed lines; vs a353d20; head faf5e3a)

Detail: merged origin/main d1854fe (1.21.3 so far: #163 pin, #371, #372, #370) into the approved head a353d20 with a merge commit. Conflicts: CHANGELOG.md and tinycloud-core/src/test_support.rs. The PR diff against main has the same 11 files. Nine files have byte-identical added/removed lines vs a353d20 (Cargo.lock, docs/releasing.md, tinycloud-core/Cargo.toml, database_migration.rs, db.rs, epoch_head.rs, migrations/m20261017_000000_epoch_order_space_index.rs, migrations/m20261020_000000_current_kv_key_c_index.rs, migrations/mod.rs; compared with the corrected +/- filter). The two differences, both a SUBSET of the reviewed lines (nothing added that was not reviewed):
(1) CHANGELOG.md: the ### Changes header (+ blank line) that #374 added already exists in main's Unreleased section (from #371), so this PR now adds only its two bullets; resolved by keeping main's entry first, then #374's two bullets.
(2) tinycloud-core/src/test_support.rs: #370 (merged) and #374 fixed the same Postgres TEST schema-name collision. Main did it inline (process-wide AtomicU64 counter: tc_test_{pid}_{nanos}_{counter}); #374 did it with a shared unique_name(prefix) helper (same name format) and also uses it for PostgresTestDatabase. Resolution: keep MAIN's inline schema code and keep #374's unique_name + PostgresTestDatabase. Effect: this PR loses 6 of its reviewed lines (its -let schema = format!(…) block and +let schema = unique_name("tc_test")); no line is added; schema names are byte-for-byte the same format; unique_name is still used (by PostgresTestDatabase, line 173), so no dead_code. Waiver: Naomi, conditional on CI fully green at this exact head incl. clippy -D warnings and the PostgreSQL-backed test job (if clippy flags anything or anything else changes: stop).

@samgbafa
samgbafa merged commit 50cb05c into main Oct 19, 2026
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant