perf: use prepared statements for frequently-called server DB queries - #1878
perf: use prepared statements for frequently-called server DB queries#1878Joel-Claw wants to merge 2 commits into
Conversation
Add 13 prepared statements to the DB struct, prepared once at OpenDB time and reused across all requests. Previously, every call to GetStats, GetRoleCounts, GetAllRoleCounts, resolveNodePubkey, GetObservationsForHash, GetMaxTransmissionID, GetMaxObservationID, and count-only fast paths in QueryPackets/GetPacketGroups issued raw db.conn.QueryRow calls that required SQLite to parse and compile the SQL from scratch each time. Statements prepared: - COUNT(*) FROM transmissions (used in GetStats + 2 query fast paths) - COUNT(*) FROM observations (GetStats) - COUNT(*) FROM nodes WHERE last_seen > ? (GetStats) - COUNT(*) FROM nodes (GetStats) - COUNT(*) FROM observers WHERE inactive IS NULL OR inactive = 0 - COUNT(*) FROM observations WHERE timestamp > ? (last hour + last day) - SELECT public_key FROM nodes WHERE public_key = ? OR name = ? (resolveNodePubkey) - SELECT id FROM transmissions WHERE hash = ? (GetObservationsForHash) - COUNT(*) FROM nodes WHERE role = ? AND last_seen > ? (GetRoleCounts) - COUNT(*) FROM nodes WHERE role = ? (GetAllRoleCounts) - COALESCE(MAX(id), 0) FROM transmissions (GetMaxTransmissionID) - COALESCE(MAX(id), 0) FROM observations (GetMaxObservationID) The ingestor already uses prepared statements (10 stmts in db.go). This brings the server in line with the same pattern. Matches issue Kpa-clawbot#1875.
…atibility Test DB setup functions (setupTestDB, setupTestDBv2, setupTestDBV2, setupCapabilityTestDB) don't call prepareStatements(), leaving prepared statement fields nil. This caused TestBridgeScore_HandleNodesSurface to panic with nil pointer dereference at database/sql.(*Stmt).QueryRowContext. Add stmtQueryRow helper that uses the prepared statement when non-nil (production path) or falls back to a direct db.conn.QueryRow query when nil (test path). Update all 14 prepared statement call sites to use the helper. Production behavior is unchanged — prepared statements are still used when available. Test DBs without prepareStatements() now gracefully fall back to ad-hoc queries.
|
Review from the queue triage. Written 2026-08-30 against the tree at that time; posting now that the maintenance window in #1922 has opened. Verdict: approve with comments. No correctness defect found.
|
|
This went CONFLICTING because #1909 merged first, which I flagged in my review above as the deliberate order: #1909 is the correctness fix ( The conflict is in the two places I named: both PRs rewrite Want me to do the rebase? I have done the same for #1916, #1771 and #1863 today, keeping the original authorship on the commits and opening it as a continuation PR rather than force-pushing to your branch. Say the word and I will, or take it yourself if you would rather. My review points are unchanged and none of them block: the duplicated SQL between |
|
Yes, please go ahead with the rebase. Happy to let you handle it given you have the context from the other rebases today. Thanks. |
… (rebase of #1878) (#1934) Continues #1878 by @Joel-Claw, at their request. Both commits are theirs, authorship unchanged; I only rebased them onto master and resolved the conflict with #1909. ## The conflict, and how it is resolved Exactly the two places I named in the review on #1878: `OpenDB` and `Close()`. Both PRs rewrite them, and #1909 went first because it is the correctness fix. **`OpenDB`** — kept #1909's pinned-connection `detectSchema` and added this PR's `prepareStatements()` after it: ```go derr := d.detectSchema(ctx, sc) _ = sc.Close() if derr != nil { conn.Close(); return nil, fmt.Errorf("schema detection failed: %w", derr) } // Statements are prepared after schema detection so they can never be // compiled against a schema mode that turned out to be wrong (#1901). if err := d.prepareStatements(); err != nil { ... } ``` The ordering matters and is not arbitrary: preparing before detection would compile statements against a schema mode that #1909 exists to stop trusting. **`Close()`** — kept this PR's statement closing and **did not** restore the WAL checkpoint. #1909 removed it deliberately: the handle is `mode=ro`, so `PRAGMA wal_checkpoint(TRUNCATE)` can only ever fail with "disk I/O error (778)" and was emitting a misleading storage-fault line on every shutdown. That reasoning survives; the statement closing is added in front of it. ## Verification - Both commits cherry-picked onto `e5595ad9` - `cmd/server` builds - **Full `cmd/server` suite: ok, 0 failures** (not just the targeted DB tests — after master briefly went red today from a two-PR interaction, a full local run seemed worth the two minutes) ## Review points still open, none blocking From my review on #1878, unchanged by the rebase: 1. Every SQL string now exists twice, once prepared and once as the `stmtQueryRow` fallback literal, with nothing keeping them in sync. The fallback is genuinely needed — twelve test helpers build `&DB{conn: ...}` directly and never call `prepareStatements` — but a constructor for those helpers would remove the duplication. 2. `stmtCountObsLastHour` and `stmtCountObsLastDay` are byte-identical SQL. 3. `OpenDB` now refuses to start rather than degrading when a Prepare fails. Contained today, since none of the 13 prepared queries touch a schema-conditional column, but the failure mode changed. @Joel-Claw — your work, your credit. Ping me if you would rather take it back. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01Wzwr3eXseyNM7Xj598djjE --------- Co-authored-by: Joel Claw <358739783+Joel-Claw@users.noreply.github.com>
|
Merged via #1934, which is your two commits rebased onto master with your authorship intact. Closing this as the vehicle rather than the work. The conflict resolution, for the record: The three review points from my earlier comment are unchanged and none of them blocked: the SQL duplicated between Thanks for the quick reply on the rebase offer, and for turning around the |
… (#1936) Continues #1887 by @Jonher937. The commit is theirs, authorship unchanged; I only rebased it onto master. It went CONFLICTING because #1934 (prepared statements, originally @Joel-Claw's #1878) landed in the same `DB` struct. Both PRs add fields there and this one also replaces the single-slot channels cache. Resolution: kept this PR's keyed caches (`channelsCache`, `encChannelsCache`, `msgCache` plus their entry types and TTL constants) and kept master's thirteen prepared-statement fields alongside them. The old single-slot `channelsCacheKey`/`channelsCacheRes`/`channelsCacheExp` trio is gone, which is the point of this PR. Nothing else touched. Verified: `cmd/server` builds and the **full suite passes**, not just the channel tests. My review stands: approve, with two questions that do not block and are worth a look at some point. 1. `msgCache` is keyed by `hash|limit|offset|region`, and `offset` grows without bound as someone pages through a channel. Each entry also holds a full page of message maps, so a full 256-entry cache at `limit=50` holds around 12,800 maps. The other two caches are keyed by region only and genuinely low-cardinality as your comment says; this one is the odd one out. 2. `getMsgCache` returns the cached slice directly, so every hit hands the caller the same message maps. If any handler mutates one before serialising, it corrupts the cache for the next ten seconds. Same class as the finding on #1871, which was fixed there by copying at the two broadcast sites. Co-authored-by: Jonathan Herlin <jonte@jherlin.se>
2 participants
Problem
The server makes 72+ raw
db.conn.QueryRow/db.conn.Querycalls. Every call requires SQLite to parse and compile the SQL from scratch. The ingestor already uses 10 prepared statements (stmtGetTxByHash,stmtInsertTransmission, etc.) but the server layer never adopted the pattern.Fix
Add 13 prepared statements to the
DBstruct, prepared once atOpenDBtime and reused across all requests:COUNT(*) FROM transmissionsCOUNT(*) FROM observationsCOUNT(*) FROM nodes WHERE last_seen > ?COUNT(*) FROM nodesCOUNT(*) FROM observers WHERE inactive IS NULL OR inactive = 0COUNT(*) FROM observations WHERE timestamp > ?SELECT public_key FROM nodes WHERE public_key = ? OR name = ?SELECT id FROM transmissions WHERE hash = ?COUNT(*) FROM nodes WHERE role = ? AND last_seen > ?COUNT(*) FROM nodes WHERE role = ?COALESCE(MAX(id), 0) FROM transmissionsCOALESCE(MAX(id), 0) FROM observationsImpact
GetStatsis called on every/api/statsrequest — 8 queries, now all preparedGetRoleCounts+GetAllRoleCounts— 8 queries (4 roles x 2 functions), now all preparedGetMaxTransmissionID+GetMaxObservationID— polled by WebSocket clients for new-data detectionresolveNodePubkey— called per-request for node name resolutionGetObservationsForHash— called on packet detail lookupsThese are the hottest query paths. The remaining ~50 raw calls are dynamic query builders (optional WHERE clauses, GROUP BY, etc.) that are harder to prepare and less frequently called.
Testing
go buildpassesgo vetpassesCloses #1875