Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
e8c5f44
draft(userspec): create user-spec for pg-stat-statements-jit
Jun 22, 2026
71ff182
chore(userspec): validation round 1 — move impl detail to tech-spec, …
Jun 22, 2026
74d4e8f
chore(userspec): approve user-spec for pg-stat-statements-jit
Jun 22, 2026
26cd2f0
draft(techspec): create tech-spec for pg-stat-statements-jit
Jun 22, 2026
44b42d5
chore(techspec): validation round 1 — document duration-sort invarian…
Jun 22, 2026
27c9750
chore(techspec): approve tech-spec for pg-stat-statements-jit
Jun 22, 2026
34162dd
draft(tasks): create 4 tasks from tech-spec for pg-stat-statements-jit
Jun 22, 2026
13ba749
chore(tasks): task decomposition approved for pg-stat-statements-jit
Jun 22, 2026
79514ea
feat: task 01 — pg_stat_statements JIT query consts + version selector
Jun 22, 2026
8631ffd
chore: complete wave 1 — task 01 done, decisions + review reports
Jun 22, 2026
f7defde
feat: task 02 — register statements_jit view + Configure + count-test…
Jun 22, 2026
304ee62
fix: address review round 1 for task 02
Jun 22, 2026
5028374
docs: task 02 review reports (code/security/test reviewers)
Jun 22, 2026
718ceb4
chore: complete wave 2 — task 02 done, decisions + review reports
Jun 22, 2026
1d8d678
feat: task 03 — pg_stat_statements JIT menu item + x-cycle wiring
Jun 22, 2026
217b69d
fix: address review round 1 for task 03
Jun 22, 2026
e8dcf17
docs: task 03 review reports (code-reviewer approved, test-reviewer p…
Jun 22, 2026
0c69a2b
chore: complete wave 3 — task 03 done, decisions + review reports
Jun 22, 2026
92f151b
chore: complete wave 4 — task 04 pre-deploy QA done, manual TUI verified
Jun 22, 2026
22c13eb
docs: finalize 007 — update PK, features catalog, ADR log, roadmap, m…
Jun 22, 2026
04f43a3
docs: archive 007-feat-pg-stat-statements-jit feature directory
Jun 22, 2026
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .claude/skills/project-knowledge/overview.md
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ It reads PostgreSQL internal statistics views and presents them in a top-like in
- `pg_replication_slots` (+ `pg_stat_replication_slots`) — replication slots screen (hotkey `o`; PG 14–18; multi-row, all slots; retained WAL + wal_status + spill/stream; TUI-only, not recordable in 0.11.0)
- `pg_stat_io` — unified IO breakdown by backend_type × object × context (hotkey `j` toggles count↔time sub-screens, `J` opens the mode menu; PG 16+; multi-row; this is where `buffers_backend`/`buffers_backend_fsync` went on PG 17+ and WAL IO timings on PG 18; TUI-only, not recordable in 0.11.0)
- `pg_stat_wal` — WAL generation stats (PG 14+; reduced schema in PG 18 — WAL IO timings moved to `pg_stat_io`)
- `pg_stat_statements` — top queries by various metrics (requires extension)
- `pg_stat_statements` — top queries by various metrics (requires extension); 7 sub-screens under the `X` menu / `x` cycle: timings, general, IO, temp files, local (temp tables), WAL, and **JIT** (compilation cost per query — generation/inlining/optimization/emission phase times + functions, `+deform` on PG 17+; PG 15+; rows filtered to `jit_functions > 0`; TUI-only, not recordable in 0.11.0)
- System stats — CPU, memory, disk, network (read from /proc or via PL/Perl schema)

## Target Audience
Expand Down
15 changes: 12 additions & 3 deletions .claude/skills/project-knowledge/patterns.md
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,13 @@ When a PG version changes columns in a stats view:
- Call it in `view.Configure()` under the correct view name
- Add version-specific test cases in `*_test.go`

When versions differ by **column count** (not just names), the selector must also carry the
layout: return `(string, int, [2]int)` (`+DiffIntvl`, see `io.go`) or `(string, int, [2]int, int)`
(`+UniqueKey`, see the `statements_jit` selector in `internal/query/statements.go` — UniqueKey
points at the trailing md5 `queryid`, whose index shifts with ncols) and patch all returned
fields in `Configure()`. When only names change but the count is constant (e.g. `statements_timings`),
`Configure()` swaps `QueryTmpl` alone and the static `Ncols`/`DiffIntvl`/`UniqueKey` stay valid.

Reference implementations of the single-row version-aware view: `internal/query/wal.go` and `internal/query/bgwriter.go`. The bgwriter screen is notable for placing absolute event-counter columns (`ckpt_*`, `rstpt_*`) **outside** the contiguous `DiffIntvl` range so they render cumulative, while the work/time/buffer columns inside the range render as per-interval deltas.

For a **multi-row hybrid view** that LEFT JOINs two stats views, see `internal/query/replication_slots.go` (the `replslots` screen). Two patterns it establishes:
Expand All @@ -28,9 +35,11 @@ When the row identity is **composite** (more than one column), emit a synthetic

## Adding a New View — test counts that must be updated

Registering a view in `view.New()` couples to two count-based tests that fail in CI (not always locally) if missed:
- `internal/view/view_test.go: TestNew` pins the total view count (and `TestView_VersionOK` pins per-version availability).
- `record/record_test.go: Test_filterViews` pins, per version, how many views `filterViews` drops vs keeps. A `NotRecordable: true` view is always dropped, so every `wantN` row increases by the number of new `NotRecordable` views (feature 006 added 2 → `+2` each row; `wantV` unchanged). This test runs without Postgres, so a stale count is a real failure even though the rest of the `record` package skips/fails on a missing PG fixture — do not assume a red `record` package is only the connection-refused tests.
Registering a view in `view.New()` couples to count-based tests that fail in CI (not always locally) if missed:
- `internal/view/view_test.go: TestNew` pins the total view count. `TestView_VersionOK` pins per-version availability — its row at a version **≥ the new view's `MinRequiredVersion`** also increases by one (feature 007's PG15+ view bumped only the `160000` row, not the `≤140000` rows).
- `record/record_test.go: Test_filterViews` pins, per version, how many views `filterViews` drops vs keeps. A `NotRecordable: true` view is always dropped, so every `wantN` row increases by the number of new `NotRecordable` views (feature 006 added 2 → `+2` each row; feature 007 added 1 → `+1`; `wantV` unchanged). This test runs without Postgres, so a stale count is a real failure even though the rest of the `record` package skips/fails on a missing PG fixture — do not assume a red `record` package is only the connection-refused tests.

