From bb550dbadfab914005c6afbf114c861aac2d4c35 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Fri, 2 Oct 2026 16:55:27 +1000 Subject: [PATCH] fix(search): match the resolved kind-0 head; judge rewrite by expression Addresses the two open findings from the review of PR #8044. The third, the stale migration-version pin, was fixed in c2b3426d6. Profile search applied `search_tsv @@ query` before `DISTINCT ON (pubkey)`, so it kept the newest *matching* live row, not the current head. With two live kind-0 rows for one pubkey, an older `Wes` and a newer `Alice`, a search for `wes` dropped the current row and returned the superseded one, exposing a name and `about` text the user had removed. Make the head subquery the row source instead: it resolves each pubkey's newest live kind-0 row in the replaceable store's `created_at DESC, id ASC` order with only the identity fences inside it (community, `deleted_at IS NULL`, `kind = 0`, authors), and the search predicate, channel scope, and since/until apply to the resolved head, as they would on a store holding one row per person. The outer wrapper that carried `created_at` and `search_tsv` through `heads` goes away because the outer query reads them from the subquery directly; the artifact-head predicate and `kind = ANY` are skipped for kind 0 since the subquery pins the kind. Non-kind-0 searches emit the same SQL as before; `push_authors` serves both paths. `ProfileSearchPolicy::rewrite_pending` required `events_populated`, so a database that was populated when 0056 ran and emptied afterwards reported `ok` at relay startup and from `buzz-admin profile-search-policy` while every new profile still indexed its whole JSON, because 0056 never reruns. Judge the rewrite on the live expression alone and drop the `events_populated` field and its query. The relay warning no longer says "populated", and docs/profile-search-deployment.md records that emptying the table does not help. Tests. `kind0_search_resolves_the_current_head_before_matching` seeds a superseded `Wes` head under a current `Alice` head and asserts that `wes` finds nobody, `alice` finds the current row, and an `until` window that excludes the current head finds nobody rather than the stale row. With the search predicate moved back into the head subquery it fails at the first assertion (the stale row returns) while the existing duplicate-heads test still passes. `profile_search_policy_probe_tracks_the_maintenance_ rewrite` gains the sequence populated upgrade, `DELETE FROM events`, probe still pending, a new avatar-only profile matches `needle`, maintenance rewrite clears the probe and reindexes that row. The `empty_table_is_never_pending` unit test goes with the term it covered. Plan check on a scratch copy of the local database (PostgreSQL 17.9) seeded with 20k chat rows, 3,000 profiles including 300 with ~50 KB avatar tsvectors, and 500 duplicate heads: the head subquery is bounded to the community's kind-0 rows (`idx_events_addressable` on mixed partitions), 3,500 rows sorted in ~1 MB; 1.7 ms against 0.4 ms for the old shape on the post-0056 expression and 6.7 ms against 4.8 ms on the whole-JSON expression. On that data the old shape returned the renamed `Wes 7` for `wes`; the new shape does not. Verified with the lld linker workaround for sqlx proc-macros on this machine: `scripts/postgres-test-run.sh -p buzz-search --tests` (24 passed), `scripts/postgres-test-run.sh -p buzz-db --lib -E 'test(migration::postgres_tests)'` (13 passed), `cargo test -p buzz-db --lib search_policy`, `cargo test -p buzz-admin profile_search`, `cargo fmt --all --check`, and `cargo clippy -p buzz-search -p buzz-db -p buzz-admin -p buzz-relay --all-targets -- -D warnings`. Co-Authored-By: Claude Fable 5.1 Signed-off-by: Matt Toohey --- crates/buzz-admin/src/main.rs | 2 - crates/buzz-db/src/runtime/migration.rs | 57 ++++++- crates/buzz-db/src/store/search_policy.rs | 36 ++--- crates/buzz-relay/src/main.rs | 8 +- crates/buzz-search/src/query.rs | 152 ++++++++++-------- .../tests/postgres_fts_integration.rs | 97 ++++++++++- docs/profile-search-deployment.md | 16 +- 7 files changed, 255 insertions(+), 113 deletions(-) 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: