feat(namedlock): add PostgreSQL advisory-lock implementation

[Kiran01bm]

Summary

Adds namedlock.Postgres, the PostgreSQL advisory-lock implementation of the namedlock.Locker seam. This is the Postgres counterpart to namedlock.MySQL (GET_LOCK/RELEASE_LOCK) and a prerequisite for both the Postgres schema bootstrapper and the Postgres state store's injected locker.

What

• namedlock.Postgres acquires session-level advisory locks (pg_advisory_lock / pg_advisory_unlock) on the pinned connection, matching the connection-lifetime semantics the seam already guarantees.
• Zero-timeout acquisition uses pg_try_advisory_lock (non-blocking, immediate verdict).
• Positive bounded waits set a transaction-local lock_timeout via set_config(..., true) so the timeout cannot leak into pooled connections; SQLSTATE 55P03 (lock_not_available) maps to acquired=false, err=nil, mirroring MySQL's GET_LOCK(...) = 0.
• A commit failure after the advisory lock is granted undoes the acquisition (best-effort unlock, then connection discard), preserving the seam's "error ⇒ lock not held" invariant so a pooled session can never strand the lock.
• Lock names map to stable int64 advisory-lock keys via sha256 (first 8 bytes), with pinned-value tests over the production lock names so mixed-version pods derive the same key during rolling deploys.
• The integration suite is parameterized over both lockers: acquire/contention, bounded-wait elapse, release-of-unheld, distinct-name independence, and auto-release on session end all run against MySQL and PostgreSQL containers, with the lock_timeout leak check as a Postgres-only extra.

Why

MySQL named locks (GET_LOCK) have no direct PostgreSQL equivalent; advisory locks are the idiomatic replacement but differ in shape (int64 keys instead of strings, no per-call wait argument). This change absorbs those differences behind the existing Locker interface so callers (apply-target lock, pending-drops lock, schema-bootstrap lock) stay dialect-agnostic.

Before / after

Before:
┌──────────────────────┐     ┌───────────────────────────────┐
│ namedlock.Locker     │────▶│ MySQL: GET_LOCK(name, wait)   │
│ (pinned conn seam)   │     └───────────────────────────────┘
└──────────────────────┘      Postgres: (none — fails closed)

After:
┌──────────────────────┐     ┌───────────────────────────────┐
│ namedlock.Locker     │────▶│ MySQL: GET_LOCK(name, wait)   │
│ (pinned conn seam)   │     ├───────────────────────────────┤
└──────────────────────┘     │ Postgres:                     │
                             │  wait=0 → pg_try_advisory_lock│
                             │  wait>0 → local lock_timeout  │
                             │           + pg_advisory_lock  │
                             │  55P03  → acquired=false      │
                             │  key    = sha256(name)[:8]    │
                             └───────────────────────────────┘

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[aparajon]

🤖 Adversarial correctness review, requested by Armand and performed by his agent. Reviewed at head aa55d64.

Verdict: one fix-before-merge finding — a lock-leak window on the pooled-connection acquire-error path — plus a PR-body correction. The core design (transaction-local lock_timeout around pg_advisory_lock, session lock surviving commit, 55P03 → (false, nil), FNV-1a key pinning) is correct, and all 8 integration tests pass locally against real MySQL 8.4 and postgres:16 containers.

Finding 1 (fix before merge): a failed commit leaves a pooled session holding the advisory lock

Acquire's commit-failure comment says reporting failure makes the caller "close conn, which drops the session and the lock with it" (postgres.go:81–83), and the interface godoc makes the same claim ("dropped by Release or when conn closes"). That's only true when closing the conn actually ends the session. The apply-target lock draws its connection from the store's long-lived pool, and its acquire-error path calls plain conn.Close() (closeApplyTargetLockConn, pkg/storage/mysqlstore/applies.go:255) — a pool return, not a session teardown. Only the release path discards via driver.ErrBadConn (discardApplyTargetLockConn).

The window is realizable, not theoretical: database/sql watches the context on an open *sql.Tx and auto-rolls it back on cancellation. If the caller's ctx is cancelled between pg_advisory_lock succeeding and Commit, the tx rolls back and Commit returns an error — but session-level advisory locks ignore transaction end, so the session goes back to the pool still holding the lock. Every later apply for that target then waits out applyTargetLockWait against an idle pooled session until the pool happens to retire it, with no operator handle on the stuck state (no live drive holds the lock).

The MySQL locker can't hit this — its single GET_LOCK either grants or errors — so the close-vs-discard asymmetry in applies.go is correct today; the hazard arrives with this implementation. (The other two call sites are safe by construction: pending-drops cleanup and the EnsureSchema bootstrap each open a dedicated *sql.DB per acquisition and close it with the pass, so their conn.Close() genuinely terminates the session.)

Suggested fix, contained to postgres.go: on commit failure, best-effort pg_advisory_unlock($1) on the same conn before returning the error — using context.WithoutCancel(ctx), since cancellation is the likely trigger and cleanup on the dead ctx would fail the same way — folding an unlock failure into the returned error. The same cleanup is worth applying on the non-55P03 error branch after pg_advisory_lock was issued (a cancel can race the grant there too; unlocking an unheld lock is a safe no-op, as TestPostgresReleaseUnheldLock shows). Then correct both comments: the lock is dropped by Release, by real session teardown, or by the unlock-on-error path — not by a pooled conn.Close(). The alternative — requiring callers to discard-not-close on Acquire error — pushes an engine-specific burden onto every pool-backed call site; fixing it in the locker keeps the Locker error contract simple: on error, the lock is not held.

Finding 2 (PR body): the test claims overstate the diff

The body says the shared integration suite "now runs against both MySQL and PostgreSQL containers, proving behavioral parity (acquire, contention, timeout, release, connection-scoped auto-release)". What's in the diff: the shared suite is still the two MySQL-only tests, the PG coverage is four separate mirrored tests, and no test in either dialect covers connection-scoped auto-release. Worth correcting the body — and a real auto-release test (dedicated *sql.DB, acquire, close the db, contender acquires) would document exactly the server behavior the cleanup story in finding 1 relies on.

Verified solid: the set_config('lock_timeout', $1, true) transaction-local scoping is the right mechanism — it reverts at transaction end including the 55P03 abort, and TestPostgresBoundedWaitElapsesWithoutLeakingTimeout proves the connection comes back with lock_timeout = 0; the session-level lock correctly survives the commit; waitMillis ceiling math is exact; negative waits are rejected, mirroring MySQL; errors.As on *pgconn.PgError is the right way to detect 55P03 through database/sql with the pgx stdlib driver; the FNV-1a key derivation is pinned by value so mixed-version pods coordinate during rolling deploys, and the collision consequence (extra serialization, never lost mutual exclusion) is documented accurately; server-wide advisory-lock scope matches GET_LOCK's server-wide semantics, so parity holds for multi-database instances; rollbackAfter folds rollback failures into the returned error instead of losing them; the TestMain refactor's deferred terminations keep an earlier container from leaking when a later one fails to start. Build, unit (-race), and integration suites all green locally; CI is green 32/32.

This review was generated by Claude Code (claude-fable-5).

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[Copilot]

Pull request overview

Adds PostgreSQL support to SchemaBot’s session-scoped “named lock” seam by implementing namedlock.Postgres using advisory locks, and introduces embedded PostgreSQL storage-schema DDL plus tests to keep Postgres and MySQL schemas aligned.

Changes:

• Implement namedlock.Postgres with pg_try_advisory_lock for zero-wait and bounded waits via transaction-local lock_timeout.
• Add embedded pkg/schema/postgres/*.sql storage schema files and schema parity/lint/integration tests against a real Postgres container.
• Extend namedlock integration tests and CI image pre-pulls to include PostgreSQL.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
pkg/testutil/container.go	Generalize container connection string helper comment for non-MySQL DBs.
pkg/schema/schema.go	Embed and document PostgresFS alongside MySQLFS.
pkg/schema/postgres/webhook_events.sql	PostgreSQL DDL for webhook_events table + indexes.
pkg/schema/postgres/tasks.sql	PostgreSQL DDL for tasks table + indexes.
pkg/schema/postgres/settings.sql	PostgreSQL DDL for settings table + indexes.
pkg/schema/postgres/plans.sql	PostgreSQL DDL for plans table + indexes.
pkg/schema/postgres/plan_comments.sql	PostgreSQL DDL for plan_comments table + indexes.
pkg/schema/postgres/locks.sql	PostgreSQL DDL for locks table + indexes.
pkg/schema/postgres/checks.sql	PostgreSQL DDL for checks table + indexes.
pkg/schema/postgres/apply_target_locks.sql	PostgreSQL DDL for apply_target_locks table + indexes.
pkg/schema/postgres/apply_operations.sql	PostgreSQL DDL for apply_operations table + indexes.
pkg/schema/postgres/apply_logs.sql	PostgreSQL DDL for apply_logs table + indexes.
pkg/schema/postgres/apply_control_requests.sql	PostgreSQL DDL for apply_control_requests table + indexes.
pkg/schema/postgres/apply_comments.sql	PostgreSQL DDL for apply_comments table + indexes.
pkg/schema/postgres/applies.sql	PostgreSQL DDL for applies table + indexes.
pkg/schema/postgres_test.go	Unit tests to lint Postgres schema files and enforce parity with MySQL indexes/files.
pkg/schema/postgres_integration_test.go	Integration test that executes Postgres DDL and validates column-by-column parity with MySQL schema.
pkg/namedlock/postgres.go	New Postgres advisory-lock implementation of namedlock.Locker.
pkg/namedlock/postgres_test.go	Unit tests pin advisory-lock key hashing for cross-version stability.
pkg/namedlock/postgres_integration_test.go	Integration tests validating Postgres locker semantics (contention, timeout, release).
pkg/namedlock/namedlock.go	Update package docs to include Postgres lock semantics.
pkg/namedlock/namedlock_integration_test.go	Start a shared Postgres container in TestMain alongside MySQL for integration tests.
go.mod	Add pgx driver and testcontainers Postgres module dependencies.
go.sum	Dependency lockfile updates for Postgres-related modules.
.github/workflows/test.yaml	Pre-pull postgres:16 image in CI to reduce test startup overhead.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Kiran01bm]

Review response from Kiran's (@Kiran01bm) AI code review assessment agent

Both findings are fixed at the current head (the branch was also rebased past the #936 squash-merge, so the reviewed head aa55d64 no longer exists).

#	Finding	Status	Explanation
1	Failed commit leaves a pooled session holding the advisory lock	fixed	Acquire now undoes the acquisition on both failure paths after the lock may have been granted — the commit-failure branch and the non-55P03 error branch (a cancel racing the grant, as suggested): best-effort pg_advisory_unlock on a bounded context.WithoutCancel context, and if that also fails the connection is marked bad (driver.ErrBadConn) so the pool destroys it and the session teardown drops the lock. The Locker contract now states "on error the lock is not held", and the pooled-conn.Close() claims in both comments were corrected to discard/session-end semantics.
2	PR body overstates test coverage; no auto-release test in either dialect	fixed	The integration suite is now parameterized over both lockers (exclusivity, bounded-wait elapse, release-of-unheld, distinct names), and a new auto-release test acquires on a dedicated *sql.DB, ends the session, and asserts a contender can take the lock — on both MySQL and PostgreSQL. The PR body was corrected to match.