Adding a `pg_stat_statements` **sub-screen** (or any `menuPgss`/cycle entry) additionally breaks `top` tests — `Test_selectMenuStyle` (pins each menu's item count), `Test_statementsNextView`, and `Test_switchViewTo` (pin the `x`-cycle transitions). These `top` tests DO run locally without Postgres, so they catch the miss in `make test` — but feature 007's code-research overlooked them (the task wrongly assumed the TUI layer had no tests). When touching `top/menu.go` or `top/config_view.go`, grep `top/*_test.go` for the function you changed before assuming it is untested.

## Error Wrapping

Expand Down
32 changes: 32 additions & 0 deletions docs/decisions-log.md
Original file line number Diff line number Diff line change
Expand Up @@ -391,3 +391,35 @@ Used by tech-spec planning and code research to avoid repeating mistakes and re-
**Rationale:** Follows the [004] per-version-branch ADR (not NULL-padded). Identical headers keep the DBA's table stable across versions. `coalesce` is mandatory — `pg_stat_io` returns NULL pervasively (e.g. `fsyncs` for `temp relation`), and a NULL inside `DiffIntvl` aborts the whole sample ([005] lesson).

**Alternatives considered:** Single NULL-padded query across versions (rejected by [004]); a separate PG 17 branch (unnecessary — 16==17).

## [007-feat-pg-stat-statements-jit] JIT query selector returns a 4-tuple (query, Ncols, DiffIntvl, UniqueKey)

**Date:** 2026-06-22
**Feature:** 007-feat-pg-stat-statements-jit
**Status:** Accepted

**Context:** The JIT sub-screen's column COUNT differs across versions (PG 15/16 → 13 columns; PG 17+ → 15, adding `deform_total`/`deform,ms`). Because `UniqueKey` points at the trailing synthetic md5 `queryid`, its index shifts with the column count, and `DiffIntvl` shifts too. The existing `statements_timings` selector returns only a query string (its count is constant at 13 across versions), and the `pg_stat_io` selector returns `(string, int, [2]int)` (its `UniqueKey` is a fixed col 0).

**Decision:** `SelectStatStatementsJITQuery(version) (string, int, [2]int, int)` returns query + `Ncols` + `DiffIntvl` + `UniqueKey`; `view.Configure()` patches all four. Two-way branch: `>= PostgresV17` → (Default, 15, {7,12}, 13); else → (PG15, 13, {6,10}, 11). Gate `MinRequiredVersion: query.PostgresV15`.

**Rationale:** A trailing-key layout whose index moves with version forces the selector to carry the full layout — returning only the query (timings model) would leave stale `Ncols`/`DiffIntvl`/`UniqueKey`; returning a 3-tuple (io model) would leave a stale `UniqueKey`. Extending to a 4-tuple keeps the layout invariant explicit and unit-testable, and column hiding remains unavailable (`internal/align` floors width at 8, ADR [006]).

**Alternatives considered:** Compute `UniqueKey = Ncols-2` in `Configure()` (works, but hides a layout invariant in arithmetic). Hide `deform` columns on PG 15 (impossible per ADR [006]). Both rejected.

---

## [007-feat-pg-stat-statements-jit] JIT column layout: timings-style total+interval, drop the *_count phase counters

**Date:** 2026-06-22
**Feature:** 007-feat-pg-stat-statements-jit
**Status:** Accepted

**Context:** `pg_stat_statements` exposes 8 JIT metrics on PG 15/16 (4 counts + 4 times) and 10 on PG 17+. Mirroring `statements_io`'s total+interval doubling across all of them would be ~22–26 columns; pgcenter has no horizontal column scroll (ADR [006]).

**Decision:** Model the screen on `statements_timings`: cumulative `*_total` text durations + per-interval `*,ms` columns for the four (PG 17+: five) phase TIMES, plus a single `functions` counter. Drop the `inlining_count`/`optimization_count`/`emission_count` (and `deform_count`) metrics. Filter rows to `WHERE jit_functions > 0` (pg_stat_io zero-row precedent), surfacing the `jit=off` empty state via the view `Msg`.

**Rationale:** The per-phase TIME is the cost signal a DBA acts on; the phase counts are secondary and `jit_functions` already represents "how much JIT work". Dropping the counts keeps the screen at 13/15 columns — within terminal width. The `Msg`-as-hint reuses the existing `track_io_timing` note mechanism (no new GUC-detection code).

**Alternatives considered:** All counts+times with total+interval doubling (too wide). Splitting into count/time sub-screens like `pg_stat_io` (out of scope — JIT is the low-risk release closer, and the time-only set fits one screen). Both rejected.

---
20 changes: 20 additions & 0 deletions docs/features-catalog.md
Original file line number Diff line number Diff line change
Expand Up @@ -122,3 +122,23 @@ Used by spec-writer to understand existing functionality and avoid duplication o
- `Q` does not reset `pg_stat_io` (it is shared/cluster-wide stats) — noted in the help screen.

**Touches:** Shares the multi-row view model with `replslots`/`tables`; third + fourth `NotRecordable` views. Closes the visibility gaps left by [004-feat-bgwriter-checkpointer] (`buffers_backend` on PG 17+) and the `pg_stat_wal` screen (WAL IO timings on PG 18).

### [007-feat-pg-stat-statements-jit] pg_stat_statements JIT Screen

**What it does:** Adds a 7th `pg_stat_statements` sub-screen — **JIT** — under the `X` menu (and the `x` cycle), showing per-statement JIT compilation cost. Lets a DBA find which normalized queries pay heavy JIT generation/inlining/optimization/emission time (the classic cause of "mysterious" latency on short queries when `jit=on`) without dropping to `psql`.

**Key scenarios:**
- Press `X` → choose `pg_stat_statements JIT compilation` (or press `x` to cycle `… wal → jit → timings …`). The screen opens sorted by `gen_total` (cumulative generation time) descending — the heaviest JIT compilers on top.
- Columns: `user`, `database`, cumulative phase totals `gen_total`/`inline_total`/`opt_total`/`emit_total` (`+deform_total` on PG 17+), per-interval `gen,ms`/`inline,ms`/`opt,ms`/`emit,ms` (`+deform,ms` on PG 17+), `functions` (JIT-compiled functions this interval), `queryid`, `query`.
- Decide whether JIT pays off: a query with large phase times but few rows is a candidate for raising `jit_above_cost`/`jit_optimize_above_cost` or turning JIT off for the workload.
- Re-sort (arrows — `*_total` text durations sort numerically) and filter (`/`) like the other pgss sub-screens.

**Limitations:**
- TUI-only in 0.11.0 — the view is `NotRecordable`, so `pgcenter record`/`report` skip it. record/report is the planned next phase.
- PG 15+ only (JIT columns appeared in PG 15; `deform_*` in PG 17). On PG < 15 the sub-screen reports "not supported" via the runtime version guard.
- Rows with no JIT activity (`jit_functions = 0`) are hidden in SQL; under `jit=off` the screen is empty and the command line shows a hint.
- The `*_count` phase counters (inlining/optimization/emission) are omitted to fit the screen width (no horizontal scroll) — only `functions` is shown; the value is in the phase times.

**Touches:** Shares the pgss sub-screen model (synthetic md5 `queryid` `UniqueKey`, total+interval columns) with `statements_timings`/`statements_io`; fifth `NotRecordable` view added in 0.11.0 (after bgwriter, replslots, stat_io ×2). Closes release 0.11.0.

---
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
{
"status": "approved",
"findings": [
{
"category": "overengineering",
"severity": "major",
"issue": "The user-spec embeds tech-spec-level implementation detail: exact column-index tables with idx/DiffIntvl/Ncols/UniqueKey/OrderKey values (lines 82-104), the internal selector function name and signature SelectStatStatementsJITQuery(version) (lines 151, 185-187), specific source file paths (internal/stat/postgres.go, statements_io reference), and the precise test-count edits view_test.go::TestNew (26->27) and record/record_test.go::Test_filterViews +1 (lines 153, 197).",
"why_matters": "User-spec defines WHAT and WHY; HOW (column indices, function names, file paths, Ncols/DiffIntvl values, test-count deltas) belongs in tech-spec. Pinning indices in the user-spec couples the requirement to one implementation, and if the tech-spec chooses a slightly different layout the acceptance criteria become wrong/contradictory. It also blurs the review boundary — these numbers should be derived and validated during tech-spec, not pre-committed here.",
"fix": "Move the index/Ncols/DiffIntvl/UniqueKey tables, the selector name+signature, file paths, and the exact test-count edits into the tech-spec (they are already captured in code-research.md). In the user-spec keep behavioural acceptance criteria: which columns appear per PG version (by meaning, not index), default sort by generation time desc, jit_functions=0 rows hidden, PG<15 unavailable, NotRecordable, hint on empty. Restate AC lines 151/153 as behaviour, not as code edits."
},
{
"category": "underengineering",
"severity": "minor",
"issue": "Edge cases are listed for the main flows (jit=off / empty set, PG<15 degradation, pg_stat_statements absent, sort/filter), but the duration-aware numeric sort of *_total text columns is mentioned only as an assumption (line 59-60) rather than as an explicit acceptance criterion, and there is no edge case for very large cumulative times (3+ digit hours / 'N days ...') beyond a parenthetical note.",
"why_matters": "The *_total columns are text durations; sorting them numerically (HH:MM:SS with 3+ digit hours and 'N days') is a known sharp edge in internal/stat/postgres.go. Without an explicit AC, a regression there would pass all listed criteria.",
"fix": "Add an acceptance criterion: sorting by a *_total column orders rows numerically by duration (correct for 100+ hour and multi-day values), not lexically. This is behaviour, so it belongs in the user-spec."
}
],
"worst_category": "overengineering",
"summary": "The idea is sound, feasible, and correctly right-sized as S: a 7th pgss sub-screen built strictly by analogy with established, version-aware NotRecordable views (statements_timings / statements_io / stat_io / replslots) using only the existing stack (pgx/v5, gocui, version selectors). No new dependencies, infrastructure, or architectural conflicts; the chosen single-interval-block layout and WHERE jit_functions>0 filter are the simplest viable approach and match prior ADRs. Approved with no blocking concerns — main improvement is pulling implementation detail (column indices, selector signature, file paths, test-count edits) out of the user-spec into the tech-spec."
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
{
"status": "approved",
"checks": {
"architectural_fit": "pass",
"scalability": "pass",
"database": "n/a",
"coupling": "pass",
"api_contracts": "n/a",
"backward_compat": "pass"
},
"findings": [
{
"check": "architectural_fit",
"severity": "minor",
"issue": "The tech-spec states 'OrderKey: 2, gen_total, desc' for the initial sort, but col 2 is a cumulative text interval (date_trunc output, e.g. '00:01:23'), not a numeric or diffed column. The sort engine in internal/stat/postgres.go:sort() auto-detects by probing the first row's value: if parseDuration succeeds it uses duration sort, otherwise falls back to string sort. Under an empty screen (WHERE jit_functions > 0 with no JIT activity) there are zero rows, so sort is a no-op. On a live screen the value '00:00:00' does parse as a duration via parseDuration(), so sorting is numerically correct — but only because the format matches HH:MM:SS. If a statement accumulates more than 24 hours of JIT time the format becomes 'N days HH:MM:SS', which parseDuration also handles. This works correctly and is consistent with the existing statements_timings precedent (OrderKey 0, user, which falls through to string sort). The minor concern is that the tech-spec does not explicitly document that OrderKey 2 relies on the duration-sort branch of stat.sort(), making the reasoning opaque for future contributors.",
"location": "Tech-spec section 'Decisions / Decision 2: Column layout', acceptance criteria OrderKey:2",
"fix": "Add a comment in the view entry or tech-spec noting that gen_total is formatted as HH:MM:SS text, which triggers the duration-sort branch in internal/stat/postgres.go:sort(). No code change required."
},
{
"check": "architectural_fit",
"severity": "minor",
"issue": "The code-research file (section 5c) flags a risk: 'verify the top view-availability filter removes statements_jit on PG<15 so that viewSwitchHandler on PG14 does not yield a zero-value View{}'. The tech-spec names 'existing VersionOK runtime guard (stat.go:199)' as the mitigation, but does not explicitly resolve this concern. Looking at the codebase: view.New() registers statements_jit unconditionally; config.views is built from view.New() in top/top.go. The VersionOK check lives in the collector path (internal/stat), not in viewSwitchHandler or statementsNextView. This means pressing 'x' on PG14 would call viewSwitchHandler(config, 'statements_jit'), which reads config.views['statements_jit'] — a valid View entry (MinRequiredVersion is set but the view IS in the map). The collector then calls v.VersionOK(version) and returns the 'not supported' message. This is the same behavior as statements_wal on PG12: the view is in the map, but the collector guards it. So the existing pattern handles it correctly. However the tech-spec does not confirm this analysis explicitly; it should note that this is the same graceful-degradation path as statements_wal on PG<13.",
"location": "Tech-spec section 'How it works', step 3 — VersionOK guard",
"fix": "Add one sentence to the tech-spec confirming that the PG<15 degradation path is identical to statements_wal on PG<13: the view is in the map, viewSwitchHandler succeeds, and the collector's VersionOK guard returns the 'not supported' message. No code change required."
},
{
"check": "backward_compat",
"severity": "minor",
"issue": "The tech-spec marks the statementsNextView cycle change as purely additive ('wal → jit → timings') and calls backward compatibility N/A. This is correct for the record/report path. However, the x-key cycle order is a user-observable behavior change for anyone who relies on the existing 'wal → timings' cycle. The spec describes this as intended, and the change is safe because no external API contract is affected. The mention of it here is informational only — the cycle insertion is the right approach.",
"location": "Tech-spec section 'Backward Compatibility'",
"fix": "Optionally note in the Backward Compatibility section that the x-cycle order changes from 'wal → timings' to 'wal → jit → timings', which is a deliberate UX change, not a breakage."
}
],
"risk_assessment": {
"performance": "low",
"complexity": "low",
"tech_debt": "low"
},
"summary": "The spec is architecturally sound and fits the existing codebase precisely. The selector-returns-(query,Ncols,DiffIntvl,UniqueKey) design correctly follows the SelectStatIOQuery model (not the timings model) because Ncols changes across PG versions; the NotRecordable+MinRequiredVersion+synthetic-md5-UniqueKey combination matches the stat_io/stat_io_time precedent exactly; and the column index arithmetic for both the PG15 (13-col, DiffIntvl{6,10}, UniqueKey 11) and PG17 (15-col, DiffIntvl{7,12}, UniqueKey 13) layouts is internally consistent and correct. Three minor findings — all documentation gaps rather than correctness defects — do not block implementation."
}
Loading
Loading