Repository navigation
Add compact ClickHouse lookups for preview profiles, zaps and naddr - #67
Conversation
nstr.to's profile, zap and naddr reads filter events_local by kind, so they depend on the ~1.33 TiB full-row events_by_kind projection; without it they take 20-60+ seconds. Add three small lookup tables (prepared migration 017) fed by insert-triggered views, a bounded backfill tool, and a reversible PREVIEW_COMPACT_LOOKUPS flag that keeps the legacy reads for comparison and rollback. The projection is not touched. - profile_latest / addressable_latest: argMin states keyed by pubkey and (pubkey, kind, d_tag); newest created_at wins, lowest id breaks ties - zap_receipts_by_target: one row per (target, receipt) with migration 009 amount rules and a parser_version for side-by-side corrections - reads collapse duplicates themselves, so results are identical before and after merges, under replays and out-of-order arrival - preview-lookups tool: plan (disk estimate), backfill (projection-enforced, gap preflight, disk/parts guards, resumable checkpoints), status, validate (Rust oracle + legacy classification), bench (projections and query cache off) - integration tests against ClickHouse 24.8 (just ch-test-up && just test-clickhouse) plus a CI job; runbook for staged rollout, rollback and projection-consumer audit Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
erskingardner
left a comment
erskingardner
left a comment
There was a problem hiding this comment.
| Review metadata | Value |
|---|---|
| Reviewed at (UTC) | 2026-10-10T19:15:22Z |
| Commit reviewed | d6e6ffa3c793347bc9798468534014c1c19bf2ba |
| Model | gpt-6.1-sol |
| Reasoning level | high |
| Recommended action | Resolve serious concerns before merge |
The compact lookup approach addresses the measured projection dependency, and the reads explicitly collapse duplicates. Three operator-tool issues should be fixed before using the documented production rollout: validation can pass without any comparisons, concurrent inserts can produce false validation failures, and the gap pass bypasses the normal resource guards. Details and regression cases are inline.
Validation: reviewed the remote code at the exact head and existing CI. Rustfmt, Clippy, operational tests, doctests and ClickHouse integration (24.8) have passed. Still running: test (shard 1/3), test (shard 2/3), test (shard 3/3). No local build or tests were run, and no checkout state was changed.
This is a comment review because GitHub is authenticated as the PR author and does not permit a formal request-changes review on one's own PR.
| let report = validate::run(&client, &database, &options).await?; | ||
| print!("{}", report.render()); | ||
| write_report(out.as_ref(), &report)?; | ||
| Ok(if report.unexplained() == 0 { | ||
| ExitCode::SUCCESS |
There was a problem hiding this comment.
[P2] Treat zero validation coverage as inconclusive
The success condition only checks differences, so a nonempty target whose sampled keys are all skipped by the cutoff returns success without comparing a single compact result. For example, import/replay the source rows less than the default 300 seconds ago and validate while the compact history is incomplete: every sampled key is skipped, both outcome maps stay empty, and the command prints unexplained differences: 0 and exits 0. This satisfies the runbook's cutover gate despite providing no correctness evidence. Require actual comparisons for each nonempty requested target (or explicitly return an inconclusive/nonzero result and replenish the sample with eligible keys), and cover the all-skipped case.
| let compact_rows = compact::fetch_profiles(ctx.compact, batch).await?; | ||
| let legacy_rows = query::fetch_profiles(ctx.legacy, batch).await?; | ||
|
|
||
| for key in batch { | ||
| let versions = by_key.get(key.as_str()).cloned().unwrap_or_default(); | ||
| if versions.iter().any(|r| r.indexed_at > ctx.cutoff) { |
There was a problem hiding this comment.
[P2] Recheck source freshness after reading the lookup results
The cutoff check uses only the source rows fetched before these two lookup queries. If a new winning profile is inserted between the source read and fetch_profiles, the materialized view correctly exposes the new event, but versions still contains only the old rows. The key passes the cutoff check and the correct compact answer is classified as Unexplained. The same race exists for zap receipts and naddr updates; it is particularly relevant to the hot keys and the long per-key validation loop while live ingestion continues. Bracket the comparison with a second source freshness check and skip/retry changed keys, or use a comparison strategy that really shares a snapshot. Add a regression covering an insert between the source and compact reads.
| let insert_client = projection_client(client, plan.require_projection); | ||
| // Read amplification is expected here (indexed_at is not a key column). | ||
| let options = BackfillOptions { | ||
| max_read_amplification: f64::INFINITY, | ||
| ..options.clone() | ||
| }; | ||
| let checkpoint = run_shard( |
There was a problem hiding this comment.
[P2] Apply backfill safety guards to the gap pass too
run_gap_pass goes directly to run_shard, bypassing the disk and merge-backlog checks in run_backfill. In particular, rerunning a completed plan with backfill --gap-pass --min-free-gib 100 skips the entire guarded shard loop and still starts this INSERT even when unreserved space is already below 100 GiB or the target has too many parts. The pass can write every receipt/state in its indexed-at window, so it can consume the reserve that the operator explicitly asked to preserve. Check disk space and target backlog before the gap INSERT, account for its expected output, and honor an exhausted session runtime before starting it.
erskingardner
left a comment
erskingardner
left a comment
There was a problem hiding this comment.
| Review metadata | Value |
|---|---|
| Reviewed at (UTC) | 2026-10-10T19:20:41Z |
| Commit reviewed | d6e6ffa3c793347bc9798468534014c1c19bf2ba |
| Model | Grok 4.7 |
| Reasoning level | Not exposed by runtime |
| Recommended action | Fix blocking issues before merge |
The compact tables, read-time collapse, and reversible flag are the right way to take profile, zap, and naddr previews off the 1.33 TiB events_by_kind projection. CI is green on this commit, including the ClickHouse 24.8 job.
One backfill guard fails open. If system.query_log never records a shard, that shard is still checkpointed and the run continues. That check is what stops a partial-projection scan. Details inline.
Posted as a comment review because GitHub rejects request-changes from the pull request author. Treat it as a request for changes.
| if let Some(entry) = &query_log { | ||
| check_projection_use(plan, index, estimated_rows, entry, options)?; | ||
| } else { | ||
| tracing::warn!( |
There was a problem hiding this comment.
When require_projection is set, a missing system.query_log row still falls through to the checkpoint below. force_optimize_projection = 1 only proves the planner used events_by_kind for some input. It does not catch parts that lack the projection and were read from the base table. The read_rows amplification check is what catches that, and the runbook says a shard that cannot show projection use stops the run.
Refuse to checkpoint in this branch when plan.require_projection is true, the same as check_projection_use returning an error. A retry redoes one shard. Continuing after an unverified shard is how a partial-projection backfill turns into a sequence of full scans.
- validate: re-check source freshness after the compact and legacy reads so inserts that land mid-validation are skipped instead of reported as differences; a target whose sampled keys were all skipped is inconclusive and fails the run - backfill: when the projection is required, a shard without a system.query_log entry is not checkpointed (fail closed) - gap pass: size it first and apply the runtime, disk and merge-backlog guards like any shard; disk guard arithmetic is overflow-safe - regression tests for each case Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
erskingardner
left a comment
erskingardner
left a comment
There was a problem hiding this comment.
| Review metadata | Value |
|---|---|
| Reviewed at (UTC) | 2026-10-10T19:33:04Z |
| Commit reviewed | cad1b67189c2170d1397c42c032fa34de55ca7fa |
| Model | gpt-6.1-sol |
| Reasoning level | high |
| Recommended action | Resolve serious concerns before merge |
The earlier all-skipped validation, concurrent-insert freshness, missing-query-log, and gap-pass disk/backlog cases have been addressed. Two operator-tool gaps remain: an empty sample is still accepted without proving an empty source, and sizing/merge waits can exhaust the session budget before another INSERT starts. Please address the inline cases before relying on the production cutover and bounded-session procedures.
Validation: remote/API-only inspection at this exact head. Existing CI passed: operational tooling tests, rustfmt, clippy, doctests, ClickHouse integration (24.8). Still pending: test (shard 1/3), test (shard 2/3), test (shard 3/3). No local build or tests were run and no repository state was changed.
Posted as a comment review because the authenticated GitHub account is the PR author; GitHub does not allow request-changes reviews on one's own PR.
| /// Keys were sampled but none could be compared, so the target provides | ||
| /// no correctness evidence. | ||
| pub fn inconclusive(&self) -> bool { | ||
| self.sampled > 0 && self.compared() == 0 |
There was a problem hiding this comment.
[P2] Distinguish an empty sample from an empty source
sampled == 0 is treated as a pass, but the report contains no source cardinality evidence. For example, with historical profile rows in events_local and an empty profile_latest, validate --target profiles --allow-full-scan --sample 0 --hot 0 runs no comparisons and still exits 0. This can also happen with random-only validation (--hot 0) when the hashed fraction selects no keys. The new inconclusive guard catches all-skipped samples but leaves this zero-coverage route through the cutover gate. Reject configurations that disable all sampling, and when sampling returns no keys, establish that the target's source key set is empty or return an inconclusive result. Add a nonempty-source/empty-sample regression alongside the all-skipped test.
| if let Some(backlog) = wait_for_merges(client, plan, options).await? { | ||
| return Ok(StopReason::PartsBacklog(backlog)); | ||
| } |
There was a problem hiding this comment.
[P2] Recheck the session deadline after sizing and merge waits
The gap pass checks only whether the supplied budget is zero at entry. Its sizing query can take up to the per-query execution cap, and wait_for_merges can then wait 30 minutes; neither consumes/rechecks max_runtime before run_shard starts. A pass entered with 30 seconds remaining can therefore spend minutes sizing/waiting and then launch the full INSERT after the authorized session window. The ordinary shard loop has the same problem: its elapsed-time check precedes wait_for_merges, with no check after the wait. Track an absolute session deadline, bound merge polling by it, and recheck it immediately before starting an INSERT (including after gap sizing). Cover a positive budget that expires during a wait; the existing zero-budget test cannot catch this case.
erskingardner
left a comment
erskingardner
left a comment
There was a problem hiding this comment.
| Review metadata | Value |
|---|---|
| Reviewed at (UTC) | 2026-10-10T19:38:07Z |
| Commit reviewed | cad1b67189c2170d1397c42c032fa34de55ca7fa |
| Model | Grok 4.7 |
| Reasoning level | Not exposed by runtime |
| Recommended action | Merge |
The compact tables, read-time collapse, and reversible PREVIEW_COMPACT_LOOKUPS flag are the right way to take profile, zap, and naddr previews off events_by_kind without dropping the projection.
The follow-up commit closes the earlier findings: validation fails closed when every sampled key is skipped, freshness is rechecked after the compact and legacy reads, the gap pass uses the disk, merge-backlog, and exhausted-runtime guards, and a required projection with no query_log row is not checkpointed. ClickHouse 24.8 integration and the rest of CI are green on this commit.
No further changes needed before merge. Production still needs the runbook preflight, backfill, and bench before the flag is switched; that stays outside this PR.
Posted as a comment review because GitHub rejects an approval from the pull request author. Treat it as an approval.
…ion samples - backfill and gap pass: max_runtime is an absolute session deadline that bounds merge waits and is re-checked immediately before every INSERT, so no INSERT starts after the window closes - validate: reject --sample 0 --hot 0; when sampling finds no keys, check whether the source has any and report the target inconclusive if so - regression tests for a budget expiring mid-wait and for an empty sample of a non-empty source Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
PREVIEW_DISABLE_PROJECTIONS=true runs every preview query with optimize_use_projections=0, so real nstr.to traffic can prove the compact lookups before the events_by_kind projection is dropped. Queries are capped at 10 s and cancelled when their request is dropped, so any lookup still on its legacy path fails fast instead of piling up table scans; startup warns unless every lookup is compact. Off by default; rollback is a restart. - integration test asserts every preview query logs the setting, a cap and no projection use (and fails when the setting is off) - runbook step 9: rehearsal procedure, pass criteria and rollback Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Production preflight (2026-10-10) shows the ingester inserts synchronously: async_insert = 0 for its user and system.asynchronous_insert_log is empty, because the async settings in ops/production/clickhouse/config.xml are profile settings in a server config.d file, which ClickHouse does not apply. A failing view therefore fails the INSERT (retried and reindex-queued) rather than dropping rows silently. - design doc and migration 017 header describe the real failure mode - runbook preflight and post-migration checks use query_views_log and query_log insert exceptions instead of the empty asynchronous_insert_log - malformed-event test covers synchronous as well as async inserts Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Production sizing (2026-10-10) found 646M rows across all replaceable and addressable kinds, dominated by bulk machine kinds (30382 alone is 219M), so backfilling all of them would need ~188 GiB of worst-case headroom against 256 GiB free. naddr demand was 4 previews in 7 days, all kind 30023, and the legacy naddr query never used events_by_kind. Cover only shareable content kinds (articles, video, live events, wiki, publications, listings, calendar, communities, marketplace, curation sets, git repos, music, starter packs): about 12.7M rows and 2.9M coordinates. Other naddr kinds keep the legacy query. The kind list lives in one Rust constant and a unit test requires migration 017's view to filter on exactly that list. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 participant
Problem
nstr.to previews look up profiles, zap totals and naddr articles by filtering the main events table on
kind. That table is ordered by event id, so these lookups are only fast because of a 1.33 TiB full-rowevents_by_kindprojection. Production sampling with projections disabled showed profile lookups taking 21–30 s and an event-zap lookup taking more than 60 s. That means we can't reclaim the disk without breaking previews for every visitor. The existing reads also breakcreated_atties arbitrarily (a profile's id and content can come from two different events), and they double-count zap receipts that have been replayed but not yet merged.Fix
Three small lookup tables answer exactly the questions previews ask, keyed by pubkey, zap target, and (pubkey, kind, d tag). Views keep them current on every insert. Every read collapses duplicates itself, so answers are correct before background merges as well as after, and under replays or out-of-order arrival. A bounded, resumable backfill loads history while the projection still exists. A per-lookup flag switches previews over and can switch them back, with the old reads left in place for comparison and rollback.
What's in the branch
docs/migrations/prepared/017_preview_compact_lookups.sql, with a rollback. It is excluded fromch-migrate-all.PREVIEW_COMPACT_LOOKUPSflag:none(default),all, or any ofprofiles,zaps,addressable. Preview queries are tagged insystem.query_logper lookup and source.preview-lookupstool:plan: disk estimate including merge headroom.backfill:status.validate: compares against an independent Rust reference at a fixed cutoff and labels known legacy defects.bench: runs with projections and the query cache off.PREVIEW_DISABLE_PROJECTIONSrehearsal setting (off by default). When on, every preview query runs withoptimize_use_projections = 0, capped at 10 s and cancelled if its request is dropped. Real traffic can then prove the change before the projection is dropped (runbook step 9).docs/preview_compact_lookups.md. Staged rollout, rollback and the projection-consumer audit are inops/RUNBOOK.mdunder "Preview compact lookups".Behaviour differences
events_by_kind.Not in this PR
events_by_kindprojection is not dropped. Removing it remains a separate manual operation after validation, the projections-off rehearsal and the consumer audit, coveringpensieve-servekinds/stats, negentropy seeding and Grafana.Needs production validation
events_localDDL, and that every part carriesevents_by_kind./data, especiallyaddressable_latest.asynchronous_insert_log,query_views_log).bench --drop-cachesin a quiet window. The sub-second target is not claimed until this is measured.query_logaudit of other projection consumers.Test plan
just precommitjust ch-test-up && just test-clickhousepensieve-previewlocally with the flag set tononeand toall; the profile, zapped-note and naddr JSON responses were identical.🤖 Generated with Claude Code