mirror of
https://github.com/vitorpamplona/amethyst.git
synced 2026-10-05 11:18:24 +00:00
fix(store): pin (kind,pubkey) index for multi-author no-limit REQs
The profiles scenario — Filter(kinds=[0], authors=[50]), no limit — was geode's ~100x loss to strfry on the 1M corpus (99.5ms vs 0.94ms). Root cause: the REQ path always appends ORDER BY created_at DESC. For a multi-author pubkey IN(...) filter, query_by_kind_created (kind, created_at) satisfies that order for free by scanning an ENTIRE kind, so SQLite prefers it over the selective query_by_kind_pubkey_created (which would need a sort). The scan is O(all kind-0 profiles) — cheap at 2k, the 99.5ms at 1M. ANALYZE does not fix it (verified: even a reopened store reading fresh sqlite_stat1 keeps the scan, since the ORDER BY genuinely lets the scan skip a sort). A single author is costed right and already seeks; only the IN-list of >1 is mis-costed. Fix: pin INDEXED BY query_by_kind_pubkey_created for exactly that shape — multi-author + kinds, no ids, no d-tags, no limit — keeping the ORDER BY. SQLite seeks the authors and sorts the small result: identical rows, identical newest-first order (zero behavior change), ~8x at 2k profiles, growing to ~100x at 1M. Limited feeds (home/global) keep the created_at scan + early LIMIT; single-author and d-tag queries are untouched. The index is created unconditionally so the hint never dangles. (Considered dropping the ORDER BY for no-limit author queries — faster and hint-free, but it changes on-the-wire result ordering, which broke FsParityTest's ordered-parity assertions, so it's client-visible. Rejected in favor of the order-preserving pin.) Adds ProfilesQueryPlanBenchmark (regression guard: asserts the live REQ plan seeks the composite index, not the kind scan) and plans/2026-07-04-profiles-query-plan.md. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012EZeWww5TJnzBZKPoc6mvU
This commit is contained in:
@@ -1,8 +1,16 @@
|
||||
# The `profiles` query: why kind-0 + authors scans, and how to fix it
|
||||
|
||||
**Status: diagnosed + two fixes measured, not yet applied** (the fix is a
|
||||
core-QueryBuilder behavior change with an ordering trade-off — wants a
|
||||
maintainer call). Follow-up to the 1M relayBench run, where `profiles`
|
||||
**Status: shipped — Fix B applied** (the order-preserving one, chosen over
|
||||
Fix A because dropping the ORDER BY changed observable result ordering — it
|
||||
broke `FsParityTest`'s ordered-parity assertions, i.e. it's client-visible).
|
||||
`QueryBuilder.makeSimpleQuery` now pins `INDEXED BY query_by_kind_pubkey_created`
|
||||
for the exact broken shape — multi-author (`pubkey IN (…)`) + `kinds`, no
|
||||
`limit`, no d-tags — keeping the newest-first ORDER BY. The REQ plan for
|
||||
`profiles` seeks the composite index instead of scanning, ~8× at 42k profiles
|
||||
and growing with profile count (the ~100× at 1M), with **zero behavior
|
||||
change** (identical rows, identical order). Guarded by
|
||||
`ProfilesQueryPlanBenchmark`. Follow-up to the 1M relayBench run, where
|
||||
`profiles`
|
||||
(`Filter(kinds=[0], authors=[…50…])`, profile hydration, **no limit**) was
|
||||
the single query geode lost badly on: **99.5 ms p50 vs strfry 0.94 ms
|
||||
(~100×)**, while geode won or tied the other nine scenarios.
|
||||
@@ -51,15 +59,29 @@ At 2k profiles / 42k events (the gap widens with profile count):
|
||||
| **Fix A** — drop `ORDER BY` when `limit == null` | planner picks `query_by_kind_pubkey_created` itself | **0.17 ms (7×→~100× at 1M)** | grouped by author |
|
||||
| **Fix B** — force composite index, keep `ORDER BY` | `query_by_kind_pubkey_created` + TEMP B-TREE sort | **0.19 ms** | newest-first (unchanged) |
|
||||
|
||||
**Fix A** (recommended): in `makeSimpleQuery`, emit `ORDER BY created_at
|
||||
DESC` only when `limit != null`. Without a limit the relay returns the whole
|
||||
match set and ordering is a NIP-01 *SHOULD* (clients re-sort); dropping it
|
||||
lets SQLite choose the selective index on its own — no hint, no fragility —
|
||||
and speeds up **every** no-limit `kinds + authors` query (reactions,
|
||||
metadata, relay lists by a follow set), not just profiles. Trade-off:
|
||||
multi-author no-limit results come back grouped by author rather than
|
||||
globally newest-first. Single-author and kind-only no-limit queries keep
|
||||
their order (their chosen index is already `created_at`-ordered).
|
||||
**Fix A (rejected):** emit `ORDER BY` only when `limit != null || authors ==
|
||||
null`. Fastest (0.15 ms) and no hint, but dropping the order for multi-author
|
||||
no-limit queries is **client-visible** — it broke `FsParityTest`'s
|
||||
ordered-parity assertions, i.e. it changes what a REQ returns on the wire.
|
||||
Not worth a semantic change for this.
|
||||
|
||||
**Fix B (applied):** in `makeSimpleQuery`, pin `INDEXED BY
|
||||
query_by_kind_pubkey_created` when the filter is multi-author (`authors.size >
|
||||
1`) with `kinds`, no `ids`, no d-tags, and **no `limit`** — keeping the ORDER
|
||||
BY. SQLite seeks the authors and sorts the (small) result: same rows, same
|
||||
newest-first order, ~8× here / ~100× at 1M. The scoping is exact:
|
||||
|
||||
- **single author** (`pubkey = ?`) is already costed right and seeks — no pin;
|
||||
- **with a limit** (the 150-author home feed) the `created_at` scan + early
|
||||
`LIMIT` is the better plan — no pin;
|
||||
- **d-tag / addressable** filters have their own index — no pin;
|
||||
- **authors-only, no kinds** keeps the planner's choice (needs the
|
||||
flag-gated `query_by_pubkey_created`; it was only a ~3× minor loss, not the
|
||||
100× regression) — left for later.
|
||||
|
||||
`query_by_kind_pubkey_created` is created unconditionally, so the hint never
|
||||
dangles. No existing test changed — the only multi-author cases in
|
||||
`QueryAssemblerTest` carry a `limit` or a `search`, both excluded.
|
||||
|
||||
**Fix B** (zero behavior change): when the filter has `kinds` + `authors`
|
||||
and **no limit**, add `INDEXED BY query_by_kind_pubkey_created` and keep the
|
||||
|
||||
+26
@@ -963,6 +963,29 @@ class QueryBuilder(
|
||||
}
|
||||
}
|
||||
|
||||
// A multi-author query (`pubkey IN (…)`) with no limit, combined with
|
||||
// `ORDER BY created_at DESC`, mis-costs in SQLite: it satisfies the
|
||||
// order for free off `query_by_kind_created` (kind, created_at) by
|
||||
// scanning an *entire* kind, rather than doing N seeks on the
|
||||
// selective `query_by_kind_pubkey_created` (kind, pubkey, …) and
|
||||
// sorting the (small) result — the ~100× `profiles` regression at 1M.
|
||||
// Pin the selective index for exactly that shape:
|
||||
// - single author (`pubkey = ?`) is costed correctly and already
|
||||
// seeks — no pin needed;
|
||||
// - *with* a limit, the `created_at` scan + early LIMIT is the better
|
||||
// plan (the 150-author home feed), so leave it to the planner;
|
||||
// - d-tag/addressable filters have their own index.
|
||||
// Order is preserved (the ORDER BY stays); this only redirects the
|
||||
// index. See quartz/plans/2026-07-04-profiles-query-plan.md.
|
||||
val pinKindPubkeyIndex =
|
||||
project &&
|
||||
kinds != null &&
|
||||
authors != null &&
|
||||
authors.size > 1 &&
|
||||
ids == null &&
|
||||
dTags == null &&
|
||||
limit == null
|
||||
|
||||
val sql =
|
||||
buildString {
|
||||
if (project) {
|
||||
@@ -970,6 +993,9 @@ class QueryBuilder(
|
||||
} else {
|
||||
append("SELECT row_id FROM event_headers")
|
||||
}
|
||||
if (pinKindPubkeyIndex) {
|
||||
append(" INDEXED BY query_by_kind_pubkey_created")
|
||||
}
|
||||
if (clause.conditions.isNotEmpty()) {
|
||||
append("\nWHERE ")
|
||||
append(clause.conditions)
|
||||
|
||||
+38
-28
@@ -29,6 +29,8 @@ import com.vitorpamplona.quartz.utils.EventFactory
|
||||
import kotlinx.coroutines.runBlocking
|
||||
import kotlin.test.Test
|
||||
import kotlin.test.assertEquals
|
||||
import kotlin.test.assertFalse
|
||||
import kotlin.test.assertTrue
|
||||
|
||||
/**
|
||||
* relayBench's `profiles` scenario — `Filter(kinds=[0], authors=[…50…])`,
|
||||
@@ -45,15 +47,17 @@ import kotlin.test.assertEquals
|
||||
*
|
||||
* `ANALYZE` does **not** help — even a connection that reads fresh stats
|
||||
* keeps the scan, because with the ORDER BY the scan genuinely avoids a
|
||||
* sort. Two fixes work (see `plans/2026-07-04-profiles-query-plan.md`):
|
||||
* - **drop the ORDER BY when there is no limit** — the planner then picks
|
||||
* the composite index itself, ~16×, at the cost of result order across
|
||||
* authors (a NIP-01 SHOULD; clients re-sort);
|
||||
* - **force the composite index and keep the ORDER BY** — ~10×, seeks then
|
||||
* sorts the small result, newest-first order preserved.
|
||||
* sort. **Applied fix** (`QueryBuilder.makeSimpleQuery`, see
|
||||
* `plans/2026-07-04-profiles-query-plan.md`): for a **multi-author**
|
||||
* (`pubkey IN (…)`) filter with `kinds`, no `limit`, and no d-tags, pin
|
||||
* `INDEXED BY query_by_kind_pubkey_created` — SQLite then seeks the authors
|
||||
* and sorts the small result, keeping the newest-first ORDER BY (zero
|
||||
* behavior change). Single-author queries already seek; limited feeds keep
|
||||
* the `created_at` scan + early LIMIT.
|
||||
*
|
||||
* This is a diagnosis + regression artifact; it prints the plans and timings
|
||||
* and asserts only correctness (the row counts match across shapes).
|
||||
* This is now a regression guard: it asserts the live REQ plan for this
|
||||
* shape seeks the composite index (not the kind scan), and prints the
|
||||
* before/after timings.
|
||||
*/
|
||||
class ProfilesQueryPlanBenchmark {
|
||||
private val hex = "0123456789abcdef"
|
||||
@@ -155,29 +159,35 @@ class ProfilesQueryPlanBenchmark {
|
||||
|
||||
println("─ ProfilesQueryPlanBenchmark: $total events, $authorCount kind-0 profiles, filter kinds=[0]+50 authors ─")
|
||||
|
||||
// 1. Current REQ shape: ORDER BY created_at DESC, no limit.
|
||||
val (baseN, baseMs) = timeRaw("$base ORDER BY created_at DESC")
|
||||
println(" current (ORDER BY, no limit):")
|
||||
// The old (bad) shape: ORDER BY created_at DESC forces the kind scan.
|
||||
val (oldN, oldMs) = timeRaw("$base ORDER BY created_at DESC")
|
||||
println(" old shape (ORDER BY, no limit):")
|
||||
println(planTree(store.store.explainQuery("$base ORDER BY created_at DESC")))
|
||||
println(" → $baseN events, ${"%.3f".format(baseMs)} ms/run")
|
||||
println(" → $oldN events, ${"%.3f".format(oldMs)} ms/run")
|
||||
|
||||
// 2. Fix A: drop ORDER BY (no limit) — planner picks composite itself.
|
||||
val (fixaN, fixaMs) = timeRaw(base)
|
||||
println(" Fix A — no ORDER BY (no limit), planner's own choice:")
|
||||
println(planTree(store.store.explainQuery(base)))
|
||||
println(" → $fixaN events, ${"%.3f".format(fixaMs)} ms/run (${"%.1f".format(baseMs / fixaMs)}×)")
|
||||
// The fix, as the REQ path now builds it: INDEXED BY the composite
|
||||
// index for this multi-author, no-limit shape — seeks the authors,
|
||||
// keeps the newest-first ORDER BY (sorts the small result).
|
||||
val reqPlan = store.store.planQuery(profiles)
|
||||
val fixedSql = "SELECT $cols FROM event_headers INDEXED BY query_by_kind_pubkey_created WHERE kind = 0 AND pubkey IN ($inList) ORDER BY created_at DESC"
|
||||
val (newN, newMs) = timeRaw(fixedSql)
|
||||
println(" fixed REQ path (INDEXED BY composite, ORDER BY kept):")
|
||||
println(planTree(reqPlan))
|
||||
println(" → $newN events, ${"%.3f".format(newMs)} ms/run (${"%.1f".format(oldMs / newMs)}× faster, grows with profile count)")
|
||||
|
||||
// 3. Fix B: force composite index, keep ORDER BY (order preserved).
|
||||
val hinted = "SELECT $cols FROM event_headers INDEXED BY query_by_kind_pubkey_created WHERE kind = 0 AND pubkey IN ($inList) ORDER BY created_at DESC"
|
||||
val (fixbN, fixbMs) = timeRaw(hinted)
|
||||
println(" Fix B — INDEXED BY composite + ORDER BY (newest-first preserved):")
|
||||
println(planTree(store.store.explainQuery(hinted)))
|
||||
println(" → $fixbN events, ${"%.3f".format(fixbMs)} ms/run (${"%.1f".format(baseMs / fixbMs)}×)")
|
||||
|
||||
// Correctness: every shape returns the same events (all 50 here).
|
||||
assertEquals(baseN, fixaN, "Fix A must return the same rows")
|
||||
assertEquals(baseN, fixbN, "Fix B must return the same rows")
|
||||
assertEquals(store.query<Event>(profiles).size, baseN, "REQ path matches raw SQL")
|
||||
// Regression guards: the live REQ path must seek the composite
|
||||
// index (not scan the kind-only one) and preserve the ORDER BY.
|
||||
assertEquals(oldN, newN, "row count unchanged by the fix")
|
||||
assertEquals(newN, store.query<Event>(profiles).size, "REQ path returns the match set")
|
||||
assertTrue(
|
||||
reqPlan.contains("query_by_kind_pubkey_created"),
|
||||
"REQ plan should seek the (kind, pubkey) index, was:\n$reqPlan",
|
||||
)
|
||||
assertFalse(
|
||||
reqPlan.contains("query_by_kind_created "),
|
||||
"REQ plan must not scan the kind-only index, was:\n$reqPlan",
|
||||
)
|
||||
assertTrue(reqPlan.contains("ORDER BY"), "newest-first order is preserved, was:\n$reqPlan")
|
||||
|
||||
store.close()
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user