PostgreSQL support: a dialect-aware handle, a PostgreSQL migration set, and the test suite on both engines - #29
Open
distronode-com wants to merge 17 commits into
Open
Conversation
Calnode's SQL is hand-written and portable apart from placeholder syntax, so supporting a second engine is mostly a matter of not spreading "? vs $n" across 763 call sites. This adds the layer that hides it: - Dialect (sqlite, postgres) selected from the DATABASE_URL scheme, with Dialect.SQL for the few statements that genuinely cannot be shared. - Rebind: a small lexer, not a string replace. A ? inside a string literal, a quoted identifier or a comment is data, and renumbering one corrupts the statement in a way that only shows up at runtime. - DB/Tx wrapping sql.DB/sql.Tx, rebinding every statement on the way through. Query, Exec, QueryRow, their Context forms, Prepare and Begin/BeginTx are all covered, so a call site moving onto the wrapper is a type change and nothing else. Begin returns *db.Tx rather than *sql.Tx deliberately: a transaction that quietly stopped rebinding is the easiest way to reintroduce ? on Postgres. - OpenDB returns the wrapper; Open still returns the bare *sql.DB so nothing outside this package changes yet. The SQLite path is untouched: same pragmas, and the one-connection pool stays because it is the correctness guarantee behind the booking overlap check (ARCHITECTURE section 17), not a tuning choice. Postgres gets an ordinary pool, and openPostgres records what that costs so the gap is not rediscovered later. Migrations move to migrations/sqlite/ unchanged (git detects all 57 as renames); the postgres set lands in the next commit. is_applied is now tested for truth rather than compared to 1, which is the one spelling both engines accept -- goose stores it as INTEGER on SQLite and BOOLEAN on Postgres. pgx v5.10.0 is the driver, via database/sql (stdlib), so nothing here depends on the pgx-native API.
57 files in migrations/postgres, one per SQLite migration and numbered
identically, embedded alongside them. Open picks the directory and the goose
dialect from the DSN, so nothing else has to know which set is in use.
What the translation does, and what it deliberately does not:
- Flag columns stay integers (INTEGER -> SMALLINT), never BOOLEAN. Those columns
are scanned into Go ints across the codebase, so changing the type would break
every one of those scans. Columns that hold real numbers stay INTEGER. A test
reads information_schema and fails on a boolean.
- Timestamp defaults keep TEXT and keep SQLite's exact string format:
strftime('%Y-%m-%dT%H:%M:%fZ','now') becomes
to_char(now() AT TIME ZONE 'UTC', 'YYYY-MM-DD"T"HH24:MI:SS.MS"Z"') and
datetime('now') becomes the seconds-precision equivalent. TIMESTAMPTZ was the
alternative and is wrong here: every time value in this schema is written,
compared, sorted and paginated as a string, and database/sql would hand those
columns back formatted as RFC3339Nano, which sorts differently and would not
match a stored value. A test asserts both formats against the shapes SQLite
writes.
- Partial unique indexes carry over verbatim; a test proves idx_bookings_no_double
still refuses a second booking at the same start time and still allows re-booking
a cancelled slot.
- crypto_keystore.id relied on SQLite's rowid (keyvault.go inserts without an id),
so it becomes BIGINT GENERATED BY DEFAULT AS IDENTITY. server_settings.id does
not: it is seeded explicitly and pinned by CHECK(id = 1), where a sequence would
only mislead. BLOB becomes BYTEA.
- The three table rebuilds become what Postgres can do directly: 00012 drops the
four columns, 00014 and 00042 replace a constraint in place. 00042 therefore
needs neither NO TRANSACTION nor the foreign_keys PRAGMA toggle, both of which
existed only to make SQLite's rebuild safe.
- lower(hex(randomblob(16))) becomes replace(gen_random_uuid()::text, '-', ''),
the same 32 lowercase hex characters with no extension to install.
INSERT OR IGNORE becomes ON CONFLICT DO NOTHING.
- Downs that are no-ops on SQLite stay no-ops here even where Postgres could drop
the column. A down that lands on a different schema per engine is worse than one
that lands on none, and it would make the following up fail on one engine only.
Tests are opt-in on CALNODE_TEST_POSTGRES_DSN and skip cleanly when it is unset,
so upstream CI and any contributor running SQLite is unaffected. Each test runs in
a schema of its own, created and dropped around it, so runs neither collide nor
leave anything behind in an operator's database.
Every other cross-engine test asserts that one specific thing survived the translation. This one asserts that nothing else changed: it migrates SQLite and Postgres to 57 in the same process and compares the table list and each table's column names, so a column quietly dropped, renamed or added in one set fails here rather than in whichever handler reads it. Names only, deliberately. TEXT versus text and SMALLINT versus integer are the translation doing its job, and the types that do matter are already pinned by TestPostgres_flagColumnsStayIntegers. The table-count floor is there because an empty map on either side would satisfy every comparison in the test.
The dialect-aware wrapper rebinds ? to $n on Postgres and is a no-op on SQLite,
so the only way the rest of Calnode benefits is to hold the wrapper rather than
the bare *sql.DB. Method names match database/sql, so this is a type change at
the declarations and almost nothing at the 763 call sites.
Three places did need a decision rather than a substitution:
- internal/connstore.Execer is an interface over QueryRowContext, so both the
wrapper and a bare *sql.DB satisfy it; only its doc comment changed.
destination_test.go opens the sqlite driver directly against a bespoke
fragment schema, and stays on *sql.DB for that reason.
- handler.Readyz passes h.db.DB to db.SchemaReady, which takes a bare pool.
Safe: that statement has no placeholders, so there is nothing to rebind.
- cmd/calnode moved from db.Open + db.Migrate(database) to db.OpenDB and the
handle's own Migrate method, which is what carries the dialect.
SQLite behaviour is unchanged: Rebind returns its input untouched there.
Timestamps first, because they are 38 of the 44 sites. datetime('now') and
strftime('%Y-%m-%dT%H:%M:%fZ','now') are now computed in Go and bound as
parameters, which removes the dialect difference rather than encoding it and
makes the value something a test can predict.
internal/dbtime holds the two layouts. Two, not one: the schema already stores
both shapes, and they are load-bearing. recordings.created_at is the space-
separated form and meeting_consents.decided_at the millisecond form, and
consentWindow converts between them to build a lexicographic BETWEEN; notes'
updated_at is handed to a client verbatim; jobs.run_at written by enqueueJob is
deliberately the space form, which sorts before every T-separated run_at and is
what makes those jobs due immediately. Normalising would have been tidier and
would have quietly changed all three. dbtime_test asserts both layouts against
SQLite's own output rather than against a reading of its documentation.
The rest:
- INSERT OR IGNORE becomes ON CONFLICT DO NOTHING with no conflict target,
which both engines accept and which keeps OR IGNORE's "any unique
constraint" scope.
- COLLATE NOCASE becomes LOWER(col) = LOWER(?). No dialect pair, because
nothing indexes booking_attendees.email: there is no collation for an index
to depend on, so there is no plan to preserve.
- demo.Reset's PRAGMA foreign_keys is SQLite-only now. Postgres has no
equivalent without superuser rights, so that branch wipes with a single
TRUNCATE ... CASCADE over every table, which needs no delete ordering at
all. sqlite_master becomes pg_tables scoped to current_schema(), the one
Dialect.SQL pair added here — current_schema() also keeps it right inside an
isolated test schema.
RETURNING is left alone: both engines support it. No UPDATE or DELETE carries a
LIMIT; that was checked by scanning every backquoted string in the tree rather
than grepping single lines.
SQLite behaviour is unchanged, stored bytes included.
ARCHITECTURE §17 says the app-level double-booking check is free of TOCTOU races because SetMaxOpenConns(1) serialises every transaction. That is true and it is a SQLite property. On a Postgres pool two overlapping bookings can both read "host is free" and both insert, and idx_bookings_no_double is UNIQUE(host_id, start_at) so it only catches an IDENTICAL start time — 10:00-10:30 against 10:15-10:45 are two distinct keys and both inserts satisfy it. lockHosts takes pg_advisory_xact_lock on every host whose availability the transaction is about to decide, before the first hostBusy read. The lock is released by the transaction ending, so there is no unlock to forget and no leak down the several early-return paths these functions have. Per host rather than global, so bookings for different hosts stay concurrent — otherwise this would just reinstate the single writer it replaces. On SQLite it returns immediately and nothing changes. The key is SHA-256 of "calnode:booking:host:" + id, first eight bytes big-endian as int64. Derived in Go rather than with hashtext() so it is readable and testable from Go, and domain-separated so a later advisory lock on some other entity cannot collide by hashing the same raw id. Ids are sorted and deduplicated on a COPY before locking: two transactions needing the same two hosts in opposite orders would deadlock, and Create's HostIDs arrive in round-robin priority order, which decides who gets the booking. Three call sites, where the packet named two. ReassignHost has the identical check-then-write shape — hostBusy's own doc comment names all three write paths — and leaving it out would have left a known double-booking hole. Reschedule locks the primary host BEFORE reading booking_hosts rather than after, because a concurrent ReassignHost holds that same key, and that ordering is what stops a reassignment committing between the host-list read and the UPDATE. isUniqueViolation now matches SQLSTATE 23505 as well as SQLite's message text. It was a substring match on "UNIQUE constraint failed", so on Postgres the index backstop would have surfaced as a 500 rather than ErrDoubleBooked. SQLSTATE because the message is localised by the server's lc_messages. Measured against PostgreSQL 17, 40 rounds of two goroutines racing overlapping slots: with the lock, 40 created / 40 conflicts / 0 overlapping pairs in the database. With the lock disabled, 79 created / 1 conflict / 39 overlapping pairs. The unlocked run is why the index cannot be described as a sufficient guard: it caught one race in forty. internal/dbtest arrives with this rather than with the docs commit, because the committed version of that test needs it. Each test gets its own calnode_test_<random> schema via search_path in the DSN, dropped on cleanup; the test that asserts the isolation actually reaches the server is in that package, because a search_path that silently stopped being forwarded would migrate every package into public and read as flakiness.
22 test helpers across 18 files moved from db.OpenDB("sqlite://:memory:") +
Migrate() to dbtest.Open(t), so which engine the suite exercises is an
environment variable rather than 22 hardcoded DSNs. With
CALNODE_TEST_POSTGRES_DSN unset — every local run, and the existing CI job —
nothing changes: it is the same in-memory SQLite as before.
Four helpers stay on SQLite deliberately: dbtime_test pins SQLite's own timestamp
output, hostlock_internal_test asserts the advisory lock is a no-op there,
connstore/destination_test opens the bare driver against a bespoke fragment
schema, and two health_test cases want an UNMIGRATED database. keyvault_test
keeps its shared-cache DSN, which it has for a reason.
ARCHITECTURE §4 is amended rather than extended, because a new section would have
sat next to prose asserting the opposite. The heading covers both engines; the
pool difference is stated along with the way the wrapper can be bypassed
silently; the rows-cursor gotcha is scoped to SQLite together with the reason the
materialise-first pattern has to stay regardless; and a new subsection states the
double-booking guarantee per engine — the single connection on SQLite, the
advisory lock on Postgres — plus what the partial unique index actually covers,
with the measured 39-double-bookings-without-the-lock figure. §17's first gotcha
and one now-engine-conditional aside in §8 were pulled into line with it.
The new CI job runs the Go half against a postgres:17 service. Separate job, not
a matrix, because only Go is engine-dependent and svelte-check would otherwise
run twice. It carries a pg_isready health check: without one the first connection
races the server's startup and fails as "connection refused", which reads like a
bad DSN rather than a timing problem.
That job will be RED until internal/db/migrations/postgres exists — the database
layer is another worker's file set and it has only the sqlite directory today.
Measured rather than assumed: with the DSN set, dbtest.Open reports "run
migrations: migrations/postgres directory does not exist".
Thirteen sites decided between 409, 400, 404 and 500 by substring-matching
SQLite's error text. On PostgreSQL none of them matched, so a duplicate slug, a
replayed idempotency key and a bad enum value all fell through to
{"error":"internal error"} with a 500. Seven Postgres test failures were this.
internal/db now exports IsUniqueViolation, IsCheckViolation and
IsForeignKeyViolation. PostgreSQL is matched on SQLSTATE (23505 / 23514 / 23503)
because the message is localised by the server's lc_messages, so a server running
in a non-English locale would defeat any text match no matter how carefully it was
written. SQLite keeps a text match, because modernc.org/sqlite does not expose a
code on its error value — but it is now in exactly one place instead of thirteen,
and it is pinned by a test.
A *pgconn.PgError whose code does not match returns false rather than falling
through to the text comparison. Falling through would classify a PostgreSQL error
by whether its message happened to contain another engine's English phrase, which
is the fragility being removed.
The test provokes a REAL violation of each class against the real migrated schema
on whichever engine dbtest is configured for — a duplicate users.email, a booking
with a status the CHECK forbids, a booking referencing a nonexistent event type —
rather than constructing an error value, because a constructed error only proves
the predicate agrees with what the author believed the driver returns, and that
belief being wrong is why this commit exists. It also asserts exactly ONE of the
three predicates matches each violation, and that a missing-table error and a
plain errors.New match none: a predicate that answered true for everything would
have made all thirteen call sites pass and been badly wrong.
internal/booking and internal/handler keep thin local wrappers where a local name
read better at the call site; their bodies now delegate.
grep -rn "constraint failed" --include='*.go' internal/ cmd/ returns three hits,
all of them the constants inside internal/db/constraint.go.
With the constraint predicates in, eight Postgres failures were left, and they were
four unrelated causes rather than one.
Computed booleans. "(user_id = ?) AS owned" and "(archived_at IS NOT NULL) AS
archived" are 0/1 on SQLite and a boolean on PostgreSQL, and the boolean does not
scan into the int the real 0/1 columns beside them use — "converting driver.Value
type bool to a int". Both are CASE WHEN ... THEN 1 ELSE 0 END now: portable, and
the same 0/1 convention the schema uses for stored flags, so the != 0 idiom in the
scanner is untouched.
json_extract in production. replaceReminderJobs deleted a booking's reminder jobs
by filtering on json_extract(payload, '$.booking_id'), a SQLite JSON1 function. On
PostgreSQL the DELETE errored, so rescheduling silently left the old reminder in
place — a real bug, not just a red test. No portable spelling exists, so this is a
Dialect.SQL pair: payload::json ->> 'booking_id', with the cast because the column
is TEXT rather than json.
ORDER BY rowid. webhook_deliveries had no timestamp of its own, so "the 50 most
recent" was rowid DESC. PostgreSQL has no rowid, and the query was not strictly
correct on SQLite either: rowid tracks insertion order only until something
renumbers it, and VACUUM may. Migration 00058 adds created_at to both dirs, the
writer binds it, and the order is created_at DESC, id DESC — the tiebreak because
two deliveries written in the same millisecond would otherwise come back in
whatever order the engine felt like. The default is a constant '' rather than a
timestamp expression because SQLite's ALTER TABLE ADD COLUMN forbids a
parenthesised DEFAULT; rows predating the migration sort last, which is where the
oldest deliveries belong.
randomblob in a gcal test helper: SQLite's id generator, swapped for uid.New(),
which is what every other row in those tests already used.
Two test changes, neither of them an assertion:
- reschedule_test held its own copy of the json_extract expression AND discarded
the Scan error, so "function json_extract does not exist" surfaced two seconds
later as time.Parse failing on "". It now uses the same dialect pair and
reports the query error, so the next engine difference names itself.
- postgres_test pinned the migration count as the literal 57 in two places. It is
now one named constant at 58, so adding a migration is a single edit that
cannot be half-done.
go test -count=1 ./... is green on both engines from this commit: 28 of 28 packages
on SQLite, 28 of 28 on PostgreSQL.
The comment in constraint.go claimed modernc.org/sqlite exposes no error codes. That was wrong. It defines type Error with Code(), populated for every constraint class, and the codes are SQLite's extended result codes. Measured against a real in-memory database rather than read off a doc page: UNIQUE 2067, PRIMARY KEY 1555, CHECK 275, FOREIGN KEY 787, NOT NULL 1299 ⛔ The reason this is worth a commit rather than a tidy-up: a PRIMARY KEY collision reports 1555, NOT 2067, while still saying "UNIQUE constraint failed" in its message. The text match caught both by accident. Switching to Code() == 2067 alone would silently stop recognising primary-key collisions — and Calnode has one that matters: idempotency_keys.idempotency_key is a bare PRIMARY KEY, so every idempotent replay arrives as 1555. Verified by removing 1555 and re-running: the new subtest fails naming the code and the table, and TestCreateBooking_idempotentReplay goes to "replay: 500". So the regression was real, not theoretical. IsUniqueViolation therefore matches BOTH 2067 and 1555. PostgreSQL needs no equivalent change: a primary-key collision is 23505 like any other unique violation, which is why the trap exists on only one side. Constants are named after the SQLite symbols so the numbers are greppable. The text comparison stays as a fallback, and only as a fallback. It covers an error that arrives without its concrete driver type attached — a driver release that changes the type, a layer that reformats rather than wraps — where the message is the only signal left and answering from it beats returning a 500. TestConstraintTextFallback exercises that branch directly, so it is not unexecuted code that reads like an accident. A *pgconn.PgError or a *sqlite.Error whose code does not match is a DEFINITE no and does not fall through: falling through would reintroduce this very trap in reverse, readmitting a 1555 by its message after excluding it by code. Also fixes a flake in the dbtest harness, which is what the two intermittent handler failures under `go test ./...` actually were — not an application fault. Calnode's handlers do several things fire-and-forget (notify hosts, enqueue webhook, enqueue reminders), those goroutines outlive the test body, and closing the pool does not stop an in-flight statement. DROP SCHEMA CASCADE needs an exclusive lock on every object, meets them, and PostgreSQL reports "deadlock detected" (40P01) — visible only under the full run, where packages compete and everything is slower, in a different pair of tests each time. The drop now runs on a pinned connection with lock_timeout set so an attempt fails fast instead of deadlocking, and retries within a bounded budget; a schema that still cannot be dropped is reported, because a leaked schema accumulates on a shared server.
Includes the measured code table, the primary-key trap and how it was proved, and the dbtest teardown deadlock that the two intermittent handler failures actually were.
Timestamps are TEXT and compared lexicographically on purpose: the worker
claims with run_at <= ?, sessions and tokens expire on expires_at > ?, the
consent window brackets decided_at, booking overlap compares start_at/end_at
against bound strings, and several lists are ORDER BY created_at. On SQLite
that is memcmp. On Postgres it is the column's collation, which by default is
the database's.
Migration 00059 alters 54 columns across 27 tables to COLLATE "C" on Postgres
(every *_at, jobs.locked_until, and the availability HH:MM / YYYY-MM-DD
columns, which are ordered as times too). Fixed in the schema rather than in
40-odd predicates that would each have to remember a COLLATE clause. The
SQLite half is a no-op file: BINARY is already memcmp, and the file exists so
the two directories keep one file per version.
internal/handler/notetaker.go is the site that makes this load-bearing rather
than theoretical: it writes datetime('now')'s space-separated shape precisely
because it sorts before any T-separated stamp, which is what makes a notetaker
job due immediately.
collation_test.go holds it three ways:
- an audit over information_schema.columns, matching by NAME so a timestamp
column added by a later migration is caught without anyone remembering;
- a positive control that skips, rather than passing vacuously, if the server's
own default already orders byte-wise;
- an ordering test on jobs.run_at through the worker's real claim predicate.
Measured, not assumed: the two shapes the schema actually stores do NOT flip
under this server's en_US.utf8 default, nor under any of the other 878
collations installed on it - glibc ignores the space at the primary level but
still sorts a digit before 'T'. The control therefore also carries RFC 3339's
lower-case t/z spelling (permitted by RFC 3339 section 5.6, so an importer can
hand it to us), where en_US.utf8 puts lower case first and memcmp puts upper
case first. Proved failable by dropping jobs.run_at from the migration: the
audit reports "jobs.run_at = <database default>" and the ordering test reports
the two spellings transposed.
Boundary 2 ported the tree's one RETURNING clause (question_handler.go's auto-position INSERT) and left it verified on SQLite and unverified on PostgreSQL. It runs unchanged on both: no d.SQL pair, no rewrite. The test goes through the HTTP handler on dbtest.Open(t), so it follows CALNODE_TEST_POSTGRES_DSN like the rest of the suite, and it asserts the value the handler SCANNED from RETURNING as well as the row left behind - a RETURNING that quietly produced a zero would still leave a correct row, so checking the table alone (which TestCreateQuestion_autoPosition already did) cannot see it. The explicit-position case is in the same test on purpose: after a pinned position 9, the next auto position must be 10, which is what shows the COALESCE(MAX(position)+1, 0) subselect inside VALUES is really evaluated by the engine rather than the sequence being an artifact of insertion order. Measured: 0, 1, 2, then 9 explicit, then 10, identical on sqlite and postgres.
Open("...") returned a plain *sql.DB and was documented as being "for callers
that have not moved to OpenDB yet". Nothing outside the package's own tests
used it, and it could only ever do harm: a statement issued through that handle
is not rebound, so every ? in it is a Postgres syntax error discovered at
runtime, a long way from the call. It was also the shape most likely to be
copied by the next person adding a call site.
/readyz was the one production caller reaching into the exported embedded field
(db.SchemaReady(ctx, h.db.DB)). *DB now carries SchemaReady and AppliedVersion,
so the handler asks the handle. The package-level functions stay for the
bare-pool cases that really do need one - goose's own bookkeeping, and the
tests that open an unmigrated pool - and db_test.go passes database.DB to them
explicitly, which says at the call site that the bare pool is deliberate.
No test asserts the absence of the symbol: the build is the proof, and a
reflective check on a package's exported names is not cheap enough to be worth
the fixture. TestOpen_inMemory is renamed TestOpenDB_inMemory rather than left
named after something that no longer exists.
SetMaxOpenConns(10) / SetMaxIdleConns(5) were literals in openPostgres. The number that fits is a property of the server, not of Calnode: PostgreSQL's max_connections is shared with every other client, and an instance behind PgBouncer wants a different figure from one talking to a 200-connection server directly. DB_MAX_OPEN_CONNS and DB_MAX_IDLE_CONNS now carry it, defaults unchanged at 10/5. Validation lives in config.PoolFromEnv: unset, unparsable or non-positive falls back to the default with a warning (matching getBool/getDuration, which also refuse to fail a boot over a typo in an optional knob), and an idle limit above the open limit is clamped - database/sql silently reduces it anyway, so the clamped pair is the honest description of what the pool will do. The clamp applies to the DEFAULT idle too, which is the case a test pins: DB_MAX_OPEN_CONNS=2 alone must give 2/2, not 2/5. OpenDB reads PoolFromEnv itself rather than taking the numbers as an argument, so every entry point (server, mcp, rotate_key, recover_key, reset_admin) picks them up without five identical edits and without one of them silently keeping the defaults. db -> config is not a cycle; config imports only the standard library. A caller that must not follow the environment passes db.WithPool. SQLite stays pinned at 1/1 and ignores both the environment and WithPool. That is a correctness guarantee, not a preference: the single connection is what serialises write transactions (it is why booking.lockHosts is a no-op there), and the pragmas are connection-scoped, so a second connection would not have them. TestOpenDB_sqlitePoolIsNotConfigurable asserts it with both knobs set to 40/20, for both the :memory: and file forms. The idle half cannot be read back from database/sql (DBStats exposes the open limit only), so it is measured instead: four live transactions force four connections, and after committing them all the pool keeps 1 with WithPool(4, 1). Measured on the live server, "idle after release 1".
The four remaining items and what was measured for each: 54 columns across 27 tables pinned to COLLATE "C", the server collation the control fired against (en_US.utf8, libc) and the correction that the two shapes the schema really stores do not flip under any of the 878 collations installed there, RETURNING position giving 0/1/2 then 9 then 10 identically on both engines, and the pool keeping 1 idle connection with WithPool(4, 1). Open item 2 is marked closed in place rather than deleted, since Boundary 6 is where it was raised.
PROGRESS.md is the working log of the fork this branch was developed on. It is not part of the change and does not belong upstream; the reasoning it holds is in the commit messages.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Important
Solid dual-engine port, but product/audit/deploy surfaces still describe a SQLite-only world. Update those before this ships as “Postgres support,” or operators and the audit gate will disagree with the binary.
Reviewed changes Full PR (17 commits): dialect-aware OpenDB + rebinding handle, parallel Postgres/SQLite goose trees, portable SQL ports, host advisory locks, collation pin, dual-engine CI/dbtest, and call-site threading of *db.DB.
- Dialect handle —
*db.DB/*db.Txrebind?→$n; bare embedded*sql.DBis a documented footgun with an e2e test that fails without the wrapper. - Migrations — locked-step
migrations/{sqlite,postgres}/(59 versions), schema name parity, flag columns stay integer-typed for Go scans. - Double-booking —
pg_advisory_xact_lockon Create/Reschedule/ReassignHost with sorted keys; 40-round partial-overlap race test on Postgres. - SQL ports —
dbtime, constraint codes (incl. SQLite PK 1555),LOWER()email match,ON CONFLICT, JSON extract viaDialect.SQL, demoTRUNCATEvs SQLite FK pragma. - CI / harness — separate Postgres job, per-test schemas, pool knobs, positive/skip controls for opt-in DSN.
⚠️ Public claims and audit guards still describe SQLite-only
The binary can open Postgres, but the public and audit story was not updated with the engine split. That leaves a green audit job and outdated marketing while the code contradicts both.
Technical details
# Align claims and audit with optional Postgres
## Affected sites
- `audit/claims.yaml` (`single-binary-no-server-db`) — still says no Postgres driver and verify via `grep postgres go.mod` / “only modernc.org/sqlite”
- `.github/workflows/audit.yml` — `grep -iE 'postgres|mysql|redis' go.mod` does **not** match `jackc/pgx`, so the gate stays green
- same workflow — `grep … internal/db/migrations/*.sql` no longer expands after the move to `migrations/{sqlite,postgres}/` (tenant check becomes a silent no-op)
- `AUDIT.md`, `README.md` — still “no Postgres”; `docs/ARCHITECTURE.md` §1 still “Persistence is **SQLite**” while §4 documents both engines
- `audit/claims.yaml` instance-per-tenant verify path uses the same stale migrations glob
## Required outcome
- Rewrite the claim to “SQLite by default; Postgres optional via DATABASE_URL” (or equivalent truth)
- Fix audit verify commands (match `pgx` if the claim is “no *required* server DB”, and glob `migrations/**/*.sql`)
- Soften README/AUDIT/ARCHITECTURE openers so they do not deny the feature this PR adds⚠️ Postgres DATABASE_URL plus Litestream is an unguided footgun
DEPLOY.md documents postgres:// and pool knobs, but the image entrypoint and backup story remain SQLite/Litestream-shaped. An operator can set both and get a working app with a useless or confusing replica path.
Technical details
# Document Postgres backups and Litestream scope
## Affected sites
- `DEPLOY.md` env table — Postgres `DATABASE_URL` added; `LITESTREAM_REPLICA_URL` still “recommended” without engine scope
- `entrypoint.sh` (unchanged) — always `litestream restore/replicate` against `/data/calnode.db` when replica URL is set
- Storage UI / recordings still hang off `LITESTREAM_*` S3 for object storage (orthogonal, but the “backup” label is SQLite-only)
## Required outcome
- State explicitly that Litestream is SQLite-only
- For `postgres://` deployments: use the provider’s native backups / PITR; do not expect Litestream to cover the app DB
- Optional: skip Litestream in entrypoint when `DATABASE_URL` is postgres (or fail fast with a clear message)ℹ️ Question auto-position loses SQLite’s free serialization on Postgres
INSERT … (SELECT MAX(position)+1 …) RETURNING position was effectively race-free under MaxOpenConns(1). The multi-connection pool does not lock that path, and there is no unique constraint on (event_type_id, position). Duplicate positions are admin UX only, not booking integrity — but the new test’s comment claims concurrency safety the SQL does not provide on Postgres.
Technical details
# Honest coverage for question position
## Affected sites
- `internal/handler/question_handler.go` — auto-position INSERT subquery
- `internal/handler/question_test.go` — comment “two concurrent creates cannot land on the same position”
## Required outcome
- Soften the comment to “RETURNING value matches stored row” (what the test actually checks), **or**
- Serialize with a lock / unique `(event_type_id, position)` + retry if concurrent admin creates matterℹ️ Nitpicks
docs/ARCHITECTURE.md§1 still says persistence is SQLite while §4 (updated here) describes both engines — fix the opener in the same PR.- Worker purge comment still refers only to “the SQLite file Litestream replicates”; harmless, but stale once Postgres is first-class.
- PR body’s “wrong password FAILS the suite” is behavioral (open/migrate hard-fails) rather than a dedicated negative-control test; fine, just don’t oversell it as a named control.
Grok ｜ 𝕏
| | `BASE_URL` | **yes (prod)** | `http://localhost:3000` | Identity host — admin UI, OAuth callbacks, invite links. **Must include the scheme** (`https://booking.example.com`). The `https://` prefix flips the app into production mode (secure cookies, encryption-key enforcement). | | ||
| | `PUBLIC_BASE_URL` | no | = `BASE_URL` | Booker-facing host for booking links/emails, if different from the identity host. | | ||
| | `DATABASE_URL` | no | `sqlite://./data/calnode.db` | Point at the persistent volume, e.g. `sqlite:///data/calnode.db`. | | ||
| | `DATABASE_URL` | no | `sqlite://./data/calnode.db` | Point at the persistent volume, e.g. `sqlite:///data/calnode.db`. A `postgres://user:pass@host:5432/dbname` URL selects PostgreSQL instead; anything else is SQLite. | |
Contributor
There was a problem hiding this comment.
Postgres is documented here, but LITESTREAM_REPLICA_URL two rows down is still unqualified “recommended,” and the image entrypoint always runs Litestream against /data/calnode.db. Please spell out that Litestream is SQLite-only and that postgres:// deployments need native Postgres backups (or skip Litestream when the URL scheme is postgres).
Technical details
# Scope Litestream to SQLite in deploy docs
## Affected sites
- `DEPLOY.md` — this row + `LITESTREAM_REPLICA_URL`
- `entrypoint.sh` — restore/replicate of `/data/calnode.db` whenever replica URL is set
## Required outcome
- Operators choosing Postgres know not to rely on Litestream for the app DB
- Optional runtime guard so both env vars together do not silently mislead| // tree (question_handler.go): when the caller sends no position, the handler | ||
| // computes it inside the INSERT — VALUES (…, (SELECT COALESCE(MAX(position)+1, 0) | ||
| // …)) RETURNING position — rather than SELECT-then-INSERT, so two concurrent | ||
| // creates cannot land on the same position. |
Contributor
There was a problem hiding this comment.
This comment overclaims: the test checks RETURNING vs the stored row on one connection, not concurrent creates. Under Postgres’s multi-connection pool, two concurrent MAX(position)+1 inserts can still collide (no unique on (event_type_id, position)). Soften the comment to match what is asserted, or add a real concurrent test if you intend to lock this path.
Technical details
# Align comment with actual guarantee
## Affected sites
- `question_handler.go` auto-position INSERT
- this test comment (lines 237–239)
## Required outcome
- Comment describes RETURNING scan fidelity, not concurrency, unless locking/unique is added
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this adds
Calnode can now run on PostgreSQL as well as SQLite, chosen by the
DATABASE_URLscheme. SQLite stays the default and the single-binary story is unchanged: every existing test passes without modification, and the SQLite path is byte-identical in behaviour.internal/db: a dialect-awareOpenand a small*db.DB/*db.Txwrapper that rebinds?placeholders per engine, so the ~180 call sites keep their SQL and gain nothing engine-specific. The baresql.Openpath is deleted; readiness goes through the handle.knownMigrationCountmoves with every migration, on purpose).INSERT OR IGNORE,datetime()arithmetic,json_extract,RETURNING position, and the last four odd spots), each as its own commit with the reason.COLLATE "C"so the job queue'sORDER BY run_atsorts the same way on both engines regardless of the server's locale.DB_MAX_OPEN_CONNS/DB_MAX_IDLE_CONNS, defaulting to 10/5) on PostgreSQL only; SQLite keeps its single connection.How it is tested
CI runs the whole suite twice: on SQLite as before, and against a PostgreSQL service with
CALNODE_TEST_POSTGRES_DSNset. The PostgreSQL lane has a positive control (TestPostgres_*assert they ran) and a negative one: a wrong password FAILS the suite rather than silently skipping it, which is the failure mode this kind of dual-engine setup usually has.docs/ARCHITECTURE.mdgains the section describing the two engines and the rules for SQL that has to work on both.Compatibility
No configuration change is needed for existing deployments.
sqlite://DSNs behave exactly as before;postgres://DSNs are new.🤖 Generated with Claude Code