-
Notifications
You must be signed in to change notification settings - Fork 8
Proposal: optional PostgreSQL support, alongside SQLite #28
Description
We are running Calnode on Cloud Run and would like to move the store to PostgreSQL. We have a
working branch and would rather agree the shape with you before opening a large pull request.
Nothing below changes SQLite behaviour. Single-binary-plus-a-file stays the default and the
documented path; Postgres is opt-in via DATABASE_URL.
Why we want it
Cloud Run scales to zero and gives an instance an ephemeral filesystem, so SQLite lives on an
in-memory volume with Litestream replicating to object storage. That works, and we have verified
a real restore after an idle scale-down. But it forces --max-instances 1 permanently: two
instances would replicate two copies of the same file into one bucket. Postgres is what lets a
deployment scale horizontally without changing anything else.
If you would rather Calnode stay single-engine, say so and we will keep this in our fork. That is
a legitimate answer and we would not be offended.
What the work actually is
sqlc.yaml points at db/queries, which does not exist, and internal/db/sqlcdb is empty, so
every statement is hand-written: 763 Query/QueryRow/Exec call sites across 124 files,
453 of them outside tests in 74 files. The approach that made this tractable:
- One choke point. A
*db.DB/*db.Txwrapper whose method set matchesdatabase/sql
exactly and rebinds?to$nwhen the dialect is Postgres. Because the names match, the
763 call sites needed no edit at all — only the declarations that hold the handle changed. - Rebinding is a small lexer, not a string replace, so a
?inside a string literal, a
quoted identifier or a comment is left alone. Nested block comments are honoured because
Postgres nests them. - Per-dialect goose migration directories, the Postgres set generated once from the 57
existing files.
The three decisions we would most like your opinion on
1. Booleans stay integers. The schema stores flags as INTEGER NOT NULL DEFAULT 0/1 and the
tree compares them literally (email_login = 1, boolToInt(...)). We translated those to
SMALLINT, not BOOLEAN, so no scan target changes. BOOLEAN would be tidier and would touch
a great many call sites.
2. Timestamps stay TEXT. Every time value is a string end to end — WHERE run_at <= ?,
ORDER BY created_at, (start_at, id) pagination keys — so we translated
datetime('now') to to_char(now() AT TIME ZONE 'UTC', ...) rather than adopting TIMESTAMPTZ.
With TIMESTAMPTZ, database/sql hands a time.Time to a *string as RFC3339Nano, which sorts
differently from the stored SQLite form and would silently change API output.
order on SQLite but depends on the server's collation on Postgres. Mixed shapes (space-separated
vs T-separated) may not order identically. COLLATE "C" would settle it.
3. The single connection is a correctness guarantee, and it needed replacing.
docs/ARCHITECTURE.md is explicit that the app-level booking-overlap check is free of TOCTOU
races only because SetMaxOpenConns(1) serialises every transaction. That property does not
survive a pool. We take pg_advisory_xact_lock on the host id inside the booking and reschedule
transactions, released automatically at commit.
We proved it with a negative control rather than asserting it. Two goroutines racing overlapping
bookings for one host, 40 rounds:
| created | conflicts | overlapping pairs | |
|---|---|---|---|
| advisory lock in place | 40 | 40 | 0 |
| lock disabled | 79 | 1 | 39 |
The unlocked run is the interesting half: idx_bookings_no_double caught 1 race in 40, because a
partial unique index on (host_id, start_at) only catches an exact start-time collision, not a
partial overlap. We also cover ReassignHost, which has the same check-then-write shape.
One thing our change breaks that we would fix in the same PR
Splitting the migrations into per-dialect directories makes .github/workflows/audit.yml:98
match nothing — it globs internal/db/migrations/*.sql. Because grep -r on the unexpanded
literal exits 2, the if is simply false and the job still passes, so the tenant/workspace
column guard would become silently vacuous. audit/claims.yaml's single-binary-no-server-db
claim also becomes false, and its own check greps go.mod for postgres|mysql|redis, none of
which match github.com/jackc/pgx/v5. Both want a deliberate decision from you rather than a
quiet edit from us.
A smaller bug we found on the way, worth fixing regardless of this proposal
Constraint violations are classified by matching SQLite's English error text
(UNIQUE constraint failed and friends) in 13 places across 7 files — internal/booking/service.go
and, in internal/handler, event_type.go, booking_handler.go, teams.go, override.go,
idempotency.go, availability.go. On any engine or locale where that text differs the
handler falls through to a 500 instead of a 409/400/404. It is also unnecessary on SQLite:
modernc.org/sqlite exposes Code(), and the extended result codes are populated —
SQLITE_CONSTRAINT_UNIQUE 2067, _PRIMARYKEY 1555, _CHECK 275, _FOREIGNKEY 787.
must accept both or it silently stops recognising primary-key collisions.
Happy to send that as its own small PR first if you would prefer to take it independently.
Where the branch stands
go test ./... is green on both engines from the same tree — SQLite (the default, nothing
set) and PostgreSQL 17 via CALNODE_TEST_POSTGRES_DSN. gofmt -l is empty and go vet ./... is
clean.
Two things worth saying about how that was checked, because the first version of it lied to us:
- A green Postgres run is indistinguishable from a silently skipped one, since the helpers skip
when the DSN is unset. So it is confirmed with a positive control (the ten
TestPostgres_*cases are asserted to RUN, not skip) and a negative control (a deliberately
wrong password must fail the package rather than skip it — it does,SQLSTATE 28P01). - Per-package green was not enough. Two of us ran the suite in pieces and both came up clean while
13 call sites were still classifying constraint violations by SQLite's English error text;
only running the whole suite against both engines from one tree surfaced them. That is why the
error-code change above is in this proposal at all.
How we would like to proceed
Split into reviewable pieces rather than one enormous diff:
- the dialect layer and the rebinding wrapper, with SQLite behaviour unchanged;
- the generated Postgres migrations;
- the call-site conversion and the dialect-specific SQL;
- the advisory lock and its concurrency test;
- CI running the suite against a Postgres service container.
Tell us which of these you would take, in what order, and we will follow your preferences on the
three decisions above.