From 05ee07318ad4d4ddd3f795be1906c7ddac47f2fe Mon Sep 17 00:00:00 2001 From: Pavel Grigorenko Date: Sat, 9 May 2026 13:12:24 +0200 Subject: [PATCH 1/3] events: denormalize kind onto event_tags so tag-CTE can pre-filter by kind MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit For NIP-29 group reads (`{kinds: [9,11,12], #h: ['']}`), the materialized tag CTE filters by tag only — it can't see the kind predicate. Hot groups whose tag rows are dominated by membership events (kinds 9000/9021) feed ~95k useless rows into the hash-join just to throw them away on the kind filter, dominating CPU on the read path. Production observation that motivated this: 50k users × ~2 membership events per join (9021 + 9000) makes the `h='general'` tag set ~98k rows, of which only ~1.4k are actual chat. Each subscription read pays for the full 98k. PI top SQL showed two query shapes accounting for ~65% of all DBLoad on db.m6g.large pegged at 99.7% CPU. Issue #23. Changes: - event_tags gets a nullable `kind` column. Fresh schemas have it via CREATE TABLE; existing schemas pick it up via migration 002. - saveEventWith writes the kind alongside (event_id, key, value). Batch size stays at 15000 (60000 params, under 65535 cap) to keep the kind-39002 SERIALIZABLE critical-section economy intact. - buildSelectQuery pushes `kind IN (...) OR kind IS NULL` into each per-tag CTE branch when filter.Kinds is set. The IS NULL branch keeps reads correct on un-backfilled rows during the production rollout window. Drop it in a follow-up once backfill is verified. - Init splits the schema-bootstrap into pre-migrate and post-migrate phases. The covering index `(key, value, kind, event_id)` lives in post-migrate so it runs only after migration 002 has added the column on existing schemas — avoiding the "column does not exist" failure on the production upgrade path. Migration scope is intentionally minimal (ADD COLUMN only). The backfill UPDATE and CREATE INDEX CONCURRENTLY are documented as manual one-shot ops because the auto-migration runner has a 30s per-statement deadline and CONCURRENTLY can't survive that without leaving an INVALID index that IF NOT EXISTS would silently skip. Tests: - TestEventStore_SaveEvent_PopulatesTagKind — verifies kind populates on new inserts. - TestEventStore_QueryEvents_TagAndKindCTE — saves 3 chat + 5 membership events tagged with the same group; asserts the kind filter excludes membership at the CTE. - TestEventStore_Init_UpgradeFromPre002 — drops the kind column and the migration marker on a freshly-initialized schema, then re-runs Init() and asserts the column and post-migrate index both come back. This is the test that catches the pre-migrate/post-migrate ordering bug — fresh-schema tests can't, because CREATE TABLE on a fresh schema makes the column from the start. - TestEventStore_QueryEvents_TagKindNullCompat — clears kind to NULL on a saved event's tag rows and asserts the OR IS NULL branch keeps it visible. Fixes #23. --- zooid/events.go | 71 ++++++- zooid/events_test.go | 243 +++++++++++++++++++++++ zooid/migrations/002_event_tags_kind.sql | 23 +++ 3 files changed, 326 insertions(+), 11 deletions(-) create mode 100644 zooid/migrations/002_event_tags_kind.sql diff --git a/zooid/events.go b/zooid/events.go index 86ce46a..da7f13d 100644 --- a/zooid/events.go +++ b/zooid/events.go @@ -86,7 +86,13 @@ type EventStore struct { var _ eventstore.Store = (*EventStore)(nil) func (events *EventStore) Init() error { - statements := []string{ + // Statements that run BEFORE migrations: create base tables and any + // indexes whose definitions reference only base-table columns. The + // `event_tags.kind` column was introduced by migration 002, so the + // covering index that uses it is created later — see postMigrate + // below — to avoid referencing a column that doesn't exist yet on + // pre-002 schemas. + preMigrate := []string{ events.Schema.Render(` CREATE TABLE IF NOT EXISTS {{.Name}}__events ( id TEXT PRIMARY KEY, @@ -107,6 +113,7 @@ func (events *EventStore) Init() error { event_id TEXT NOT NULL, key TEXT NOT NULL, value TEXT NOT NULL, + kind INTEGER, FOREIGN KEY (event_id) REFERENCES {{.Name}}__events(id) ON DELETE CASCADE )`), events.Schema.Render(`CREATE INDEX IF NOT EXISTS {{.Name}}__idx_event_tags_event_id ON {{.Name}}__event_tags(event_id)`), @@ -116,7 +123,7 @@ func (events *EventStore) Init() error { events.Schema.Render(`CREATE INDEX IF NOT EXISTS {{.Name}}__idx_events_kind_created_at ON {{.Name}}__events(kind, created_at DESC)`), } - for _, stmt := range statements { + for _, stmt := range preMigrate { if _, err := GetDb().ExecContext(events.rootCtx, stmt); err != nil { return fmt.Errorf("schema init failed: %w", err) } @@ -130,6 +137,17 @@ func (events *EventStore) Init() error { return fmt.Errorf("migrations failed: %w", err) } + // Statements that depend on columns added by migrations. + postMigrate := []string{ + events.Schema.Render(`CREATE INDEX IF NOT EXISTS {{.Name}}__idx_event_tags_key_value_kind_event_id ON {{.Name}}__event_tags(key, value, kind, event_id)`), + } + + for _, stmt := range postMigrate { + if _, err := GetDb().ExecContext(events.rootCtx, stmt); err != nil { + return fmt.Errorf("schema init (post-migrate) failed: %w", err) + } + } + return nil } @@ -322,6 +340,20 @@ func (events *EventStore) buildSelectQuery(filter nostr.Filter) squirrel.SelectB if len(tagFilters) > 0 { col = "e." + // Pre-compute the kind values once so each tag CTE branch can + // push them down via the (key, value, kind, event_id) covering + // index. Critical for hot groups whose tag rows are dominated by + // membership events (kinds 9000/9021): without this, a query for + // chat kinds (9, 11, 12) on h='general' hash-joins ~97k tag rows + // just to throw away the ~95k membership ones (issue #23). + var kindInts []interface{} + if len(filter.Kinds) > 0 { + kindInts = make([]interface{}, len(filter.Kinds)) + for i, k := range filter.Kinds { + kindInts[i] = int(k) + } + } + // Build one SELECT per tag filter, INTERSECT them for AND logic. var cteParts []string var cteArgs []interface{} @@ -330,6 +362,16 @@ func (events *EventStore) buildSelectQuery(filter nostr.Filter) squirrel.SelectB From(eventTagsTable). Where(squirrel.Eq{"key": tf.key}). Where(squirrel.Eq{"value": tf.values}) + if len(kindInts) > 0 { + // `kind IS NULL` keeps reads correct for un-backfilled + // rows (event_tags.kind is added nullable; backfill is a + // separate ops step). Drop the IS NULL branch in a + // follow-up once the backfill is verified complete. + subQ = subQ.Where(squirrel.Or{ + squirrel.Eq{"kind": kindInts}, + squirrel.Expr("kind IS NULL"), + }) + } sql, args, _ := subQ.ToSql() cteParts = append(cteParts, sql) cteArgs = append(cteArgs, args...) @@ -496,30 +538,37 @@ func (events *EventStore) saveEventWith(ctx context.Context, runner squirrel.Bas } // Insert single-letter tags into event_tags, chunked to stay below - // Postgres's 65535 extended-protocol parameter limit. With 3 columns per - // row, 15000 rows × 3 = 45000 params is well under the 65535 cap and - // cuts the inner round-trip count by ~3× vs. 5k batches — important for - // kind-39002 (NIP-29 member list) saves where the whole transaction runs - // under SERIALIZABLE isolation and contention is dominated by wall-clock - // duration of the critical section (issues #13, #16). + // Postgres's 65535 extended-protocol parameter limit. With 4 columns + // per row (event_id, key, value, kind), 15000 rows × 4 = 60000 params + // stays under the 65535 cap and matches the round-trip economy that + // matters most for kind-39002 (NIP-29 member list) saves — those run + // under SERIALIZABLE isolation and contention is dominated by the + // wall-clock duration of the critical section (issues #13, #16). If + // another column is ever added here the batch size must drop. const tagInsertBatchSize = 15000 eventID := evt.ID.Hex() + eventKind := int(evt.Kind) tagsTable := events.Schema.Prefix("event_tags") - batch := sb.Insert(tagsTable).Columns("event_id", "key", "value") + // kind is denormalized here so the tag-filter CTE in buildSelectQuery + // can pre-filter by kind via the (key, value, kind, event_id) covering + // index — without it, hot groups whose tag-rows are dominated by + // membership events (kinds 9000/9021) hash-join 90k+ rows just to throw + // 95% of them away on the kind filter. See zooid issue #23. + batch := sb.Insert(tagsTable).Columns("event_id", "key", "value", "kind") n := 0 for _, tag := range evt.Tags { if len(tag) < 2 || len(tag[0]) != 1 { continue } - batch = batch.Values(eventID, tag[0], tag[1]) + batch = batch.Values(eventID, tag[0], tag[1], eventKind) n++ if n >= tagInsertBatchSize { if _, err := batch.RunWith(runner).ExecContext(ctx); err != nil { return fmt.Errorf("failed to save tags for event '%s': %w", evt.ID, err) } - batch = sb.Insert(tagsTable).Columns("event_id", "key", "value") + batch = sb.Insert(tagsTable).Columns("event_id", "key", "value", "kind") n = 0 } } diff --git a/zooid/events_test.go b/zooid/events_test.go index 8718a07..bd4e1f2 100644 --- a/zooid/events_test.go +++ b/zooid/events_test.go @@ -1031,3 +1031,246 @@ func TestEventStore_GetOrCreateApplicationSpecificData(t *testing.T) { t.Error("GetOrCreateApplicationSpecificData() should create different event for different d tag") } } + +// TestEventStore_SaveEvent_PopulatesTagKind verifies that the kind +// denormalization on event_tags actually populates on insert. Issue #23. +func TestEventStore_SaveEvent_PopulatesTagKind(t *testing.T) { + store := createTestEventStore() + if err := store.Init(); err != nil { + t.Fatalf("Init: %v", err) + } + + evt := nostr.Event{ + Kind: 9000, // a NIP-29 membership kind — the workload that motivated #23 + CreatedAt: nostr.Now(), + Content: "kind populates on insert", + Tags: nostr.Tags{{"h", "general"}, {"p", "deadbeef"}}, + } + evt.Sign(nostr.Generate()) + if err := store.SaveEvent(evt); err != nil { + t.Fatalf("SaveEvent: %v", err) + } + + tagsTable := store.Schema.Prefix("event_tags") + rows, err := GetDb().QueryContext(store.rootCtx, + "SELECT key, value, kind FROM "+tagsTable+" WHERE event_id = $1 ORDER BY key", + evt.ID.Hex()) + if err != nil { + t.Fatalf("query: %v", err) + } + defer rows.Close() + + var got []struct { + key, value string + kind sql.NullInt32 + } + for rows.Next() { + var r struct { + key, value string + kind sql.NullInt32 + } + if err := rows.Scan(&r.key, &r.value, &r.kind); err != nil { + t.Fatalf("scan: %v", err) + } + got = append(got, r) + } + if len(got) != 2 { + t.Fatalf("expected 2 tag rows, got %d", len(got)) + } + for _, r := range got { + if !r.kind.Valid { + t.Errorf("tag (%s,%s): kind is NULL, want %d", r.key, r.value, evt.Kind) + } else if int(r.kind.Int32) != int(evt.Kind) { + t.Errorf("tag (%s,%s): kind=%d, want %d", r.key, r.value, r.kind.Int32, evt.Kind) + } + } +} + +// TestEventStore_QueryEvents_TagAndKindCTE verifies the read path +// correctly filters by tag AND kind via the materialized CTE. The +// motivating bug: hot groups with many membership events on the same +// h-tag dominate the CTE result, then get filtered out at hash-join +// time. With kind pushed into the CTE, only matching-kind tag rows +// participate. Issue #23. +func TestEventStore_QueryEvents_TagAndKindCTE(t *testing.T) { + store := createTestEventStore() + if err := store.Init(); err != nil { + t.Fatalf("Init: %v", err) + } + + const groupID = "general" + mkEvent := func(kind nostr.Kind, content string) nostr.Event { + evt := nostr.Event{ + Kind: kind, + CreatedAt: nostr.Now(), + Content: content, + Tags: nostr.Tags{{"h", groupID}}, + } + evt.Sign(nostr.Generate()) + return evt + } + + // 3 chat messages and 5 membership events, all tagged h=general. + chat := []nostr.Event{ + mkEvent(9, "msg-1"), + mkEvent(9, "msg-2"), + mkEvent(9, "msg-3"), + } + membership := []nostr.Event{ + mkEvent(9000, "add-1"), + mkEvent(9000, "add-2"), + mkEvent(9021, "join-1"), + mkEvent(9021, "join-2"), + mkEvent(9021, "join-3"), + } + for _, evt := range append(append([]nostr.Event{}, chat...), membership...) { + if err := store.SaveEvent(evt); err != nil { + t.Fatalf("SaveEvent: %v", err) + } + } + + // Filter for chat kinds only on the same h-tag — should return only + // the 3 chat events, none of the 5 membership events. + filter := nostr.Filter{ + Kinds: []nostr.Kind{9, 11, 12}, + Tags: nostr.TagMap{"h": []string{groupID}}, + } + var got []nostr.Event + for evt := range store.QueryEvents(filter, 100) { + got = append(got, evt) + } + + if len(got) != len(chat) { + t.Fatalf("got %d events, want %d (chat-only)", len(got), len(chat)) + } + for _, evt := range got { + if evt.Kind != 9 { + t.Errorf("returned event kind=%d, want 9; the kind filter leaked a membership event", evt.Kind) + } + } +} + +// TestEventStore_Init_UpgradeFromPre002 simulates the production +// upgrade path: a schema that was already initialized before +// migration 002 existed. The `event_tags.kind` column is missing, the +// kvstore has no row for `migration:.../002_event_tags_kind.sql`, and +// the new code path's startup index references the column. +// +// Init() must: +// 1. Defer index creation until AFTER migrations run, so the +// post-migrate `(key, value, kind, event_id)` index doesn't try to +// reference a column that doesn't exist yet. +// 2. Apply migration 002 to add the column. +// 3. Then create the new covering index successfully. +// +// This is the test that would have caught the original ordering bug +// where the index was created in the same pre-migration block as the +// base table. +func TestEventStore_Init_UpgradeFromPre002(t *testing.T) { + store := createTestEventStore() + + // Initial Init: gives us a fully-up-to-date schema. + if err := store.Init(); err != nil { + t.Fatalf("first Init: %v", err) + } + + // Now reverse-engineer the pre-002 production state: + // - drop the kind column + // - drop the new covering index + // - delete the kv row marking 002 as applied so migrations re-run + tagsTable := store.Schema.Prefix("event_tags") + indexName := store.Schema.Name + "__idx_event_tags_key_value_kind_event_id" + + if _, err := GetDb().ExecContext(store.rootCtx, "DROP INDEX IF EXISTS "+indexName); err != nil { + t.Fatalf("drop index: %v", err) + } + if _, err := GetDb().ExecContext(store.rootCtx, "ALTER TABLE "+tagsTable+" DROP COLUMN IF EXISTS kind"); err != nil { + t.Fatalf("drop column: %v", err) + } + kvKey := fmt.Sprintf("migration:%s:002_event_tags_kind.sql", store.Schema.Name) + if _, err := GetDb().ExecContext(store.rootCtx, "DELETE FROM kv WHERE key = $1", kvKey); err != nil { + t.Fatalf("delete migration marker: %v", err) + } + + // Sanity-check: column really is gone. + var hasKind bool + if err := GetDb().QueryRowContext(store.rootCtx, + "SELECT EXISTS (SELECT 1 FROM information_schema.columns WHERE LOWER(table_name)=LOWER($1) AND column_name='kind')", + tagsTable).Scan(&hasKind); err != nil { + t.Fatalf("information_schema check: %v", err) + } + if hasKind { + t.Fatalf("expected kind column to be dropped before upgrade test") + } + + // Re-init: must succeed end-to-end. This is the path that errored + // before the fix because the new covering index was created in the + // pre-migration block. + if err := store.Init(); err != nil { + t.Fatalf("upgrade Init: %v", err) + } + + // Column and index must exist after the upgrade. + if err := GetDb().QueryRowContext(store.rootCtx, + "SELECT EXISTS (SELECT 1 FROM information_schema.columns WHERE LOWER(table_name)=LOWER($1) AND column_name='kind')", + tagsTable).Scan(&hasKind); err != nil { + t.Fatalf("information_schema re-check: %v", err) + } + if !hasKind { + t.Errorf("kind column not added by migration 002") + } + + var hasIndex bool + if err := GetDb().QueryRowContext(store.rootCtx, + "SELECT EXISTS (SELECT 1 FROM pg_indexes WHERE LOWER(indexname)=LOWER($1))", indexName).Scan(&hasIndex); err != nil { + t.Fatalf("pg_indexes re-check: %v", err) + } + if !hasIndex { + t.Errorf("post-migrate index %s not created", indexName) + } +} + +// TestEventStore_QueryEvents_TagKindNullCompat ensures the read path +// still returns historical event_tags rows whose `kind` column is NULL +// (the state during the backfill window after migration 002 lands but +// before the dbops UPDATE has run). The CTE uses +// `kind IN (...) OR kind IS NULL` to keep these rows visible. Issue #23. +func TestEventStore_QueryEvents_TagKindNullCompat(t *testing.T) { + store := createTestEventStore() + if err := store.Init(); err != nil { + t.Fatalf("Init: %v", err) + } + + evt := nostr.Event{ + Kind: 9, + CreatedAt: nostr.Now(), + Content: "pretend-this-is-old", + Tags: nostr.Tags{{"h", "general"}}, + } + evt.Sign(nostr.Generate()) + if err := store.SaveEvent(evt); err != nil { + t.Fatalf("SaveEvent: %v", err) + } + + // Simulate an un-backfilled row: clear the kind column on the tag rows. + tagsTable := store.Schema.Prefix("event_tags") + if _, err := GetDb().ExecContext(store.rootCtx, + "UPDATE "+tagsTable+" SET kind = NULL WHERE event_id = $1", evt.ID.Hex()); err != nil { + t.Fatalf("UPDATE: %v", err) + } + + filter := nostr.Filter{ + Kinds: []nostr.Kind{9, 11, 12}, + Tags: nostr.TagMap{"h": []string{"general"}}, + } + var got []nostr.Event + for e := range store.QueryEvents(filter, 100) { + got = append(got, e) + } + if len(got) != 1 { + t.Fatalf("got %d events with NULL-kind tag rows, want 1 (OR IS NULL fallback broken)", len(got)) + } + if got[0].ID != evt.ID { + t.Errorf("returned wrong event") + } +} diff --git a/zooid/migrations/002_event_tags_kind.sql b/zooid/migrations/002_event_tags_kind.sql new file mode 100644 index 0000000..98c5ad3 --- /dev/null +++ b/zooid/migrations/002_event_tags_kind.sql @@ -0,0 +1,23 @@ +-- Denormalize the event's `kind` onto event_tags so the tag-filter CTE in +-- buildSelectQuery can pre-filter by kind via a covering index. Without +-- this, hot groups whose tag rows are dominated by membership events +-- (NIP-29 kinds 9000/9021) hash-join ~97k tag rows for `h=''` +-- just to throw away ~95% on the kind filter — see issue #23. +-- +-- Migration scope: +-- 1. ADD COLUMN nullable. Instant on PG 11+ (metadata-only). +-- +-- The matching covering index `(key, value, kind, event_id)` and the +-- backfill of historical rows are NOT applied here: +-- - The auto-migration runner enforces a 30s per-statement deadline, +-- and on a busy production schema both CREATE INDEX (concurrent or +-- not) and the bulk UPDATE can exceed that. +-- - CREATE INDEX CONCURRENTLY cannot run inside a transaction and +-- leaves an INVALID index on timeout, which IF NOT EXISTS would +-- then silently skip on retry. +-- +-- Run those two as one-shot ops via the dbops task before deploying the +-- code change that uses the new shape — see PR description for the +-- exact commands. Until backfill completes, the read path emits +-- `kind IN (...) OR kind IS NULL` so historical rows still match. +ALTER TABLE {{.Name}}__event_tags ADD COLUMN IF NOT EXISTS kind INTEGER; From 13511a8b7f2951c9b1e289fa790db6a8f3405777 Mon Sep 17 00:00:00 2001 From: Pavel Grigorenko Date: Sat, 9 May 2026 13:32:36 +0200 Subject: [PATCH 2/3] fix: address PR #24 reviews MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - gemini: hoist `kindInts` conversion out of the CTE block to function scope and reuse it in the outer events filter — was duplicated. - gemini: error-check `subQ.ToSql()` instead of dropping the error. Build-state failures here are programmer errors (malformed squirrel.Select), so panic loudly rather than silently appending an empty CTE part. - Copilot: strengthen TestEventStore_QueryEvents_TagAndKindCTE to assert on the generated SQL shape — the result-level assertion alone would still pass with the kind pushdown removed (the outer events.kind filter does the real work). Now we slice out the CTE text and require it to contain a `kind` predicate, so the optimization itself is guarded against regression. - Copilot: update migration 002 comment to reflect what actually happens — the post-migrate `CREATE INDEX IF NOT EXISTS` in Init() is a no-op on production after the documented dbops one-shot CONCURRENTLY create has run, fast on fresh schemas. The migration file no longer claims the index is "intentionally not applied" by any startup path. Not taken: gemini's suggestion to drop the now-prefix-redundant `(key, value, event_id)` index. The new `(key, value, kind, event_id)` covers it via prefix matching, so dropping is safe in principle, but removing a hot-path index in the same change as adding the new one mixes two reversibility profiles. Will do as a follow-up after the new index is verified in production. --- zooid/events.go | 42 +++++++++++++----------- zooid/events_test.go | 34 +++++++++++++++++-- zooid/migrations/002_event_tags_kind.sql | 29 ++++++++-------- 3 files changed, 67 insertions(+), 38 deletions(-) diff --git a/zooid/events.go b/zooid/events.go index da7f13d..83bb041 100644 --- a/zooid/events.go +++ b/zooid/events.go @@ -322,6 +322,18 @@ func (events *EventStore) buildSelectQuery(filter nostr.Filter) squirrel.SelectB return tagFilters[i].key < tagFilters[j].key }) + // Pre-compute filter.Kinds once. Used both inside the tag CTE (to + // push kind into the (key, value, kind, event_id) covering index — + // critical for hot groups whose tag rows are dominated by membership + // events, see issue #23) and on the outer events query. + var kindInts []interface{} + if len(filter.Kinds) > 0 { + kindInts = make([]interface{}, len(filter.Kinds)) + for i, k := range filter.Kinds { + kindInts[i] = int(k) + } + } + // When tag filters are present, use a materialized CTE to force the // planner to resolve tag lookups FIRST via the covering index, then // join the small result set to events. @@ -340,20 +352,6 @@ func (events *EventStore) buildSelectQuery(filter nostr.Filter) squirrel.SelectB if len(tagFilters) > 0 { col = "e." - // Pre-compute the kind values once so each tag CTE branch can - // push them down via the (key, value, kind, event_id) covering - // index. Critical for hot groups whose tag rows are dominated by - // membership events (kinds 9000/9021): without this, a query for - // chat kinds (9, 11, 12) on h='general' hash-joins ~97k tag rows - // just to throw away the ~95k membership ones (issue #23). - var kindInts []interface{} - if len(filter.Kinds) > 0 { - kindInts = make([]interface{}, len(filter.Kinds)) - for i, k := range filter.Kinds { - kindInts[i] = int(k) - } - } - // Build one SELECT per tag filter, INTERSECT them for AND logic. var cteParts []string var cteArgs []interface{} @@ -372,7 +370,15 @@ func (events *EventStore) buildSelectQuery(filter nostr.Filter) squirrel.SelectB squirrel.Expr("kind IS NULL"), }) } - sql, args, _ := subQ.ToSql() + sql, args, err := subQ.ToSql() + if err != nil { + // squirrel.Select.ToSql only fails for malformed builder + // state (unset column lists, etc.) — programmer error, + // not user input. Panic so it's caught loudly in tests + // rather than silently appending an empty string and + // corrupting the CTE. + panic(fmt.Errorf("buildSelectQuery: tag CTE ToSql: %w", err)) + } cteParts = append(cteParts, sql) cteArgs = append(cteArgs, args...) } @@ -412,11 +418,7 @@ func (events *EventStore) buildSelectQuery(filter nostr.Filter) squirrel.SelectB qb = qb.Where(squirrel.Eq{col + "pubkey": authorStrs}) } - if len(filter.Kinds) > 0 { - kindInts := make([]interface{}, len(filter.Kinds)) - for i, kind := range filter.Kinds { - kindInts[i] = int(kind) - } + if len(kindInts) > 0 { qb = qb.Where(squirrel.Eq{col + "kind": kindInts}) } diff --git a/zooid/events_test.go b/zooid/events_test.go index bd4e1f2..e5fe528 100644 --- a/zooid/events_test.go +++ b/zooid/events_test.go @@ -4,6 +4,7 @@ import ( "context" "database/sql" "fmt" + "strings" "testing" "time" @@ -1092,6 +1093,16 @@ func TestEventStore_SaveEvent_PopulatesTagKind(t *testing.T) { // h-tag dominate the CTE result, then get filtered out at hash-join // time. With kind pushed into the CTE, only matching-kind tag rows // participate. Issue #23. +// +// Two-part assertion: +// +// - Result-level: chat-kind filter on a tag shared with membership +// events excludes the membership events from output. +// - SQL-shape: the CTE itself contains a `kind` predicate. Without +// this check the test would still pass even if the kind pushdown +// into the CTE were removed, because the outer events.kind filter +// alone produces correct results — it just defeats the whole +// point of the optimization. func TestEventStore_QueryEvents_TagAndKindCTE(t *testing.T) { store := createTestEventStore() if err := store.Init(); err != nil { @@ -1129,17 +1140,34 @@ func TestEventStore_QueryEvents_TagAndKindCTE(t *testing.T) { } } - // Filter for chat kinds only on the same h-tag — should return only - // the 3 chat events, none of the 5 membership events. filter := nostr.Filter{ Kinds: []nostr.Kind{9, 11, 12}, Tags: nostr.TagMap{"h": []string{groupID}}, } + + // SQL-shape guard: the CTE must contain the kind predicate. If a + // future refactor accidentally removes the kind pushdown, the + // result-level assertion below would still pass — this one + // catches the regression directly. + sqlText, _, err := store.buildSelectQuery(filter).ToSql() + if err != nil { + t.Fatalf("buildSelectQuery.ToSql: %v", err) + } + cteStart := strings.Index(sqlText, "WITH _tag_ids AS MATERIALIZED (") + cteEnd := strings.Index(sqlText, ") SELECT ") + if cteStart == -1 || cteEnd == -1 || cteEnd <= cteStart { + t.Fatalf("could not isolate CTE in generated SQL: %s", sqlText) + } + cteText := sqlText[cteStart:cteEnd] + if !strings.Contains(cteText, "kind") { + t.Errorf("CTE missing kind predicate — kind pushdown regression. CTE=%q", cteText) + } + + // Result-level: chat-only filter excludes membership events. var got []nostr.Event for evt := range store.QueryEvents(filter, 100) { got = append(got, evt) } - if len(got) != len(chat) { t.Fatalf("got %d events, want %d (chat-only)", len(got), len(chat)) } diff --git a/zooid/migrations/002_event_tags_kind.sql b/zooid/migrations/002_event_tags_kind.sql index 98c5ad3..4b409cd 100644 --- a/zooid/migrations/002_event_tags_kind.sql +++ b/zooid/migrations/002_event_tags_kind.sql @@ -4,20 +4,19 @@ -- (NIP-29 kinds 9000/9021) hash-join ~97k tag rows for `h=''` -- just to throw away ~95% on the kind filter — see issue #23. -- --- Migration scope: --- 1. ADD COLUMN nullable. Instant on PG 11+ (metadata-only). +-- Migration scope is just ADD COLUMN (instant, metadata-only on PG 11+). +-- The matching covering index `(key, value, kind, event_id)` is created +-- by Init() in the post-migrate phase via CREATE INDEX IF NOT EXISTS, +-- which is fast on fresh/dev schemas (empty or tiny event_tags) but on +-- a populated production schema would hold a SHARE lock on event_tags +-- for the duration of the build. The backfill of historical rows is +-- not in this file at all. -- --- The matching covering index `(key, value, kind, event_id)` and the --- backfill of historical rows are NOT applied here: --- - The auto-migration runner enforces a 30s per-statement deadline, --- and on a busy production schema both CREATE INDEX (concurrent or --- not) and the bulk UPDATE can exceed that. --- - CREATE INDEX CONCURRENTLY cannot run inside a transaction and --- leaves an INVALID index on timeout, which IF NOT EXISTS would --- then silently skip on retry. --- --- Run those two as one-shot ops via the dbops task before deploying the --- code change that uses the new shape — see PR description for the --- exact commands. Until backfill completes, the read path emits --- `kind IN (...) OR kind IS NULL` so historical rows still match. +-- Production rollout therefore expects two one-shot ops via the dbops +-- task BEFORE this code is deployed: a backfill UPDATE setting kind on +-- existing rows, and a CREATE INDEX CONCURRENTLY IF NOT EXISTS for the +-- new covering index. Exact commands are in the PR description for +-- issue #23. Until the backfill is verified complete, the read path +-- emits `kind IN (...) OR kind IS NULL` so un-backfilled rows still +-- match. ALTER TABLE {{.Name}}__event_tags ADD COLUMN IF NOT EXISTS kind INTEGER; From 88fbc1b5ecea870293c69db21876b7bfd7f20cef Mon Sep 17 00:00:00 2001 From: Pavel Grigorenko Date: Sat, 9 May 2026 13:59:22 +0200 Subject: [PATCH 3/3] fix: address PR #24 follow-up review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Copilot: stop running CREATE INDEX in Init() — move it into migration 002. Migration runs once per schema, with the runner's 30s per-statement deadline acting as a fail-fast on any production schema where the index wasn't pre-built via the dbops CONCURRENTLY step. With the prep done, IF NOT EXISTS no-ops the migration's CREATE INDEX. Without prep, the migration fails loudly and the relay refuses to start with an actionable error. The pre-/post-migrate split in Init() goes away with this — there's nothing post-migrate left to run. - Copilot: stop panicking on `subQ.ToSql()` failure in `buildSelectQuery`. The function now returns `(squirrel.SelectBuilder, error)`. Both callers (`queryEventsWith`, `CountEvents`) handle the error and return / log instead of crashing the goroutine. - Copilot: replace the brittle exact-substring slicing in TestEventStore_QueryEvents_TagAndKindCTE with a regex `(?s)MATERIALIZED.*?\\bkind\\b.*?\\)\\s+SELECT\\b`. Both lazy quantifiers anchor to the nearest CTE-close-and-SELECT, so an outer-only `e.kind` filter doesn't satisfy it (there is no `) SELECT` after the outer query's kind). Tolerant of squirrel whitespace/formatting drift. --- zooid/events.go | 56 +++++++++++------------- zooid/events_test.go | 32 ++++++++------ zooid/migrations/002_event_tags_kind.sql | 28 +++++++----- 3 files changed, 61 insertions(+), 55 deletions(-) diff --git a/zooid/events.go b/zooid/events.go index 83bb041..7cf5199 100644 --- a/zooid/events.go +++ b/zooid/events.go @@ -86,13 +86,12 @@ type EventStore struct { var _ eventstore.Store = (*EventStore)(nil) func (events *EventStore) Init() error { - // Statements that run BEFORE migrations: create base tables and any - // indexes whose definitions reference only base-table columns. The - // `event_tags.kind` column was introduced by migration 002, so the - // covering index that uses it is created later — see postMigrate - // below — to avoid referencing a column that doesn't exist yet on - // pre-002 schemas. - preMigrate := []string{ + // Base tables and the indexes whose definitions reference only + // columns present in those CREATE TABLE statements. Indexes that + // depend on columns added by later migrations live in those + // migration files, not here, so we don't reference a column that + // doesn't exist yet on a pre-migration schema. + statements := []string{ events.Schema.Render(` CREATE TABLE IF NOT EXISTS {{.Name}}__events ( id TEXT PRIMARY KEY, @@ -123,7 +122,7 @@ func (events *EventStore) Init() error { events.Schema.Render(`CREATE INDEX IF NOT EXISTS {{.Name}}__idx_events_kind_created_at ON {{.Name}}__events(kind, created_at DESC)`), } - for _, stmt := range preMigrate { + for _, stmt := range statements { if _, err := GetDb().ExecContext(events.rootCtx, stmt); err != nil { return fmt.Errorf("schema init failed: %w", err) } @@ -137,17 +136,6 @@ func (events *EventStore) Init() error { return fmt.Errorf("migrations failed: %w", err) } - // Statements that depend on columns added by migrations. - postMigrate := []string{ - events.Schema.Render(`CREATE INDEX IF NOT EXISTS {{.Name}}__idx_event_tags_key_value_kind_event_id ON {{.Name}}__event_tags(key, value, kind, event_id)`), - } - - for _, stmt := range postMigrate { - if _, err := GetDb().ExecContext(events.rootCtx, stmt); err != nil { - return fmt.Errorf("schema init (post-migrate) failed: %w", err) - } - } - return nil } @@ -220,7 +208,13 @@ func (events *EventStore) queryEventsWith(ctx context.Context, runner squirrel.B queryStart := time.Now() var drainTotal time.Duration - rows, err := events.buildSelectQuery(filter).RunWith(runner).QueryContext(ctx) + qb, err := events.buildSelectQuery(filter) + if err != nil { + observeQueryTimings(totalObserver, dbObserver, drainObserver, queryStart, drainTotal) + log.Printf("QueryEvents buildSelectQuery error: %v", err) + return + } + rows, err := qb.RunWith(runner).QueryContext(ctx) if err != nil { observeQueryTimings(totalObserver, dbObserver, drainObserver, queryStart, drainTotal) log.Printf("QueryEvents query error: %v", err) @@ -298,7 +292,7 @@ func observeQueryTimings(total, db, drain prometheus.Observer, queryStart time.T drain.Observe(drainTotal.Seconds()) } -func (events *EventStore) buildSelectQuery(filter nostr.Filter) squirrel.SelectBuilder { +func (events *EventStore) buildSelectQuery(filter nostr.Filter) (squirrel.SelectBuilder, error) { eventsTable := events.Schema.Prefix("events") eventTagsTable := events.Schema.Prefix("event_tags") @@ -373,11 +367,10 @@ func (events *EventStore) buildSelectQuery(filter nostr.Filter) squirrel.SelectB sql, args, err := subQ.ToSql() if err != nil { // squirrel.Select.ToSql only fails for malformed builder - // state (unset column lists, etc.) — programmer error, - // not user input. Panic so it's caught loudly in tests - // rather than silently appending an empty string and - // corrupting the CTE. - panic(fmt.Errorf("buildSelectQuery: tag CTE ToSql: %w", err)) + // state, not user input. Propagate so callers in the + // query and count paths can log and short-circuit + // instead of crashing the process. + return squirrel.SelectBuilder{}, fmt.Errorf("buildSelectQuery: tag CTE ToSql: %w", err) } cteParts = append(cteParts, sql) cteArgs = append(cteArgs, args...) @@ -434,7 +427,7 @@ func (events *EventStore) buildSelectQuery(filter nostr.Filter) squirrel.SelectB qb = qb.Limit(uint64(filter.Limit)) } - return qb + return qb, nil } // buildTagFilteredQuery constructs a raw SQL query using a materialized CTE @@ -699,7 +692,11 @@ func (events *EventStore) CountEvents(filter nostr.Filter) (uint32, error) { // Strip limit for a true total count; ORDER BY in the subquery is // optimized away by PostgreSQL's planner inside COUNT(*). filter.Limit = 0 - qb := events.buildSelectQuery(filter).RemoveLimit() + qb, err := events.buildSelectQuery(filter) + if err != nil { + return 0, fmt.Errorf("failed to build count query: %w", err) + } + qb = qb.RemoveLimit() countQb := sb.Select("COUNT(*)").FromSelect(qb, "subquery") @@ -707,8 +704,7 @@ func (events *EventStore) CountEvents(filter nostr.Filter) (uint32, error) { defer cancel() var count uint32 - err := countQb.RunWith(GetDb()).QueryRowContext(ctx).Scan(&count) - if err != nil { + if err := countQb.RunWith(GetDb()).QueryRowContext(ctx).Scan(&count); err != nil { return 0, fmt.Errorf("failed to count events: %w", err) } diff --git a/zooid/events_test.go b/zooid/events_test.go index e5fe528..a47b439 100644 --- a/zooid/events_test.go +++ b/zooid/events_test.go @@ -4,7 +4,7 @@ import ( "context" "database/sql" "fmt" - "strings" + "regexp" "testing" "time" @@ -1145,22 +1145,28 @@ func TestEventStore_QueryEvents_TagAndKindCTE(t *testing.T) { Tags: nostr.TagMap{"h": []string{groupID}}, } - // SQL-shape guard: the CTE must contain the kind predicate. If a - // future refactor accidentally removes the kind pushdown, the - // result-level assertion below would still pass — this one + // SQL-shape guard: the materialized CTE must contain the kind + // predicate. If a future refactor accidentally removes the kind + // pushdown, the result-level assertion below would still pass + // (the outer events.kind filter does the visible work) — this // catches the regression directly. - sqlText, _, err := store.buildSelectQuery(filter).ToSql() + // + // The regex matches MATERIALIZED, then any (lazy) content + // containing `kind`, then any (lazy) content up to the CTE + // close `)SELECT`. Both lazy quantifiers anchor to + // the closest CTE end, so an outer-only `e.kind` doesn't satisfy + // it (there is no `) SELECT` after the outer query's kind). + qb, err := store.buildSelectQuery(filter) if err != nil { - t.Fatalf("buildSelectQuery.ToSql: %v", err) + t.Fatalf("buildSelectQuery: %v", err) } - cteStart := strings.Index(sqlText, "WITH _tag_ids AS MATERIALIZED (") - cteEnd := strings.Index(sqlText, ") SELECT ") - if cteStart == -1 || cteEnd == -1 || cteEnd <= cteStart { - t.Fatalf("could not isolate CTE in generated SQL: %s", sqlText) + sqlText, _, err := qb.ToSql() + if err != nil { + t.Fatalf("ToSql: %v", err) } - cteText := sqlText[cteStart:cteEnd] - if !strings.Contains(cteText, "kind") { - t.Errorf("CTE missing kind predicate — kind pushdown regression. CTE=%q", cteText) + cteWithKind := regexp.MustCompile(`(?s)MATERIALIZED.*?\bkind\b.*?\)\s+SELECT\b`) + if !cteWithKind.MatchString(sqlText) { + t.Errorf("CTE missing kind predicate — kind pushdown regression. SQL=%q", sqlText) } // Result-level: chat-only filter excludes membership events. diff --git a/zooid/migrations/002_event_tags_kind.sql b/zooid/migrations/002_event_tags_kind.sql index 4b409cd..d65ea86 100644 --- a/zooid/migrations/002_event_tags_kind.sql +++ b/zooid/migrations/002_event_tags_kind.sql @@ -4,19 +4,23 @@ -- (NIP-29 kinds 9000/9021) hash-join ~97k tag rows for `h=''` -- just to throw away ~95% on the kind filter — see issue #23. -- --- Migration scope is just ADD COLUMN (instant, metadata-only on PG 11+). --- The matching covering index `(key, value, kind, event_id)` is created --- by Init() in the post-migrate phase via CREATE INDEX IF NOT EXISTS, --- which is fast on fresh/dev schemas (empty or tiny event_tags) but on --- a populated production schema would hold a SHARE lock on event_tags --- for the duration of the build. The backfill of historical rows is --- not in this file at all. +-- Two statements: +-- 1. ADD COLUMN nullable. Instant on PG 11+ (metadata-only). +-- 2. CREATE INDEX IF NOT EXISTS for the matching covering index. +-- Idempotent — runs on every fresh schema, no-ops elsewhere. -- --- Production rollout therefore expects two one-shot ops via the dbops --- task BEFORE this code is deployed: a backfill UPDATE setting kind on --- existing rows, and a CREATE INDEX CONCURRENTLY IF NOT EXISTS for the --- new covering index. Exact commands are in the PR description for --- issue #23. Until the backfill is verified complete, the read path +-- The runner enforces a 30s per-statement deadline. On an empty or +-- small event_tags this is comfortable. On a busy production schema +-- with hundreds of thousands of rows, the inline CREATE INDEX would +-- exceed that budget and migration apply would fail — by design. +-- That fail-fast is the contract: production deploys MUST first run +-- a one-shot CREATE INDEX CONCURRENTLY plus the kind backfill via +-- the dbops task, so by the time this migration runs both the +-- column and the index already exist and the IF NOT EXISTS pair are +-- a no-op. Exact ops commands live in the PR description for issue +-- #23. Until the backfill is verified complete, the read path -- emits `kind IN (...) OR kind IS NULL` so un-backfilled rows still -- match. ALTER TABLE {{.Name}}__event_tags ADD COLUMN IF NOT EXISTS kind INTEGER; +CREATE INDEX IF NOT EXISTS {{.Name}}__idx_event_tags_key_value_kind_event_id + ON {{.Name}}__event_tags(key, value, kind, event_id);