diff --git a/crates/buzz-admin/src/main.rs b/crates/buzz-admin/src/main.rs index cd55128a51..e65fb7cb18 100644 --- a/crates/buzz-admin/src/main.rs +++ b/crates/buzz-admin/src/main.rs @@ -995,7 +995,6 @@ mod tests { fn profile_search_outcome_exit_codes_follow_the_pending_rewrite() { let pending = ProfileSearchPolicy { expression: "to_tsvector('simple'::regconfig, content)".into(), - events_populated: true, }; let (outcome, code, next_step) = profile_search_outcome(Some(&pending)); assert_eq!((outcome, code), ("rewrite_pending", 2)); @@ -1005,7 +1004,6 @@ mod tests { let rewritten = ProfileSearchPolicy { expression: "CASE WHEN (kind = 0) THEN profile_search_tsv(content) ELSE NULL END" .into(), - events_populated: true, }; assert_eq!( profile_search_outcome(Some(&rewritten)).0, diff --git a/crates/buzz-db/src/runtime/migration.rs b/crates/buzz-db/src/runtime/migration.rs index 6d9651172a..1f58976f6e 100644 --- a/crates/buzz-db/src/runtime/migration.rs +++ b/crates/buzz-db/src/runtime/migration.rs @@ -2768,12 +2768,15 @@ mod postgres_tests { /// The relay's startup warning and `buzz-admin profile-search-policy` read /// one probe. It owes nothing before the schema exists, reports the rewrite - /// pending on a populated database that upgraded through 0056, clears once - /// the maintenance script runs, and never flags a fresh install. + /// pending on a populated database that upgraded through 0056, keeps + /// reporting it after that table is emptied (0056 does not rerun, so a + /// profile published then still indexes its avatar bytes), clears once the + /// maintenance script runs, and never flags a fresh install. /// /// Mutate-bite: make 0056 rewrite unconditionally and the pending assertion - /// fails; drop the `events_populated` term from `rewrite_pending` and the - /// fresh-install assertion fails. + /// fails; gate `rewrite_pending` on a populated table again and the + /// emptied-table assertion fails while the profile published after it + /// still matches `needle` through its avatar. #[tokio::test] #[ignore = "requires Postgres"] async fn profile_search_policy_probe_tracks_the_maintenance_rewrite() { @@ -2798,13 +2801,50 @@ mod postgres_tests { .expect("probe populated database") .expect("events.search_tsv exists"); assert_eq!(policy.expression, search_tsv_expression(&pool).await); - assert!(policy.events_populated); assert!(!policy.indexes_profile_text_fields()); assert!( policy.rewrite_pending(), "populated upgrade must report the rewrite pending: {policy:?}" ); + // Emptying the table afterwards changes nothing: the migration has + // already run, the expression stays, and the next profile published + // indexes whole JSON again. + sqlx::query("DELETE FROM events") + .execute(&pool) + .await + .expect("empty events after the populated upgrade"); + let policy = db + .profile_search_policy() + .await + .expect("probe emptied database") + .expect("events.search_tsv exists"); + assert!( + policy.rewrite_pending(), + "an emptied table still owes the rewrite: {policy:?}" + ); + let community_id: uuid::Uuid = sqlx::query_scalar("SELECT id FROM communities") + .fetch_one(&pool) + .await + .expect("brownfield community survives the delete"); + sqlx::query( + "INSERT INTO events (community_id, id, pubkey, created_at, kind, tags, content, sig) \ + VALUES ($1, $2, $3, NOW(), 0, '[]'::jsonb, $4, $5)", + ) + .bind(community_id) + .bind(vec![5_u8; 32]) + .bind(vec![15_u8; 32]) + .bind(AVATAR_NEEDLE_PROFILE) + .bind(vec![25_u8; 64]) + .execute(&pool) + .await + .expect("publish a profile after the table emptied"); + assert_eq!( + needle_matches_by_kind(&pool).await, + vec![(0, Some(true))], + "a profile published after the table emptied still matches through its avatar" + ); + sqlx::raw_sql(include_str!( "../../../../scripts/maintenance/profile_search_text_fields.sql" )) @@ -2816,9 +2856,13 @@ mod postgres_tests { .await .expect("probe rewritten database") .expect("events.search_tsv exists"); - assert!(policy.events_populated); assert!(policy.indexes_profile_text_fields()); assert!(!policy.rewrite_pending(), "rewrite must clear: {policy:?}"); + assert_eq!( + needle_matches_by_kind(&pool).await, + vec![(0, Some(false))], + "the rewrite reindexes the profile on its text fields" + ); reset_public_schema(&pool).await; run_migrations(&pool).await.expect("fresh install"); @@ -2827,7 +2871,6 @@ mod postgres_tests { .await .expect("probe fresh install") .expect("events.search_tsv exists"); - assert!(!policy.events_populated); assert!(policy.indexes_profile_text_fields()); assert!( !policy.rewrite_pending(), diff --git a/crates/buzz-db/src/store/search_policy.rs b/crates/buzz-db/src/store/search_policy.rs index c6fedeb568..fe6a034050 100644 --- a/crates/buzz-db/src/store/search_policy.rs +++ b/crates/buzz-db/src/store/search_policy.rs @@ -26,8 +26,6 @@ pub const PROFILE_SEARCH_DEPLOYMENT_DOC: &str = "docs/profile-search-deployment. pub struct ProfileSearchPolicy { /// The generated expression as `pg_get_expr` prints it. pub expression: String, - /// Whether `events` holds at least one row. - pub events_populated: bool, } impl ProfileSearchPolicy { @@ -37,11 +35,13 @@ impl ProfileSearchPolicy { self.expression.contains("profile_search_tsv(content)") } - /// Whether the operator rewrite is still owed: rows exist, and kind 0 is - /// still tokenized as whole JSON. An empty table is never pending because - /// 0056 rewrites it at startup. + /// Whether the operator rewrite is still owed: kind 0 is still tokenized + /// as whole JSON. Judged on the live expression alone. The row count says + /// nothing about it: 0056 runs once, so a database that was populated then + /// and emptied since keeps the old expression for every profile published + /// from now on. pub fn rewrite_pending(&self) -> bool { - self.events_populated && !self.indexes_profile_text_fields() + !self.indexes_profile_text_fields() } } @@ -65,13 +65,7 @@ impl Db { return Ok(None); }; let expression: String = row.try_get("expression")?; - let events_populated: bool = sqlx::query_scalar("SELECT EXISTS (SELECT 1 FROM events)") - .fetch_one(&mut *connection) - .await?; - Ok(Some(ProfileSearchPolicy { - expression, - events_populated, - })) + Ok(Some(ProfileSearchPolicy { expression })) } } @@ -86,32 +80,20 @@ mod tests { THEN NULL::tsvector ELSE to_tsvector('simple'::regconfig, content) END"; #[test] - fn populated_database_on_old_expression_is_pending() { + fn old_expression_is_pending() { let policy = ProfileSearchPolicy { expression: BLOCKLIST_EXPRESSION.to_owned(), - events_populated: true, }; assert!(!policy.indexes_profile_text_fields()); assert!(policy.rewrite_pending()); } #[test] - fn rewritten_database_is_not_pending() { + fn rewritten_expression_is_not_pending() { let policy = ProfileSearchPolicy { expression: POST_0056_EXPRESSION.to_owned(), - events_populated: true, }; assert!(policy.indexes_profile_text_fields()); assert!(!policy.rewrite_pending()); } - - #[test] - fn empty_table_is_never_pending() { - // 0056 rewrites an empty table itself; nothing is owed to an operator. - let policy = ProfileSearchPolicy { - expression: BLOCKLIST_EXPRESSION.to_owned(), - events_populated: false, - }; - assert!(!policy.rewrite_pending()); - } } diff --git a/crates/buzz-relay/src/main.rs b/crates/buzz-relay/src/main.rs index b73575fc37..3a31143fcd 100644 --- a/crates/buzz-relay/src/main.rs +++ b/crates/buzz-relay/src/main.rs @@ -348,14 +348,14 @@ async fn run_relay_main(boot: BootTracker) -> anyhow::Result<()> { } }; - // Non-fatal: 0056 leaves a populated database on its old kind-0 search - // expression until an operator runs the maintenance rewrite. Say so at - // every boot until then; profile search works meanwhile, only noisier. + // Non-fatal: 0056 leaves a database that held events on its old kind-0 + // search expression until an operator runs the maintenance rewrite. Say so + // at every boot until then; profile search works meanwhile, only noisier. match db.profile_search_policy().await { Ok(Some(policy)) if policy.rewrite_pending() => warn!( script = buzz_db::search_policy::PROFILE_SEARCH_MAINTENANCE_SCRIPT, procedure = buzz_db::search_policy::PROFILE_SEARCH_DEPLOYMENT_DOC, - "Profile search still indexes whole kind-0 JSON on this populated database; \ + "Profile search still indexes whole kind-0 JSON on this database; \ run the maintenance rewrite" ), Ok(Some(_)) => {} diff --git a/crates/buzz-search/src/query.rs b/crates/buzz-search/src/query.rs index 6e56319215..05f17fbd7f 100644 --- a/crates/buzz-search/src/query.rs +++ b/crates/buzz-search/src/query.rs @@ -123,8 +123,8 @@ pub struct SearchHit { #[derive(Debug, Clone)] pub struct SearchResult { /// Hits on this page, ordered by relevance then created_at desc. For - /// `kinds == [0]` queries there is at most one hit per pubkey: the newest - /// matching live row. + /// `kinds == [0]` queries there is at most one hit per pubkey: its current + /// head (the newest live row), and only when that head itself matches. pub hits: Vec, /// 1-indexed page returned. pub page: u32, @@ -180,6 +180,15 @@ fn push_tsquery(qb: &mut QueryBuilder, mode: SearchMode, search_ } } } + +fn push_authors(qb: &mut QueryBuilder, authors: Option<&[Vec]>) { + if let Some(authors) = authors.filter(|authors| !authors.is_empty()) { + qb.push(" AND pubkey = ANY("); + qb.push_bind(authors.to_vec()); + qb.push(")"); + } +} + fn normalized_search_text(q: &str) -> Option { let trimmed = q.trim(); if trimmed.is_empty() { @@ -201,30 +210,41 @@ fn normalized_search_text(q: &str) -> Option { /// Execute a community-scoped FTS query. /// -/// SQL shape (bracketed parts are emitted only for `kinds == [0]`): +/// SQL shape: /// ```sql -/// [SELECT id, kind, pubkey, channel_id, created_at_s, rank FROM (] -/// SELECT [DISTINCT ON (pubkey)] id, kind, pubkey, channel_id, +/// SELECT id, kind, pubkey, channel_id, /// EXTRACT(EPOCH FROM created_at)::bigint AS created_at_s, /// ts_rank_cd(search_tsv, query) AS rank -/// [, created_at, @@ query AS name_match] -/// [, search_tsv] -- prefix mode, ≤ 2 chars -/// FROM events, -/// AS query -/// [, LATERAL (SELECT AS j OFFSET 0) AS profile] +/// FROM events, AS query /// WHERE community_id = $ctx /// AND deleted_at IS NULL /// AND search_tsv @@ query -/// [+ channel scope, kinds, authors, since, until] -/// [ORDER BY pubkey, created_at DESC, id) AS heads] -- newest live head per pubkey -/// ORDER BY [name_match DESC,] +/// [+ artifact head check, channel scope, kinds, authors, since, until] +/// ORDER BY rank DESC, created_at DESC, id +/// LIMIT $per_page OFFSET (($page - 1) * $per_page) +/// ``` +/// +/// For `kinds == [0]` the row source is each pubkey's current kind-0 head, +/// resolved before any search predicate runs, and the select list and ORDER BY +/// gain a name-match term: +/// ```sql +/// SELECT ..., @@ query AS name_match +/// FROM (SELECT DISTINCT ON (pubkey) id, kind, pubkey, channel_id, created_at, content, search_tsv +/// FROM events +/// WHERE community_id = $ctx AND deleted_at IS NULL AND kind = 0 [AND authors] +/// ORDER BY pubkey, created_at DESC, id) AS events, +/// AS query, +/// LATERAL (SELECT AS j OFFSET 0) AS profile +/// WHERE search_tsv @@ query +/// [+ channel scope, since, until] +/// ORDER BY name_match DESC, /// [search_tsv @@ websearch_to_tsquery('simple', $q) DESC,] -- prefix mode, ≤ 2 chars /// rank DESC, created_at DESC, id /// LIMIT $per_page OFFSET (($page - 1) * $per_page) /// ``` /// -/// `community_id = $ctx` is the first predicate and is non-negotiable. There -/// is no code path through this function that omits it. +/// `community_id = $ctx` is the first predicate on the `events` scan and is +/// non-negotiable. There is no code path through this function that omits it. #[datastore_span(name = "search", system = "postgresql")] pub async fn search(pool: &PgPool, query: &SearchQuery) -> Result { let Some(search_text) = normalized_search_text(&query.q) else { @@ -256,63 +276,74 @@ pub async fn search(pool: &PgPool, query: &SearchQuery) -> Result = QueryBuilder::new(""); - if profile_query { - // Kind 0 is replaceable, but earlier bugs may have left more than one - // live row per pubkey and nothing in the schema forbids it. Collapse - // to the newest matching head per pubkey so page slots count people, - // not rows; the outer ORDER BY below restores relevance order. - qb.push( - "SELECT id, kind, pubkey, channel_id, created_at_s, rank \ - FROM (SELECT DISTINCT ON (pubkey) ", - ); - } else { - qb.push("SELECT "); - } - qb.push( - "id, kind, pubkey, channel_id, \ + let mut qb: QueryBuilder = QueryBuilder::new( + "SELECT id, kind, pubkey, channel_id, \ EXTRACT(EPOCH FROM created_at)::bigint AS created_at_s, \ ts_rank_cd(search_tsv, search_query.query) AS rank", ); if profile_query { - // `created_at` feeds the outer ORDER BY through the wrapper. // `name_match` applies the same tsquery as the WHERE clause to the // name fields only, so both sides normalize identically. `profile.j` // is the row's content parsed once by the LATERAL below: NULL for // invalid JSON, and `->>` is NULL for a non-object document, so either // yields an empty vector and `false`; `name_match` is never NULL. qb.push( - ", created_at, to_tsvector('simple', concat_ws(' ', \ + ", to_tsvector('simple', concat_ws(' ', \ profile.j ->> 'name', \ profile.j ->> 'display_name', \ profile.j ->> 'displayName')) @@ search_query.query AS name_match", ); - if prioritize_exact_profile_lexeme { - // The outer exact-lexeme term reads `search_tsv` through the wrapper. - qb.push(", search_tsv"); - } } - qb.push(" FROM events CROSS JOIN LATERAL (SELECT "); + qb.push(" FROM "); + if profile_query { + // Kind 0 is replaceable, but earlier bugs may have left more than one + // live row per pubkey and nothing in the schema forbids it. Resolve + // each pubkey's current head, in the replaceable store's own NIP-16 + // order, before the search predicate sees any row: matching first + // would let a superseded row that still matches stand in for a current + // one that does not, resurfacing a name its owner has since changed. + // The fences below then apply to the head alone, as they would on a + // store that held one row per person, and page slots count people. + qb.push( + "(SELECT DISTINCT ON (pubkey) \ + id, kind, pubkey, channel_id, created_at, content, search_tsv \ + FROM events WHERE community_id = ", + ); + qb.push_bind(*query.community.as_uuid()); + qb.push(" AND deleted_at IS NULL AND kind = 0"); + push_authors(&mut qb, query.authors.as_deref()); + qb.push(" ORDER BY pubkey, created_at DESC, id) AS events"); + } else { + qb.push("events"); + } + qb.push(" CROSS JOIN LATERAL (SELECT "); push_tsquery(&mut qb, query.mode, &search_text); qb.push(" AS query) AS search_query"); if profile_query { - // Parse `content` once per candidate row rather than once per - // reference. `CASE` skips the cast when the guard is false, so rows - // that predate ingest-side JSON validation never raise on `::jsonb`. - // `OFFSET 0` keeps the planner from pulling the subquery up and - // re-expanding the cast at every `profile.j` reference. - // `pg_input_is_valid` needs PostgreSQL 16 or later. + // Parse `content` once per head rather than once per reference. + // `CASE` skips the cast when the guard is false, so rows that predate + // ingest-side JSON validation never raise on `::jsonb`. `OFFSET 0` + // keeps the planner from pulling the subquery up and re-expanding the + // cast at every `profile.j` reference. `pg_input_is_valid` needs + // PostgreSQL 16 or later. qb.push( " CROSS JOIN LATERAL (SELECT CASE WHEN pg_input_is_valid(content, 'jsonb') \ THEN content::jsonb END AS j OFFSET 0) AS profile", ); } - qb.push(" WHERE community_id = "); - qb.push_bind(*query.community.as_uuid()); - qb.push(" AND deleted_at IS NULL AND search_tsv @@ search_query.query"); + qb.push(" WHERE "); + if !profile_query { + qb.push("community_id = "); + qb.push_bind(*query.community.as_uuid()); + qb.push(" AND deleted_at IS NULL AND "); + } + qb.push("search_tsv @@ search_query.query"); - // Search is a current-state surface; old artifact snapshots cannot match. - qb.push(" AND (events.kind <> 45010 OR EXISTS (SELECT 1 FROM artifact_heads ah WHERE ah.community_id=events.community_id AND ah.event_id=events.id AND NOT ah.deleted))"); + if !profile_query { + // Search is a current-state surface; old artifact snapshots cannot + // match. The kind-0 head subquery pins `kind = 0`, so it never needs this. + qb.push(" AND (events.kind <> 45010 OR EXISTS (SELECT 1 FROM artifact_heads ah WHERE ah.community_id=events.community_id AND ah.event_id=events.id AND NOT ah.deleted))"); + } // Channel scope — see `ChannelScope` doc for the four-case mapping. The // emitted SQL fragments are identical to the legacy 2x2 tuple for the @@ -337,20 +368,16 @@ pub async fn search(pool: &PgPool, query: &SearchQuery) -> Result Result| SearchQuery { + community: c, + q: q.into(), + channel_scope: ChannelScope::Any, + kinds: Some(vec![0]), + authors: None, + since: None, + until, + page: 1, + per_page: 10, + mode: buzz_search::SearchMode::Prefix, + }; + + let wes = svc + .search(&profile_query("wes", None)) + .await + .expect("profile prefix search ok"); + assert!( + wes.hits.is_empty(), + "a superseded head must not match: {:?}", + wes.hits + ); + + let alice = svc + .search(&profile_query("alice", None)) + .await + .expect("profile prefix search ok"); + let ids: Vec<[u8; 32]> = alice.hits.iter().map(|h| h.event_id).collect(); + assert_eq!( + ids, + vec![current], + "the current head matches on its own name" + ); + + // A window that excludes the current head finds nobody, rather than the + // superseded row that falls inside it. + let windowed = svc + .search(&profile_query("wes", Some(1_700_000_015))) + .await + .expect("profile prefix search ok"); + assert!( + windowed.hits.is_empty(), + "`until` fences the head, it does not resurrect a superseded row: {:?}", + windowed.hits + ); + + teardown(pool, &schema).await; +} + #[tokio::test] #[ignore = "requires Postgres"] async fn prefix_mode_matches_final_token_prefix_without_changing_full_text() { diff --git a/docs/profile-search-deployment.md b/docs/profile-search-deployment.md index 6bc9acc056..50d66c62cf 100644 --- a/docs/profile-search-deployment.md +++ b/docs/profile-search-deployment.md @@ -22,23 +22,25 @@ startup probe can kill and repeat. The query-side name-first ordering in this at every boot until the rewrite has run: ```text -WARN Profile search still indexes whole kind-0 JSON on this populated database; run the maintenance rewrite script=scripts/maintenance/profile_search_text_fields.sql procedure=docs/profile-search-deployment.md +WARN Profile search still indexes whole kind-0 JSON on this database; run the maintenance rewrite script=scripts/maintenance/profile_search_text_fields.sql procedure=docs/profile-search-deployment.md ``` ## Who needs the rewrite Any deployment that held events before starting a relay version with migration -0056. Fresh installs do not. Check without side effects, from a shell with the -relay's `DATABASE_URL`: +0056. Fresh installs do not. Emptying `events` later does not help: the +migration has already run, so the old expression stays and every profile +published afterwards indexes its whole JSON again. Check without side effects, +from a shell with the relay's `DATABASE_URL`: ```sh buzz-admin profile-search-policy ``` -The command opens one read-only session and prints the live expression, -whether `events` has rows, and `outcome`: `ok` (exit 0), `rewrite_pending` -(exit 2), or `missing` (exit 5, the column does not exist yet). The same probe -drives the startup warning, so the two never disagree. +The command opens one read-only session and prints the live expression and +`outcome`: `ok` (exit 0), `rewrite_pending` (exit 2), or `missing` (exit 5, +the column does not exist yet). The same probe drives the startup warning, so +the two never disagree. The equivalent SQL: