diff --git a/AGENTS.md b/AGENTS.md index 27322f8ee8..31298d0ada 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -17,8 +17,9 @@ Before planning or reviewing a non-trivial change: 4. For any change to `migrations/`, `schema/`, or schema-dependent `buzz-db` queries, read [docs/database.md](docs/database.md): no new triggers, functions, or foreign keys; no data backfills in migrations; update every - schema location; and keep every migration compatible with the relay that - is currently deployed. + schema location; keep every migration compatible with the relay that + is currently deployed; and never make a migration refuse a populated + table. 5. Check that the proposed design advances, or at least does not contradict, that product intent. Call out any intentional tension explicitly. diff --git a/crates/buzz-db/src/runtime/migration.rs b/crates/buzz-db/src/runtime/migration.rs index 924140b1f5..ef2259dde3 100644 --- a/crates/buzz-db/src/runtime/migration.rs +++ b/crates/buzz-db/src/runtime/migration.rs @@ -53,6 +53,10 @@ pub(crate) async fn run_migrations_through(pool: &PgPool, target: i64) -> Result async fn run_migrations_locked(conn: &mut PgConnection) -> Result<()> { reject_legacy_nip_rs_cardinality_ambiguity(conn).await?; + for (version, doc, indexes) in INLINE_INDEX_BUILDS { + MIGRATOR.run_to(version - 1, &mut *conn).await?; + build_indexes_before_migration(conn, *version, doc, indexes).await?; + } MIGRATOR.run(&mut *conn).await?; // The replica-fence proof (see `replica_fence`) requires the commit-time // `created_at` floor trigger from migration 0021 — correctly shaped — on @@ -65,6 +69,122 @@ async fn run_migrations_locked(conn: &mut PgConnection) -> Result<()> { Ok(()) } +/// Indexes that a migration verifies but refuses to build on a populated +/// table, keyed by that migration's version, with the document that describes +/// its concurrent prebuild, as `(name, definition)`. +/// +/// Migrations 0076, 0078 and 0081 are checksum-frozen and expect every +/// populated database to prebuild these indexes with `CREATE INDEX +/// CONCURRENTLY`. Startup migration builds a missing index here instead, under +/// the same bounded budgets 0049 uses, so a small populated database upgrades +/// like an empty one and a large one still fails closed. `CONCURRENTLY` is not +/// used: a replica blocked on the migration lock holds a snapshot that a +/// concurrent build would wait for. New migrations build their own indexes +/// inline (docs/database.md Rule 6); the `frozen_prebuild_guards_have_inline_builds` +/// test keeps this list equal to the migrations that refuse populated tables. +type InlineIndexBuilds = &'static [(&'static str, &'static str)]; + +const INLINE_INDEX_BUILDS: &[(i64, &str, InlineIndexBuilds)] = &[ + ( + 76, + "docs/artifact-sync-deployment.md", + &[( + "idx_artifact_revisions_artifact", + "CREATE INDEX idx_artifact_revisions_artifact \ + ON public.artifact_revisions (community_id, artifact_id)", + )], + ), + ( + 78, + "docs/artifact-sync-deployment.md", + &[ + ( + "artifact_heads_modified", + "CREATE INDEX artifact_heads_modified \ + ON public.artifact_heads (community_id, modified_at, artifact_id)", + ), + ( + "artifact_heads_channel", + "CREATE INDEX artifact_heads_channel \ + ON public.artifact_heads (community_id, channel_id, artifact_id)", + ), + ( + "channel_members_sync", + "CREATE INDEX channel_members_sync \ + ON public.channel_members (community_id, pubkey, sync_modified_at, channel_id)", + ), + ( + "artifact_revisions_moves", + "CREATE INDEX artifact_revisions_moves \ + ON public.artifact_revisions (community_id, modified_at, event_id) \ + WHERE source_channel_id IS NOT NULL", + ), + ], + ), + ( + 81, + "docs/profile-search.md", + &[( + "idx_nip_fi_profile_heads_names", + // The exact pg_get_indexdef form migration 0081 verifies. + r#"CREATE INDEX idx_nip_fi_profile_heads_names ON public.nip_fi_profile_heads USING gin (to_tsvector('simple'::regconfig, ((((COALESCE((jsonb_path_query_first(event, 'strict $."tags"[*]?(@[0] == "display_name")[1]'::jsonpath, '{}'::jsonb, true) #>> '{}'::text[]), ''::text) || ' '::text) || COALESCE((jsonb_path_query_first(event, 'strict $."tags"[*]?(@[0] == "username")[1]'::jsonpath, '{}'::jsonb, true) #>> '{}'::text[]), ''::text)) || ' '::text) || COALESCE((jsonb_path_query_first(event, 'strict $."tags"[*]?(@[0] == "full_name")[1]'::jsonpath, '{}'::jsonb, true) #>> '{}'::text[]), ''::text))))"#, + )], + ), +]; + +/// Build each missing index in `indexes` in its own bounded transaction +/// unless migration `version` is already recorded, so no build holds its +/// writer-conflicting lock past its own budget. An existing index, valid or +/// not, is left for the migration to verify, and is skipped before +/// `CREATE INDEX` so a prebuilt deployment never requests that lock. +async fn build_indexes_before_migration( + conn: &mut PgConnection, + version: i64, + doc: &str, + indexes: InlineIndexBuilds, +) -> Result<()> { + let applied: bool = + sqlx::query_scalar("SELECT EXISTS (SELECT 1 FROM _sqlx_migrations WHERE version = $1)") + .bind(version) + .fetch_one(&mut *conn) + .await?; + if applied { + return Ok(()); + } + let build = async { + for (name, definition) in indexes { + let exists: bool = sqlx::query_scalar("SELECT to_regclass($1) IS NOT NULL") + .bind(format!("public.{name}")) + .fetch_one(&mut *conn) + .await?; + if !exists { + let mut tx = conn.begin().await?; + sqlx::raw_sql("SET LOCAL lock_timeout = '1s'; SET LOCAL statement_timeout = '5s'") + .execute(&mut *tx) + .await?; + sqlx::raw_sql(*definition).execute(&mut *tx).await?; + tx.commit().await?; + } + } + Ok::<(), sqlx::Error>(()) + }; + build.await.map_err(|error| { + // Only a lock or statement timeout means the table is too large or + // too busy for the startup budget; pass any other failure through. + let over_budget = error + .as_database_error() + .and_then(|db| db.code()) + .is_some_and(|code| code == "55P03" || code == "57014"); + if !over_budget { + return error.into(); + } + crate::DbError::InvalidData(format!( + "migration {version} blocked: building its indexes within the startup budget failed \ + ({error}); prebuild them concurrently as described in {doc}, then retry" + )) + }) +} + /// Run `op` while holding the exclusive schema/destruction session lock. /// /// `op` receives ownership of the detached connection that owns the advisory @@ -2074,9 +2194,9 @@ mod postgres_tests { if *path == this_file { assert_eq!( (macro_hits, run_hits, run_to_hits), - (1, 1, 1), + (1, 1, 2), "migration.rs must embed the migrator once, run it once in production, \ - and expose exactly one test-only bounded run" + and have one test-only and one production bounded run" ); } else if *path == push_gateway_exception { continue; @@ -2113,9 +2233,16 @@ mod postgres_tests { source[entry..locked].contains("with_exclusive_schema_destruction_lock("), "run_migrations must delegate through the exclusive schema/destruction lock" ); + let production_run_to_site = source[run_to_site + 1..] + .find(&migrator_run_to) + .map(|offset| run_to_site + 1 + offset) + .expect("production bounded migrator run site"); assert!( - run_site > locked && run_site < wrapper, - "the production migrator run site must live inside run_migrations_locked" + run_site > locked + && run_site < wrapper + && production_run_to_site > locked + && production_run_to_site < run_site, + "the production migrator run sites must live inside run_migrations_locked" ); assert!( run_to_site > entry @@ -3107,6 +3234,226 @@ mod postgres_tests { .expect("second post-apply reconciliation"); } + /// Migrations 0076 and 0078 refuse a populated table whose index was not + /// prebuilt concurrently. A relay that auto-migrated through earlier + /// releases and holds artifact data must still boot through every migration. When a + /// bounded inline build cannot take its lock it must fail closed without + /// recording that migration, keeping earlier migrations and each index + /// already built in its own transaction. + #[tokio::test] + #[ignore = "requires Postgres"] + async fn populated_artifact_tables_auto_migrate_through_every_migration() { + let pool = connect_test_pool().await; + reset_public_schema(&pool).await; + MIGRATOR + .run_to(75, &pool) + .await + .expect("apply migrations 1-75"); + + let community_id = uuid::Uuid::new_v4(); + sqlx::query("INSERT INTO communities (id, host) VALUES ($1, $2)") + .bind(community_id) + .bind(format!("artifact-{}.example", community_id.simple())) + .execute(&pool) + .await + .expect("insert community"); + let artifact_id = uuid::Uuid::new_v4(); + sqlx::query( + "INSERT INTO artifact_revisions (community_id, event_id, artifact_id) \ + VALUES ($1, $2, $3)", + ) + .bind(community_id) + .bind(vec![1_u8; 32]) + .bind(artifact_id) + .execute(&pool) + .await + .expect("insert artifact revision"); + sqlx::query( + "INSERT INTO artifact_heads \ + (community_id, artifact_id, event_id, channel_id, artifact_type) \ + VALUES ($1, $2, $3, $4, 'doc')", + ) + .bind(community_id) + .bind(artifact_id) + .bind(vec![1_u8; 32]) + .bind(uuid::Uuid::new_v4()) + .execute(&pool) + .await + .expect("insert artifact head"); + + let mut holder = pool.begin().await.expect("begin lock holder"); + sqlx::query("LOCK TABLE artifact_revisions IN ROW EXCLUSIVE MODE") + .execute(&mut *holder) + .await + .expect("hold a writer lock"); + let blocked = run_migrations(&pool) + .await + .expect_err("a build that cannot take its lock must fail closed") + .to_string(); + assert!( + blocked.contains("migration 76 blocked") + && blocked.contains("docs/artifact-sync-deployment.md"), + "unexpected error: {blocked}" + ); + assert_eq!(applied_versions(&pool).await.last().copied(), Some(75)); + holder.rollback().await.expect("release writer lock"); + + // A prebuilt index is skipped without requesting the build lock, so + // the same held writer lock first blocks 0077's ALTER TABLE instead. + sqlx::query( + "CREATE INDEX idx_artifact_revisions_artifact \ + ON public.artifact_revisions (community_id, artifact_id)", + ) + .execute(&pool) + .await + .expect("prebuild the 0076 index"); + let mut holder = pool.begin().await.expect("begin second lock holder"); + sqlx::query("LOCK TABLE artifact_revisions IN ROW EXCLUSIVE MODE") + .execute(&mut *holder) + .await + .expect("hold a writer lock again"); + let blocked = run_migrations(&pool) + .await + .expect_err("0077 cannot alter a table under a held writer lock") + .to_string(); + assert!( + !blocked.contains("migration 76 blocked"), + "a prebuilt index must not be rebuilt: {blocked}" + ); + assert_eq!(applied_versions(&pool).await.last().copied(), Some(76)); + holder.rollback().await.expect("release second writer lock"); + + // Block the third 0078 build. The two artifact_heads builds before it + // commit separately, so neither holds its lock while the third waits. + run_migrations_through(&pool, 77) + .await + .expect("apply migration 77"); + let mut holder = pool.begin().await.expect("begin third lock holder"); + sqlx::query("LOCK TABLE channel_members IN ROW EXCLUSIVE MODE") + .execute(&mut *holder) + .await + .expect("hold a channel_members writer lock"); + let blocked = run_migrations(&pool) + .await + .expect_err("a blocked 0078 build must fail closed") + .to_string(); + assert!( + blocked.contains("migration 78 blocked"), + "unexpected error: {blocked}" + ); + assert_eq!(applied_versions(&pool).await.last().copied(), Some(77)); + let committed: bool = + sqlx::query_scalar("SELECT to_regclass('public.artifact_heads_modified') IS NOT NULL") + .fetch_one(&pool) + .await + .expect("read earlier 0078 build"); + assert!( + committed, + "an earlier build must commit before the next one" + ); + holder.rollback().await.expect("release third writer lock"); + + run_migrations(&pool) + .await + .expect("populated small database auto-migrates"); + assert_boots_through_every_migration(&pool).await; + assert_eq!( + valid_indexes( + &pool, + &[ + "idx_artifact_revisions_artifact", + "artifact_heads_modified", + "artifact_heads_channel", + "channel_members_sync", + "artifact_revisions_moves", + ] + ) + .await, + 5 + ); + } + + /// Migration 0081 refuses a populated `nip_fi_profile_heads`. A relay that + /// has published NIP-FI verified profiles must still boot through every + /// migration. + #[tokio::test] + #[ignore = "requires Postgres"] + async fn populated_verified_profiles_auto_migrate_through_every_migration() { + let pool = connect_test_pool().await; + reset_public_schema(&pool).await; + MIGRATOR + .run_to(80, &pool) + .await + .expect("apply migrations 1-80"); + sqlx::query( + "INSERT INTO nip_fi_profile_heads (community_id, pubkey, issuer, issued_at, event) \ + VALUES ($1, $2, $3, 1, $4)", + ) + .bind(uuid::Uuid::new_v4()) + .bind("a".repeat(64)) + .bind("b".repeat(64)) + .bind(serde_json::json!({"tags": [["display_name", "Wes Johnson"]]})) + .execute(&pool) + .await + .expect("insert verified profile head"); + + run_migrations(&pool) + .await + .expect("database with verified profiles auto-migrates"); + assert_boots_through_every_migration(&pool).await; + assert_eq!( + valid_indexes(&pool, &["idx_nip_fi_profile_heads_names"]).await, + 1 + ); + } + + async fn assert_boots_through_every_migration(pool: &PgPool) { + let embedded: Vec = MIGRATOR.iter().map(|migration| migration.version).collect(); + assert_eq!( + applied_versions(pool).await, + embedded, + "a populated database must boot through every embedded migration" + ); + } + + async fn valid_indexes(pool: &PgPool, names: &[&str]) -> i64 { + sqlx::query_scalar( + "SELECT count(*) FROM pg_index i JOIN pg_class c ON c.oid = i.indexrelid \ + WHERE c.relname = ANY($1) AND i.indisvalid AND i.indisready AND i.indislive", + ) + .bind(names) + .fetch_one(pool) + .await + .expect("read index flags") + } + + /// A migration that refuses a populated table crash-loops every + /// auto-migrating relay with data. Only the frozen ones may, and each must + /// have an inline build the startup runner applies first. + #[test] + fn frozen_prebuild_guards_have_inline_builds() { + let dir = std::path::Path::new(env!("CARGO_MANIFEST_DIR")).join("../../migrations"); + let mut guarded: Vec = std::fs::read_dir(&dir) + .expect("read migrations dir") + .filter_map(|entry| { + let path = entry.expect("read migration entry").path(); + let name = path.file_name()?.to_str()?.to_owned(); + let version: i64 = name.split('_').next()?.parse().ok()?; + let sql = std::fs::read_to_string(&path).expect("read migration"); + sql.contains("RAISE EXCEPTION 'prebuild ") + .then_some(version) + }) + .collect(); + guarded.sort_unstable(); + let inline: Vec = INLINE_INDEX_BUILDS.iter().map(|(v, _, _)| *v).collect(); + assert_eq!( + guarded, inline, + "a migration must build its own index instead of refusing a populated table \ + (docs/database.md Rule 6); only frozen guards listed in INLINE_INDEX_BUILDS may remain" + ); + assert_eq!(inline, [76, 78, 81], "do not add new prebuild guards"); + } + #[tokio::test] #[ignore = "requires Postgres"] async fn pre_0007_ambiguous_nip_rs_data_blocks_without_mutation_and_allows_retry() { diff --git a/docs/artifact-sync-deployment.md b/docs/artifact-sync-deployment.md index e4bf6abb82..14148e7920 100644 --- a/docs/artifact-sync-deployment.md +++ b/docs/artifact-sync-deployment.md @@ -13,8 +13,11 @@ New application writers stamp every head change, grant, rejoin, removal, redaction, and move source. Changes to channel listing also require recovery. Old application updates do not maintain all these stamps, so mixed-version serving cannot advertise incremental correctness. -For a populated database, first build the revision lookup index outside a -transaction, before migration `0076`: +Relay startup migration and `buzz-admin migrate` build the indexes below +themselves within the migrations' budgets. A deployment too large for those +budgets runs with auto-migration off; before applying the migrations out of +band, first build the revision lookup index outside a transaction, before +migration `0076`: ```sql CREATE INDEX CONCURRENTLY idx_artifact_revisions_artifact @@ -39,11 +42,20 @@ CREATE INDEX CONCURRENTLY artifact_revisions_moves ``` Migration `0078` verifies each index definition and its valid, ready, and live -flags. It refuses a populated table whose index is missing. A fresh empty +flags. Applied directly as SQL, it refuses a populated table whose index is +missing. A fresh empty database can build the indexes under the migration's short budgets. Migration `0076` also verifies the exact definition and all three flags. Remove an invalid index with `DROP INDEX CONCURRENTLY` before a new build. +Relay startup migration and `buzz-admin migrate` build any of these indexes +that are missing before applying `0076` and `0078`. They use the migrations' +one-second lock and five-second statement budgets, which is enough for a small +database. Each index builds in its own transaction. A database too large for that +budget fails closed with an error that points here. The migration whose index +failed is not recorded, while earlier migrations and indexes stay committed. +Prebuild the rest concurrently as above, then retry. + After all index builds succeed, run `buzz-admin migrate` with the target `DATABASE_URL`. It verifies and records `0076`, reapplies the idempotent `0077`, then verifies and records `0078`. Direct SQL application does not record the diff --git a/docs/database.md b/docs/database.md index 0a3c9b58e7..b96c0c6876 100644 --- a/docs/database.md +++ b/docs/database.md @@ -205,9 +205,10 @@ deletes to ship first, then the constraint drop as a separate contract step. ### 3. Backfills are not migrations A migration MUST contain schema changes only. It MUST NOT `UPDATE`, `INSERT`, -or `DELETE` existing rows, and MUST NOT scan or rewrite a table while holding a -write-blocking lock. Scans that do not block writes, such as -`VALIDATE CONSTRAINT` or a bounded concurrent index build, are allowed (Rule 6). +or `DELETE` existing rows, and MUST NOT scan or rewrite a large table while +holding a write-blocking lock. Scans that do not block writes, such as +`VALIDATE CONSTRAINT` or a concurrent index build, are allowed, as is a plain +`CREATE INDEX` under Rule 6's lock and statement budgets (Rule 6). - Historical data repair MUST be a backfill definition that conforms to [data-backfills.md](data-backfills.md): stable ID, finite upper bound, @@ -410,11 +411,24 @@ parity assertion on the table for that window. `SET LOCAL statement_timeout` when a statement can scan. A migration that cannot get its lock fails and is retried. It MUST NOT queue behind traffic on a hot table. -- In `migrations/`, an index on an existing populated table MUST be built - with `CREATE INDEX CONCURRENTLY`, because auto-migrating deployments have no - online schema change tool to build it for them. CONCURRENTLY cannot run - inside the migration transaction, so follow the prebuild-then-verify - procedure in [thread-window-deployment.md](thread-window-deployment.md). +- With `BUZZ_AUTO_MIGRATE` on, migrations are the whole upgrade procedure, so + every migration MUST apply its own change on a database that holds data. A + migration MUST NOT refuse to run because a table has rows: every + auto-migrating relay with data would crash-loop on that version until + someone intervened by hand. +- In `migrations/`, a new index on an existing table is built by the + migration itself. Skip `CREATE INDEX` when `to_regclass` finds the index, + otherwise build it inline under the migration's lock and statement budgets, + then verify its definition and flags, as `0049` does. A deployment too + large for those budgets runs with auto-migration off and applies migrations + out of band; only such a deployment prebuilds the exact index with + `CREATE INDEX CONCURRENTLY` (procedure in + [thread-window-deployment.md](thread-window-deployment.md)) before applying + the migration, which then verifies it. Frozen migrations `0076`, `0078` and + `0081` predate this rule; the startup runner builds their indexes first + (`INLINE_INDEX_BUILDS` in `crates/buzz-db/src/runtime/migration.rs`). A + source test rejects any other migration that raises `'prebuild …'`; it + matches that wording only, so review must catch other refusals. Partitioned parents need per-partition concurrent builds attached to an `ON ONLY` parent index. - The declarative schema source declares indexes with plain `CREATE INDEX`. @@ -456,8 +470,8 @@ description: test for each changed table and index. - [ ] That the currently deployed relay works against the new schema, and how that was checked. -- [ ] Lock and statement timeouts, concurrent index builds in `migrations/`, - and any operator prebuild step. +- [ ] Lock and statement timeouts, how each new index is built inline, and + any prebuild step for deployments that apply migrations out of band. ## Related documents diff --git a/docs/profile-search.md b/docs/profile-search.md index 2e30ae6ac0..5d70db4883 100644 --- a/docs/profile-search.md +++ b/docs/profile-search.md @@ -65,9 +65,11 @@ never matches and never makes the search fail ## Deployment -Verified-name matching reads `idx_nip_fi_profile_heads_names`. Migration -`0081` builds it only while `nip_fi_profile_heads` is empty; on a populated -database it fails until the index exists. Build it first, outside a +Verified-name matching reads `idx_nip_fi_profile_heads_names`. Relay startup +migration and `buzz-admin migrate` build it before migration `0081` under a +one-second lock and five-second statement budget. Applied directly as SQL, +`0081` builds it only while `nip_fi_profile_heads` is empty. A deployment that +applies migrations out of band (auto-migration off) builds it first, outside a transaction: ```sql diff --git a/migrations/AGENTS.md b/migrations/AGENTS.md index 82d72c94a9..34cafd615f 100644 --- a/migrations/AGENTS.md +++ b/migrations/AGENTS.md @@ -24,3 +24,6 @@ Before changing anything in this directory, read migration fails closed if its backfill has not run, and its release notes name the writer release that upgrades must not skip. - Bound every lock (`SET LOCAL lock_timeout`). +- Build a missing index inline under the lock and statement budgets, as + `0049` does. Never refuse a table only because it has rows: that crash-loops + every auto-migrating deployment with data, however small.