161 lines
8.1 KiB
Markdown
161 lines
8.1 KiB
Markdown
# M6.3 — store.go query port to Postgres
|
||
|
||
| Field | Value |
|
||
|---|---|
|
||
| Phase | M6 — agent-manager migration |
|
||
| Size | L — 3+ days |
|
||
| Status | ⬜ Not started |
|
||
| Flags | — |
|
||
| Spec | inlined below |
|
||
| Blocks | M6.2 |
|
||
|
||
## Goal
|
||
|
||
Every query in `internal/store/store.go` rewritten against the M6.2 schema,
|
||
sqlite dropped entirely (decided: full port, not a dual sqlite/Postgres
|
||
backend — this is single-machine usage, no standalone-without-cluster
|
||
requirement to preserve).
|
||
|
||
## Facts (inlined — no spec read needed)
|
||
|
||
Read directly from `internal/store/store.go` (1397 lines) on the
|
||
`add-headless-spawn` branch:
|
||
|
||
- **65 call sites** (`db.Exec`, `db.QueryRow`, `db.Query`, `tx.Exec`,
|
||
`tx.QueryRow`) use sqlite `?` positional placeholders. Postgres
|
||
(`pgx` or `lib/pq`) needs `$1, $2, ...` — mechanical but must be done
|
||
per-statement since arg count varies 1–16 across sites.
|
||
- **Upserts are already Postgres-compatible.** All 9 upsert sites
|
||
(`SetReviewRepo`, `SetReviewBase`, `SetReviewScope`, `SetSetting`,
|
||
`ensureGroup`, `CreateGroup`, `AddGroup`, `createSession`'s group
|
||
insert, `PlaceSession`'s group insert) already use `ON
|
||
CONFLICT(...) DO UPDATE SET ... = excluded....` / `ON CONFLICT(...) DO
|
||
NOTHING`, valid as-is in Postgres. No rewrite beyond the placeholder
|
||
swap — this corrects an earlier assumption that these were sqlite
|
||
`INSERT OR REPLACE` and needed real rework.
|
||
- `boolToInt()`/`archived != 0`-style conversions at ~8 sites go away —
|
||
scan `int archived`/`int acked`/`int pendingClaimed`/`int` locals in
|
||
`ListSessions`, `Get` and replace with `bool` directly.
|
||
- `encodeTime()`/`decodeTime()` calls at every session-timestamp
|
||
read/write go away — pass `time.Time` straight through; `pgx` handles
|
||
`TIMESTAMPTZ` natively. Delete `timeenc_test.go` along with the
|
||
functions it tests (M6.2 already noted its coverage folds into the
|
||
schema tests instead).
|
||
- `db.SetMaxOpenConns(1)` + `PRAGMA journal_mode=WAL`
|
||
(store.go:76-79) gave sqlite serialized writes for free. Some call
|
||
sites lean on that implicit serialization — most notably
|
||
`createSession`'s `sort_order = (SELECT COALESCE(MAX(sort_order)+1, 0)
|
||
FROM sessions WHERE group_name = ? AND parent_id = ?)` subquery, which
|
||
races under concurrent Postgres writers with a real connection pool.
|
||
Wrap it in `SELECT ... FOR UPDATE` inside the existing transaction (the
|
||
function already opens one) rather than trusting single-writer
|
||
semantics that no longer exist. Same pattern applies to
|
||
`ReorderSession`/`SwapSessionOrder`/`ReorderGroup`/`SwapGroupOrder`'s
|
||
read-then-renumber-then-write sequences.
|
||
- FK violations are new failure modes M6.2 introduced (group/parent FKs).
|
||
`createSession` and `PlaceSession` already validate `group_name`/
|
||
`parent_id` through `validParent`/`ensureGroup` before writing, so those
|
||
paths shouldn't hit a live constraint in practice — but wrap the actual
|
||
`INSERT`/`UPDATE` error and translate a `23503` (foreign_key_violation)
|
||
SQLSTATE into the same descriptive errors the pre-validation already
|
||
produces, so a race between the check and the write degrades to a clear
|
||
error instead of a raw driver error reaching the CLI.
|
||
- `driver: modernc.org/sqlite` import and the blank `_ "modernc.org/
|
||
sqlite"` in `Open()` are deleted; replaced with `pgx` (`github.com/
|
||
jackc/pgx/v5/stdlib` for `database/sql` compatibility, keeping the rest
|
||
of the file's `*sql.DB`-based code unchanged) or a native `pgx.Pool` —
|
||
pick `pgx/v5/stdlib` unless a later task needs pgx-native features
|
||
(e.g. `COPY`), since it's the smaller diff against the existing
|
||
`database/sql` code.
|
||
|
||
## Steps
|
||
|
||
1. Swap the driver import and `Open()`'s connection string handling
|
||
(sqlite file path -> Postgres DSN, likely from an env var or flag the
|
||
CLI already has a slot for — check `cmd/` for how `Open()` is called
|
||
today).
|
||
2. Delete `db.SetMaxOpenConns(1)` / `PRAGMA journal_mode=WAL`; size the
|
||
pool deliberately instead (`SetMaxOpenConns` to something sane for a
|
||
single-machine client, e.g. 5–10).
|
||
3. Delete `init()`'s `CREATE TABLE IF NOT EXISTS` + 14-migration list —
|
||
M6.2's migration owns schema creation now; `Open()` just connects and
|
||
optionally runs a `schema_version` sanity check.
|
||
4. Mechanically convert every `?` to `$N` across the 65 call sites, in
|
||
file order, verifying arg count against placeholder count each time
|
||
(this is where an off-by-one is easiest to introduce silently).
|
||
5. Remove `boolToInt`/`archived != 0` conversions; scan struct fields
|
||
directly as `bool`.
|
||
6. Remove `encodeTime`/`decodeTime`/`secondsCeiling`; pass `time.Time`
|
||
directly. Delete `timeenc_test.go`.
|
||
7. Add `SELECT ... FOR UPDATE` (or equivalent explicit locking) to the
|
||
sort-order read-then-write sequences named above.
|
||
8. Add FK-violation (`23503`) error translation at the write sites that
|
||
can theoretically race past their own pre-validation.
|
||
9. Run agent-manager's full existing test suite against a real Postgres
|
||
(M6.1's cluster, or local `postgres:16` for iteration) — every current
|
||
test should still pass unmodified in intent, only in backing store.
|
||
|
||
## Acceptance
|
||
|
||
- Zero references to `modernc.org/sqlite`, `?` placeholders,
|
||
`boolToInt`, `encodeTime`/`decodeTime` remain in `internal/store/`.
|
||
- Full existing store test suite passes against Postgres.
|
||
- Concurrent `CreateSession` calls (simulated) never produce duplicate
|
||
`sort_order` values within the same `group_name`/`parent_id`.
|
||
- A `spawn` CLI round trip (create session, update status, delete) works
|
||
end-to-end against the M6.1 cluster.
|
||
|
||
## Verify
|
||
|
||
**Harness:** `internal/store/*_test.go` running against the disposable
|
||
Postgres from M6.2's harness.
|
||
|
||
**Integration test** — extend/rename the existing store test files:
|
||
1. `a1_no_sqlite_references` — `grep -r "modernc.org/sqlite" internal/`
|
||
returns nothing.
|
||
2. `a2_no_bare_placeholders` — `grep -rE '\?[,)]' internal/store/store.go`
|
||
inside SQL string literals returns nothing (manual review of any
|
||
`?` that's part of a non-SQL string, e.g. a Go format verb, to avoid a
|
||
false positive).
|
||
3. `a3_existing_suite_passes` — `go test ./internal/store/...` green
|
||
against Postgres.
|
||
4. `a4_concurrent_sort_order` — spawn N goroutines each calling
|
||
`CreateSession` into the same group concurrently; assert the resulting
|
||
`sort_order` values are a dense 0..N-1 permutation with no duplicates.
|
||
5. `a5_fk_violation_translated` — attempt `PlaceSession` with a
|
||
`parentID` deleted between the read and the write (simulate via a
|
||
second connection); assert the returned error is the existing
|
||
descriptive one, not a raw pgx driver error.
|
||
6. `a6_spawn_roundtrip` — `spawn` CLI subcommand creates a session,
|
||
`UpdateStatus`, then `Delete`; assert no error and the row is gone.
|
||
|
||
**Command:** `go test ./internal/... -run TestStore -v`
|
||
|
||
**False pass:**
|
||
- Running the suite against sqlite still (leftover build tag or import)
|
||
while believing it validated Postgres. Assertion 1 is the guard —
|
||
without it, the whole task could report green while the driver swap
|
||
never actually happened.
|
||
- Skipping assertion 4. A `sort_order` race is invisible in every
|
||
single-threaded test and only shows up as duplicate/out-of-order
|
||
sessions under real concurrent use, which is exactly the class of bug
|
||
removing `SetMaxOpenConns(1)` introduces.
|
||
|
||
## Traps
|
||
|
||
- Converting `?` to `$N` by simple find-and-replace in file order without
|
||
re-checking each statement's actual arg list — a multi-arg statement
|
||
reordered during earlier edits (e.g. `createSession`'s 15-arg INSERT)
|
||
will silently bind the wrong value to the wrong column, and Go's
|
||
`database/sql` won't catch a type-compatible mismatch (e.g. two `TEXT`
|
||
columns swapped).
|
||
- Forgetting `pending_inputs` is now `JSONB` (M6.2), not `TEXT` — the
|
||
existing `json.Marshal`/`Unmarshal` round-trip in
|
||
`encodePendingInputs`/`pendingInputState` still works unchanged (pgx
|
||
scans `JSONB` into `[]byte` the same as `TEXT`), but don't add an extra
|
||
marshal layer thinking the column type changed the Go-side contract.
|
||
|
||
---
|
||
|
||
Background: [M6.2](M6.2-schema-port.md) · `internal/store/store.go`, `internal/spawn/spawn.go` (agent-manager, `add-headless-spawn` branch)
|