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)
|