perf(db): serve single-root thread reply reads from thread_metadata by TheSentinel454 · Pull Request #9075 · block/buzz

TheSentinel454 · 2026-11-04T21:26:47Z

Summary

A plain thread-reply read, {kinds:[9], #h:[channel], #e:[root], limit:100}, goes through the generic query_events path. That path gathers every event tagging the root into a MATERIALIZED CTE, then sorts it to keep one page. On a root with 500k replies, each page costs about 514k buffer reads plus a temp-file spill.

This PR serves single-root reply reads from thread_metadata instead. It walks idx_thread_metadata_window newest-first, fetches each reply by primary key, and applies every filter predicate before LIMIT. Page cost scales with the page size, not with the thread.

What changed

Routing (crates/buzz-db/src/store/event.rs). A query_events read takes the new route when it has:
- one channel,
- one lowercase-hex #e root,
- kinds limited to 9 and/or 40002,
- no #p filter (any number of values; the relay marks such filters exact_match),
- no authors, pubkey or ids selector. The route tests those per reply, so an author with no reply in a hot thread would walk the whole thread; the generic read narrows by author first.
Every other #e read stays on the fenced path from 🤖 perf: bound long-thread aux reads and make query deadlines terminal #7854.
Root check before routing. #e must name a thread root. A cheap probe looks up the target in thread_metadata first. If the target is itself a reply, the read takes the generic path, which returns its direct children the NIP-01 way. A reply counts if its row has a root, or if it is a rootless legacy stub whose own NIP-10 markers resolve to a reply.
COUNT unchanged. COUNT post-filter fallbacks (count.rs, bridge.rs) set the new EventQuery::exact_match flag through prepare_count_fallback. Routing is skipped for them, so COUNT still counts every event that matches under NIP-01.
Same predicates on both paths. The predicate block moved out of build_query_events_sql into push_event_filters, which both paths call. Filters on the route therefore match the generic path, including the #e tag match, since/until/before_id and deletion.
Unnamed statements. Routed reads run as unnamed statements, so Postgres plans each one with its root and never caches a generic plan. The benchmark below shows the routed SQL is also fast under a forced generic plan, so this is a second safeguard.
Narrowed semantics, documented. Messages that name the root only in a bare e tag, a mention tag or a lone root marker are top-level under NIP-10. They have no thread_metadata row, so the routed read omits them. Live subscriptions still deliver them. docs/nips/NIP-CW.md gains a "Plain Thread-reply Filters" section describing this. A client that needs those messages can add another kind to the filter, which sends the read down the generic path.

Benchmark (synthetic, Postgres 16, 500k-reply thread)

Median of 5, newest page of 100. "Rare" is a root with 3 replies. "Generic" forces plan_cache_mode = force_generic_plan.

	main	this PR
hot root, custom plan	2802 ms (513k buffers, 46,750 temp blocks written)	1.6 ms (407 buffers)
hot root, generic plan	1779 ms	0.6 ms
rare root, custom plan	1.2 ms	0.8 ms
rare root, generic plan	61 ms	0.08 ms

Author-filtered reads (authors:[x], limit:1) on the hot root: an author with no reply there takes 1.4 ms on the generic path but 1.7 s on the route, so author and id selectors stay generic.

On both roots, the PR's SQL returns the same rows as main.

Tests

e_tag_filter_shape_tests::thread_reply_route_admits_only_single_root_message_reads checks which reads take the route. Two roots, mixed kinds, no kinds, no channel, an uppercase root, a #p join, an exact_match read, and authors/pubkey/ids selectors all stay generic. The routed SQL is checked for the thread_metadata walk, the #e match and the window ordering, with no MATERIALIZED.
postgres_tests::thread_reply_reads_use_thread_metadata (Postgres lane) covers:
- mixed 9/40002 replies, a nested reply and a soft-deleted reply,
- a bare-e message that the route omits and the generic read (kinds [9,40002,7]) still returns,
- a lone-root message, which the route omits too,
- an exact_match read, which returns the same rows as the generic read,
- a nested-parent target, which gets its direct child the generic way,
- root stubs and a legacy parent with no row of its own, checked through the probe,
- keyset paging,
- no named prepared statement left behind after repeated runs.
handlers::req::tests::postgres_tests::multi_p_thread_filter_keeps_nip01_results (Postgres lane) runs a two-value #p thread filter through the real filter-to-query adapter and checks that a bare reference to the root still comes back.
handlers::req::tests::count_fallback_fetches_one_extra_candidate checks that COUNT fallbacks set exact_match. It is DB-free and was missing from the buzz-relay --lib allowlist, so this PR adds it to just test-unit.
Mutation-checked: the Postgres test fails if any of these is broken:
- routing disabled,
- persistent statements,
- the nested-target probe dropped,
- stubs treated as all replies or as no replies,
- COUNT allowed to route,
- the deleted filter dropped.
Blox gate: cargo fmt, clippy (-p buzz-db -p buzz-relay --all-targets), buzz-db lib (nextest and plain parallel cargo test), buzz-relay lib, and the buzz-db and buzz-relay Postgres lanes. One unrelated artifact-sync scale test timed out under load; it passes alone and in CI.

What got simpler

One predicate builder replaces the inline block, so the two read shapes cannot drift apart.
One targeted route replaces the planned extra fence variant (a keys-only MATERIALIZED CTE). That variant was dropped because it reduced the spill but still scanned every reply.

Identified debt (not in this PR)

The bridge page extension (crates/buzz-relay/src/api/bridge.rs, extract_page_offset, built for the kind:0 directory listing) still maps to SQL OFFSET. The routed read honours it too, for parity. Deep pages there should move to keyset (until + before_id).

buzz-review-completed

🤖 Generated with Claude Code

A plain `#e:[root]` read for channel messages gathered every event that tags the root into a MATERIALIZED CTE, then sorted it to keep one page. For a root with 500k replies that is ~514k buffer reads and a temp-file spill per page (2.5 s on a synthetic repro). Route reads with one channel, one lowercase-hex root, only kinds 9/40002 and no #p join through idx_thread_metadata_window: walk the root's replies newest-first, fetch each by primary key, and apply every filter predicate (the #e match included) before LIMIT. The statement is unnamed so Postgres plans it with the root. Other #e reads keep the fenced path. Messages that name the root only in a bare or mention e tag have no thread_metadata row and are omitted by the routed read; NIP-CW documents the deviation and a Postgres test pins it. Co-authored-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz> Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Luke Tornquist <[email protected]>

Co-authored-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz> Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Luke Tornquist <[email protected]>

github-actions · 2026-11-04T21:31:08Z

🔐 Codex Security Review

Note: This is an automated, security-focused review generated by Codex.
Use it as a supplement to human review; false positives are possible.

Scope

Exact PR diff: 6b8a72e8355125d24e4f4fab1e083dced4119f5c...01ec530b66639e19b17c64975f233c664e1c4e3d

Model: gpt-6.1-sol

💡 Click "edited" above to see earlier reviews for this PR.

Review Summary

Overall Risk: NONE

No concrete security, correctness, or reliability findings identified in the authorized PR range.

Findings

No concrete security, correctness, or reliability findings were identified.

Notes

Review used read-only source and history inspection. Builds and tests were not executed, as requested.

_{Generated by Codex Security Review |

Requested by: @TheSentinel454 |

Workflow run}

thread_metadata records every reply under its thread's root, so routing `#e:[P]` for a nested parent P through `root_event_id = P` returned none of P's correctly marked replies. Take the thread route only when the target is not itself a recorded reply (one idx_thread_metadata_event_id probe); nested-parent reads keep the NIP-01 generic path. The Postgres test now builds replies as ingest records them (direct replies carry a `reply` marker), adds a lone-`root` message that ingest classifies as top-level beside the bare one, and reads the nested parent. NIP-CW names the lone-`root` omission and the nested-target fallback. Co-authored-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz> Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Luke Tornquist <[email protected]>

Inserting a reply leaves a depth-0 thread_metadata stub for its root, so the nested-target probe saw every root with replies as a reply and sent it back to the generic read. Count only rows that name a root. The test asserts the stub exists and that the root is not a reply while P is. Co-authored-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz> Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Luke Tornquist <[email protected]>

Ingest stubs a parent that has no thread row of its own (a reply stored before it was indexed) and recovers its ancestry from its NIP-10 tags. That stub has no root, so the nested-target probe took the parent for a thread root and routed `#e:[parent]` to an empty walk. When the target has only a stub, parse its own markers and fall back if they resolve to a reply. The test adds a legacy reply whose child must still be found. Found by the Codex security review on the PR. Co-authored-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz> Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Luke Tornquist <[email protected]>

The reply probe now joins `thread_metadata tm` too and is a fixed-shape persistent statement, so match the routed read by its root predicate. Co-authored-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz> Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Luke Tornquist <[email protected]>

COUNT filters that need post-filtering read their candidates through query_events, so a single-root thread filter there took the narrowed thread route and undercounted bare and lone-`root` references that the SQL COUNT path counts. Add EventQuery::exact_match, set it in the shared COUNT fallback setup (renamed prepare_count_fallback), and keep such reads off the route. NIP-CW states COUNT is unaffected. Found by the Codex security review on the PR. Co-authored-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz> Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Luke Tornquist <[email protected]>

count_fallback_fetches_one_extra_candidate pins that COUNT fallbacks keep exact NIP-01 matching, but it is DB-free and outside the buzz-relay --lib allowlist, so no CI lane ran it. Co-authored-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz> Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Luke Tornquist <[email protected]>

loganj

🤖 Larry and Auto: an independent adversarial review of 629c8cb. Source review only: we read the code and did not run the Postgres tests.

Logan asked us to look hard at whether this route can return different rows. Short answer: when the route is taken, its rows are always a subset of the generic rows, in the same order. build_thread_reply_sql re-applies all of push_event_filters (the exact #e match included) plus community and deleted_at, and the LATERAL join pins (tm.event_created_at, tm.event_id) to (e.created_at, e.id), so ordering, ties, since/until/before_id, limit and offset all match. The only difference is rows the route omits. All current writers of kind 9/40002 replies (ingest resolve_nip10_thread_meta, workflow resolve_relay_reply_thread_meta) write the event and its thread_metadata row in one transaction, or reject the event, so live data omits only the documented bare/mention/lone-root messages.

Callers that take the route today: buzz-acp thread context and the buzz-app agent harness runtimes (kinds: [9, 40002]). The desktop thread view (adds 40008, uses thread_window/depth_limit), mobile (depth_limit), and buzz messages thread (adds 40003/40008/45003) stay on their current paths.

Inline: one advice on a perf shape the route makes slower, plus doc and hardening notes. Nothing must-fix. Approving.

Review follow-ups: - Reads with `authors`, a single `pubkey`, or `ids` stay on the generic path. The thread route tests those per reply, so an author with no reply in a 500k-reply thread walked the whole thread (1.7 s) where the generic read narrows by author first (1.4 ms). - The root probe orders a reply row and a joinable row first, so a stub written with a fallback timestamp cannot hide a reply. - NIP-CW lists every routing condition (no `#p`, `authors`, `ids`; lowercase hex), names replies stored before thread metadata, and states that COUNT can exceed the read on purpose. Co-authored-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz> Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Luke Tornquist <[email protected]>

Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Luke Tornquist <[email protected]>

wpfleger96

🤖 One blocking problem: filters with more than one #p value take the new thread route, though the new spec text says filters with #p never do. Everything else checks out. The routed query joins on the full (community_id, created_at, id) key. It applies deleted_at IS NULL and the shared push_event_filters block before LIMIT, and its order matches the generic read's. since, until, before_id, limit/max_limit, OFFSET and kind 40002 behave as before. The root check is scoped to the community. COUNT fallbacks stay on exact NIP-01 matching. Ingest rejects a reply whose parent it can't find, so every reply accepted through the relay gets a thread_metadata row.

IMPORTANT: a filter with two or more `#p` values takes the narrowed route

crates/buzz-db/src/store/event.rs:874, together with crates/buzz-relay/src/handlers/req.rs:1287–1294.

thread_reply_root decides "no #p" from q.p_tag_hex.is_none(). But filter_to_query_params sets p_tag_hex only when #p has exactly one value. With two or more values it stays None, so a filter like {kinds:[9,40002], #h:[ch], #e:[root], #p:[alice, bob]} passes the check and is served from thread_metadata. docs/nips/NIP-CW.md says a filter with #p is not a plain thread-reply read and keeps full NIP-01 results.

Here is what goes wrong. A message that names the root only in a bare e tag and carries ["p", alice] comes back for #p:[alice]. It disappears for #p:[alice, bob], because the route never sees it and the NIP-01 post-filter can only drop rows, not add them. WebSocket REQ history and HTTP /query both go through this adapter.

Fix: carry "the filter had any #p" into the query and exclude the route on it. One way is to set exact_match = true in filter_to_query_params whenever #p is present. A regression through the real filter-to-query seam, with two distinct #p values and a matching bare-reference event, would cover it. The current shape test only sets p_tag_hex = Some(..) (event.rs:3207).

Live run

On a relay built from this head and one built from the base commit, with the same seeded events, plain thread-reply REQs differ only by the omissions the new NIP-CW.md section documents: bare, mention and lone-root references, and a reply with no thread_metadata row. until, nested-parent reads, kind 40002, the generic REQ, COUNT, private-channel access and community isolation all agree. Two-at-a-time keyset paging, including a three-event timestamp tie, reproduces the unpaged set with no duplicates. thread_reply_reads_use_thread_metadata, the filter-shape tests and count_fallback_fetches_one_extra_candidate pass.

Non-blocking

No migration rebuilds thread_metadata for existing data. A reply stored without its row (imported, or written outside relay ingest) now drops out of plain thread reads. The new Postgres test shows this too (event.rs:3816–3848). Your production count in the review thread found no real reply missing its row. Other deployments and future imports would need the same check.

NIP-CW says a filter with `#p` is never a plain thread-reply read, but `filter_to_query_params` sets `p_tag_hex` only for a single `#p` value. A filter with two or more `#p` values therefore took the route and lost messages that name the root only in a bare `e` tag. The adapter now marks every `#p` filter `exact_match`. multi_p_thread_filter_keeps_nip01_results pins it through the real filter-to-query seam against Postgres. Co-authored-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz> Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Luke Tornquist <[email protected]>

Co-authored-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz> Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Luke Tornquist <[email protected]>

rereview

TheSentinel454 · 2026-11-05T15:43:42Z

🤖 @wpfleger96 thanks, good catch on the multi-#p hole. Fixed in 8468a6f and 01ec530:

filter_to_query_params now sets exact_match whenever the filter has any #p value. exact_match already keeps reads off the thread route, so every #p filter gets the full NIP-01 read, whether it has one value or many. The exact_match doc now says so.
New Postgres regression test handlers::req::tests::postgres_tests::multi_p_thread_filter_keeps_nip01_results. It runs through the real seam (filter_to_query_params → query_events_routed → filters_match) with #p:[alice, bob] and a reply that names the root only in a bare e tag and carries ["p", alice]. Without the fix, the test fails ([] vs the bare reply's id). The test ran and passed in the PostgreSQL Domain job at 01ec530.

On the non-blocking note: agreed, and this PR doesn't change it. Any deployment that imports replies outside relay ingest needs the same thread_metadata check. NIP-CW's "Plain Thread-reply Filters" section names that omission.

wpfleger96

🤖 Approving: the multi-#p routing fix is verified.

TheSentinel454 and others added 2 commits November 4, 2026 16:25

test(db): satisfy fmt and clippy in thread-route tests

94d32ca

Co-authored-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz> Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Luke Tornquist <[email protected]>

TheSentinel454 marked this pull request as ready for review November 4, 2026 21:27

TheSentinel454 requested a review from a team as a code owner November 4, 2026 21:27

TheSentinel454 temporarily deployed to codex-review November 4, 2026 21:27 — with GitHub Actions Inactive