From 9c507448c7cf27e7f30dcfe8ecba6b5e6e37dda0 Mon Sep 17 00:00:00 2001 From: Daedalus Date: Fri, 14 Aug 2026 18:39:06 +0200 Subject: [PATCH 1/2] docs(decisions): ADR-0013 spans carries no derived columns The VIRTUAL generated duration_ms column is the root cause of the DuckDB filter-pushdown wrong-results trap: it takes a logical slot but no storage slot, so later columns collide with the physical index of an indexed column and a bare `col = ` silently returns zero rows. Decides to drop the column outright and compute the duration at the four query sites that read it, rather than restating it as a stored column. The migration then moves no data, stays a single idempotent DROP that composes with the ADR-0010 re-apply guard, and leaves start_time/end_time as the one source of truth. The export CSV is unaffected - writeSpansCSV already derives duration in Go and the export query never selects the column. Co-Authored-By: Daedalus Co-Authored-By: Claude Opus 5 --- .../0013-spans-has-no-derived-columns.md | 129 ++++++++++++++++++ docs/decisions/index.md | 1 + 2 files changed, 130 insertions(+) create mode 100644 docs/decisions/0013-spans-has-no-derived-columns.md diff --git a/docs/decisions/0013-spans-has-no-derived-columns.md b/docs/decisions/0013-spans-has-no-derived-columns.md new file mode 100644 index 0000000..de9130b --- /dev/null +++ b/docs/decisions/0013-spans-has-no-derived-columns.md @@ -0,0 +1,129 @@ +# ADR 0013 — `spans` carries no derived columns: drop `duration_ms` + +**Date:** 2026-08-14 +**Status:** Accepted +**Deciders:** Daedalus (CTO); diagnosis by Wayland + +--- + +## Context + +`spans.duration_ms` is declared as a VIRTUAL generated column in the middle of +the table: + +```sql +start_time TIMESTAMPTZ NOT NULL, +end_time TIMESTAMPTZ NOT NULL, +duration_ms DOUBLE GENERATED ALWAYS AS ( + epoch_ms(end_time) - epoch_ms(start_time) + ), +service_name VARCHAR, +... +``` + +A virtual column takes a logical slot but no storage slot, so every column +declared after it has logical index = physical index + 1. A column is then +answered wrongly by a bare `WHERE col = ` exactly when its logical +index collides with the *physical* index of an indexed column: the scan probes +that unrelated ART index for the constant, finds nothing, and returns zero rows. +Not an error — an empty answer. + +Today `service_name` (logical 7) collides with `session_id` (physical 7) and +`tool_name` (logical 10) with `user_id` (physical 10). That is how +`/api/v1/bash-commands` shipped permanently empty behind a `WHERE tool_name = +'Bash'` that reads correctly to any reviewer. + +The trap is our table shape, not an engine version. It reproduces on DuckDB +1.1.3, 1.4.1 and 1.5.5, and 1.4+ additionally pushes `COALESCE` into the scan, +which invalidates the `COALESCE(col, '') = ?` workaround the code currently +relies on. The collision set also moves silently whenever anyone adds, reorders +or indexes a column in `spans`. + +The guard test landed alongside this analysis +(`internal/storage/pushdown_test.go`) derives the affected set from the live +schema and fails in both directions, so a moved collision turns into a red +build. It detects the trap; it does not remove it. + +## Options considered + +**A. Do nothing; keep the guard test and the `COALESCE` workaround.** +Survivable only while nobody changes `spans`' columns or indexes and we never +upgrade the engine — and 1.4+ already pushes `COALESCE` down, so the workaround +is on borrowed time. Rejected: every future query against a VARCHAR column is a +correctness landmine that reads fine in review. + +**B. Reorder the generated column to last in `CREATE TABLE`.** +Fixes fresh databases only. DuckDB rejects `ALTER TABLE … ADD COLUMN … GENERATED +ALWAYS AS` (*"Adding generated columns after table creation is not supported +yet"*), so an existing database cannot be reordered in place without a full +table rewrite. `STORED` is rejected outright on every engine tested. Rejected. + +**C. Make `duration_ms` a plain stored `DOUBLE` appended last**, backfilled once +and populated by `InsertSpan`. Every query site stays byte-identical. Rejected — +see below. + +**D. Drop `duration_ms` from `spans` entirely and compute the duration at the +four query sites that need it.** Chosen. + +## Decision + +Drop `duration_ms` from `spans`. Compute +`epoch_ms(end_time) - epoch_ms(start_time)` in the queries that need it, aliased +back to `duration_ms` so response shapes and sort keys do not move. + +**Why D over C.** Option C is the smaller diff by line count, but it buys that +with a worse migration and a new invariant to maintain: + +- **The migration moves no data.** D is one idempotent statement — `ALTER TABLE + spans DROP COLUMN IF EXISTS duration_ms`, a no-op once applied. C needs + DROP + ADD + a full-table backfill `UPDATE`. In this project a merge to `main` + is a production deploy, onto a database whose cold start is already the + sensitive path, so a migration with nothing to interrupt is worth more than a + few saved lines. +- **C composes badly with [ADR-0010](./0010-schema-version-guard).** That guard + re-applies the whole of `schema.sql` on any change to the file, including a + comment-only one — deliberately, to fail toward doing too much. A DROP + ADD + + backfill sitting in `schema.sql` is therefore not a one-time cost: it drops the + real column and rewrites the whole table on *every future schema edit*. + Avoiding that means a bespoke one-shot guard in Go, i.e. new machinery around a + column we do not need to store. D's single `DROP … IF EXISTS` is naturally + idempotent and matches the migration style already in the file. +- **A stored column can drift; a computed one cannot.** C makes `duration_ms` + writable, so it becomes possible for it to disagree with `start_time` / + `end_time`. D keeps one source of truth. +- **The public interface does not read the column anyway.** The export CSV is a + versioned interface ([ADR-0005](./0005-export-import-format)) and carries + `duration_ms` at a fixed position — but `writeSpansCSV` already derives it in + Go (`s.EndTime.Sub(s.StartTime)`) and the export query never selects the + column. The precedent for deriving duration outside storage is already in the + codebase, on the one path where the format is frozen. + +The cost of D is four SQL call sites instead of zero: the session spans +projection and the tools and bash-commands aggregates in `internal/api`, and the +roll-up sum in `internal/storage/retention.go`. That is a known, bounded, one-off +edit, against a landmine that is neither. + +**The rule this establishes:** `spans` carries no derived columns. Anything +computable from other columns is computed where it is read, or — where it must +be persisted for retention, as in `daily_usage.total_duration_ms` — stored in an +aggregate table, never as a generated column beside the raw data. + +## Consequences + +- `WHERE col = ` becomes correct on `spans` again. The `COALESCE` + workaround in `handleBashCommands` and the `pushdownBrokenSpanCols` list in the + guard test both go away; the guard test itself stays, as the detector for any + future reintroduction. +- Schema version bumps 9 → 10 under the ADR-0010 guard. +- No backfill, no table rewrite, no data movement on the deploy that introduces + it. The dropped values are recomputed on read from columns that are `NOT NULL`, + so nothing is lost and the change needs no reverse migration — re-adding the + column later is a plain additive migration. +- The export ZIP/CSV format is unchanged: same columns, same positions, same + values, `format_version` untouched. +- Reads that previously projected a stored value now evaluate two `epoch_ms` + calls per row on timestamps the scan has already loaded. Negligible against the + scan itself, and the aggregate paths (`tools`, `bash-commands`) read the same + rows either way. +- Anyone adding a convenience column to `spans` must not reach for `GENERATED + ALWAYS AS`. The guard test enforces the outcome; this ADR records the reason. diff --git a/docs/decisions/index.md b/docs/decisions/index.md index fba4271..9b4dd07 100644 --- a/docs/decisions/index.md +++ b/docs/decisions/index.md @@ -20,3 +20,4 @@ New ADRs go in this directory as `NNNN-short-title.md`, numbered sequentially. | [ADR-0010](./0010-schema-version-guard) | Guard schema migrations behind a recorded version | Accepted | | [ADR-0011](./0011-users-list-ranged-stats-and-server-side-sort) | Users list — time-ranged stats, server-side sort and pagination | Accepted | | [ADR-0012](./0012-tools-list-ranged-stats-and-server-side-sort) | Tools list — time-ranged stats, server-side sort and pagination | Accepted | +| [ADR-0013](./0013-spans-has-no-derived-columns) | `spans` carries no derived columns: drop `duration_ms` | Accepted | From 8487030fe36044642e5ad340807ac48fe3af2695 Mon Sep 17 00:00:00 2001 From: Daedalus Date: Fri, 14 Aug 2026 19:04:53 +0200 Subject: [PATCH 2/2] docs(decisions): correct ADR-0013 on the migration shape and downgrade path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The "why D over C" comparison claimed the migration was a single idempotent ALTER. It is not: DuckDB refuses to ALTER a table an index depends on, so the four secondary indexes are dropped and rebuilt around the column drop, and the ADR-0010 guard re-pays that 138 ms on every future schema edit rather than once. The conclusion is unchanged — an index rebuild is not a full-table rewrite — but the stated reason was wrong, and the decisive bullet of a decision record has to survive being checked. Also records that there is no downgrade path: an older binary opens a v10 database but errors on every query naming duration_ms. Co-Authored-By: Daedalus Co-Authored-By: Claude Opus 4.8 --- .../0013-spans-has-no-derived-columns.md | 41 +++++++++++-------- 1 file changed, 25 insertions(+), 16 deletions(-) diff --git a/docs/decisions/0013-spans-has-no-derived-columns.md b/docs/decisions/0013-spans-has-no-derived-columns.md index de9130b..f5ec558 100644 --- a/docs/decisions/0013-spans-has-no-derived-columns.md +++ b/docs/decisions/0013-spans-has-no-derived-columns.md @@ -74,20 +74,25 @@ back to `duration_ms` so response shapes and sort keys do not move. **Why D over C.** Option C is the smaller diff by line count, but it buys that with a worse migration and a new invariant to maintain: -- **The migration moves no data.** D is one idempotent statement — `ALTER TABLE - spans DROP COLUMN IF EXISTS duration_ms`, a no-op once applied. C needs - DROP + ADD + a full-table backfill `UPDATE`. In this project a merge to `main` - is a production deploy, onto a database whose cold start is already the - sensitive path, so a migration with nothing to interrupt is worth more than a - few saved lines. +- **The migration moves no row data.** D drops a column; C needs DROP + ADD plus + a full-table backfill `UPDATE`. In this project a merge to `main` is a + production deploy, onto a database whose cold start is already the sensitive + path, so a migration with no rows to rewrite is worth more than a few saved + lines. D is not the single statement it first looks like: DuckDB refuses to + `ALTER` a table an index depends on, so the migration drops the four secondary + indexes first and lets the existing `CREATE INDEX IF NOT EXISTS` block rebuild + them. Measured at 138 ms on a 108 MB database of 34 706 spans; the whole v10 + upgrade added 0.4 s to a 9.5 s cold start already dominated by WAL replay. - **C composes badly with [ADR-0010](./0010-schema-version-guard).** That guard re-applies the whole of `schema.sql` on any change to the file, including a - comment-only one — deliberately, to fail toward doing too much. A DROP + ADD + - backfill sitting in `schema.sql` is therefore not a one-time cost: it drops the - real column and rewrites the whole table on *every future schema edit*. - Avoiding that means a bespoke one-shot guard in Go, i.e. new machinery around a - column we do not need to store. D's single `DROP … IF EXISTS` is naturally - idempotent and matches the migration style already in the file. + comment-only one — deliberately, to fail toward doing too much. Neither option + is a one-time cost under that guard, but they degrade very differently: C's + DROP + ADD + backfill drops the real column and rewrites the whole table on + *every future schema edit*, where D re-pays only the index rebuild above. + Holding C to a single application means a bespoke one-shot guard in Go, i.e. + new machinery around a column we do not need to store. D's statements are all + `IF EXISTS` / `IF NOT EXISTS` and match the migration style already in the + file. - **A stored column can drift; a computed one cannot.** C makes `duration_ms` writable, so it becomes possible for it to disagree with `start_time` / `end_time`. D keeps one source of truth. @@ -115,10 +120,14 @@ aggregate table, never as a generated column beside the raw data. guard test both go away; the guard test itself stays, as the detector for any future reintroduction. - Schema version bumps 9 → 10 under the ADR-0010 guard. -- No backfill, no table rewrite, no data movement on the deploy that introduces - it. The dropped values are recomputed on read from columns that are `NOT NULL`, - so nothing is lost and the change needs no reverse migration — re-adding the - column later is a plain additive migration. +- No backfill, no table rewrite, no row data moved on the deploy that introduces + it — only the four secondary indexes are dropped and rebuilt. The dropped + values are recomputed on read from columns that are `NOT NULL`, so nothing is + lost, and re-adding the column later would be a plain additive migration. +- **No downgrade path.** An older binary still opens a v10 database — its + `CREATE TABLE IF NOT EXISTS` cannot bring the column back — but every query + naming `duration_ms` then errors. Rolling the binary back means restoring the + pre-upgrade database with it. - The export ZIP/CSV format is unchanged: same columns, same positions, same values, `format_version` untouched. - Reads that previously projected a stored value now evaluate two `epoch_ms`