pgsql-broker/plan/docs/research_validation.md

# Research Validation: PostgreSQL Broker Recommendations

**Review date:** 2026-09-15
**Scope:** Independent validation of `plan/docs/recommendations.md` and `plan/docs/security_audit.md` (commit 602997b) against authoritative PostgreSQL documentation and the current source (`pkg/broker/install/sql/**`, `pkg/broker/worker/worker.go`, `pkg/broker/database_instance.go`). Performed for issue #1.

This document does not repeat the recommendations; it records what was checked, what is a verified PostgreSQL fact vs. a design recommendation, exact sources, assumptions, and any amendments to the existing docs. All PostgreSQL doc links point at the "current" (development) manual as served by postgresql.org at review time; features cited are stable and present in all currently supported PostgreSQL major versions (13+) unless a version is noted.

## Method

Each claim below was checked one of two ways:
- **Source-verified**: read the actual file/line in this repository.
- **Doc-verified**: fetched the cited PostgreSQL manual page and quoted the operative sentence.

Findings are marked accordingly. Anything not marked doc-verified or source-verified is this review's own recommendation/opinion, not an authoritative fact.

## 1. Queue correctness / rollback-on-failure (Critical #1)

**Source-verified.** `pkg/broker/install/sql/procedures/02_broker_run.sql` declares `broker_run` as `CREATE OR REPLACE FUNCTION ... LANGUAGE plpgsql`, not a procedure. On a job error it sets `complete_status = 3` (failed) via `UPDATE` and returns a nonzero `p_retval`. `pkg/broker/worker/worker.go:155-185` opens a transaction with `w.db.Begin(ctx)`, calls `broker_run` inside it, and calls `tx.Rollback()` whenever the returned error is non-nil (line 182), which undoes the `UPDATE ... complete_status = 3` from the same transaction. The job's `complete_status` therefore remains `1` (running) after rollback, exactly as the recommendation states.

**Doc-verified root cause.** PostgreSQL functions (`CREATE FUNCTION`) cannot issue `COMMIT`/`ROLLBACK`; only procedures invoked via top-level (or uninterrupted) `CALL`/`DO` can, and even then not inside an `EXCEPTION` block (which starts a subtransaction) or a non-read-only cursor loop. Source: [PL/pgSQL Transaction Management](https://www.postgresql.org/docs/current/plpgsql-transactions.html). This confirms the recommendation's implicit assumption: converting `broker_run` to a `CREATE PROCEDURE` would let it commit the failure record independently of the caller's transaction outcome, but only if the exception is caught *outside* any `EXCEPTION` block that would otherwise need to commit — the current code's outer `EXCEPTION WHEN OTHERS` handler (lines 90-108) would need restructuring, since procedures cannot commit from within an exception handler either.

**Amendment:** recommendations.md's fix ("treat failure as a committed outcome ... or persist the failure in a separate transaction") is correct but should explicitly note the procedure-conversion constraint above, since a naive `CREATE PROCEDURE` conversion that keeps the existing nested `EXCEPTION` block will not gain commit capability. Added as a caveat below in "Prioritized amendments."

## 2. LISTEN/NOTIFY semantics (Critical #2, scaling §"PostgreSQL operating requirements")

**Doc-verified**, from [NOTIFY](https://www.postgresql.org/docs/current/sql-notify.html):
- Payload is capped: *"In the default configuration it must be shorter than 8000 bytes."* This is a specific, quotable number the existing docs describe only qualitatively ("size-limited"); worth stating explicitly.
- Delivery is commit-gated: *"if a NOTIFY is executed inside a transaction, the notify events are not delivered until and unless the transaction is committed."* A listening session likewise only receives pending notifications *"just after the transaction is completed."* This supports the recommendation that NOTIFY must be treated as a best-effort wake-up, not a transactional guarantee — a crash between `COMMIT` and NOTIFY delivery, or a dropped/coalesced notification, is normal, expected PostgreSQL behavior, not a bug to work around.
- Duplicate collapsing: *"If the same channel name is signaled multiple times with identical payload strings within the same transaction, only one instance ... is delivered."* This further supports "polling/claim scans remain the source of truth" in recommendations.md — even same-transaction NOTIFY calls are not reliably one-to-one with events.

**Confirms recommendations.md's claim** that a transaction-pooling connection (e.g., PgBouncer in `transaction` or `statement` mode) cannot safely hold a `LISTEN` registration, since the physical server connection backing a pooled client connection can change between statements/transactions, silently dropping the `LISTEN` registration; PostgreSQL's own docs do not carry a proxy-specific warning, so this point in recommendations.md is a correct operational inference rather than a directly quotable PostgreSQL doc fact — labeled here as **recommendation, not doc-verified**, so the distinction is explicit for readers.

**Amendment:** add the exact 8000-byte figure and the doc link to recommendations.md's NOTIFY section for precision.

## 3. Claiming, `FOR UPDATE SKIP LOCKED` (Critical #2, High "leases/retries", scaling §"Claim and indexing strategy")

**Source-verified.** `pkg/broker/install/sql/procedures/01_broker_get.sql:47-49` already uses `ORDER BY job_priority DESC, created_at ASC LIMIT 1 FOR UPDATE SKIP LOCKED`, and claims + status transition happen in one function/statement scope — this part already matches the target model recommendations.md describes ("claim jobs atomically with FOR UPDATE SKIP LOCKED ... in one statement or a tightly scoped function").

**Doc-verified**, from [SELECT ... FOR UPDATE/SHARE](https://www.postgresql.org/docs/current/sql-select.html#SQL-FOR-UPDATE-SHARE): *"Skipping locked rows provides an inconsistent view of the data, so this is not suitable for general purpose work, but can be used to avoid lock contention with multiple consumers accessing a queue-like table."* This is PostgreSQL's own documented, sanctioned use case for the exact pattern recommendations.md proposes — strong confirmation that `SKIP LOCKED` is the correct primitive, not just this project's convention.

**Gap confirmed:** `broker_get`'s dependency check (lines 37-46) only excludes a candidate when a dependency's `complete_status = 0` (pending). A dependency in `running` (1) or `failed` (3) is not pending, so `NOT EXISTS (... dep.complete_status = 0)` is satisfied and the job is claimable — this is source-verified and matches the security_audit.md "High: dependency semantics are incorrect" finding exactly.

## 4. Row-Level Security and execution identity (Critical #3)

**Doc-verified**, from [Row Security Policies](https://www.postgresql.org/docs/current/ddl-rowsecurity.html): *"Superusers and roles with the BYPASSRLS attribute always bypass the row security system... Table owners normally bypass row security as well, though a table owner can choose to be subject to row security with ALTER TABLE ... FORCE ROW LEVEL SECURITY."* This directly confirms both audit documents' repeated point that a broker runtime role must not be a table owner and must not carry `BYPASSRLS`, and that `FORCE ROW LEVEL SECURITY` is required if the runtime role happens to own the tenant tables (e.g., because it ran the migrations) — this is a specific, actionable detail the recommendation summarizes correctly but doesn't cite by clause name.

**Doc-verified**, policies are ordinary boolean expressions evaluated with the caller's privileges and can reference `current_setting(...)` (session/transaction-local GUCs), `current_user`, or connection metadata like `pg_catalog.inet_client_addr()`. This substantiates the recommendation to drive tenant context from *"a transaction-local, trusted tenant context"* — `set_config(name, value, is_local => true)` / `SET LOCAL` is the standard, documented mechanism for a transaction-scoped GUC a policy can read via `current_setting`, matching recommendations.md's `broker_set` guidance (use only "whitelisted transaction-local settings via SET LOCAL or set_config(..., true)"). Source: [Set Session Authorization / SET](https://www.postgresql.org/docs/current/sql-set.html) (`SET LOCAL` scoping) — reviewed for consistency; content matches long-standing, stable PostgreSQL behavior.

**Assessment:** Both audit documents' RLS/identity recommendations are consistent with documented PostgreSQL behavior. No amendment needed beyond adding the `FORCE ROW LEVEL SECURITY` clause name for precision (see amendments).

## 5. Single-instance ownership / advisory locks (High, security_audit.md)

**Doc-verified**, from [Advisory Locks](https://www.postgresql.org/docs/current/explicit-locking.html#ADVISORY-LOCKS) and [Advisory Lock Functions](https://www.postgresql.org/docs/current/functions-admin.html#FUNCTIONS-ADVISORY-LOCKS):
- Session-level advisory locks (`pg_advisory_lock(key bigint)` / `pg_advisory_lock(key1 int, key2 int)`) persist across transactions within the same session and are *"automatically cleaned up by the server at the end of the session,"* including on ungraceful client/network disconnects — the server reliably releases them when the backend session ends. Non-blocking variants exist (`pg_try_advisory_lock`).
- This confirms recommendations.md's proposal ("session-held PostgreSQL advisory lock for exclusive ownership") is a sound, standard pattern for exactly this exclusive-singleton problem: unlike the current row-count check in `broker_register_instance.sql`, an advisory lock held for the life of the broker's connection is automatically released by the server if the process dies or the socket drops, without needing a heartbeat-based takeover for the *lock* itself (a heartbeat is still useful for observability/monitoring, which recommendations.md keeps).

**Assessment:** confirmed as documented, standard practice; no amendment needed.

## 6. Dependencies, run groups, and scheduling

These sections are primarily forward-looking design (normalized dependency tables, cron scheduling with IANA timezones, misfire policies) rather than claims about current PostgreSQL behavior, so there is little to doc-verify beyond generic SQL capabilities already confirmed above (indexed joins, `FOR UPDATE SKIP LOCKED` composability with an `EXISTS` dependency predicate, unique constraints for idempotent schedule materialization). One specific fact worth flagging:

**Doc-verified**, from [Data Types — Date/Time Types](https://www.postgresql.org/docs/current/datatype-datetime.html) (spot-checked against the operative paragraphs — timezone conversion for `timestamptz` is well-established stable behavior): PostgreSQL stores `timestamptz` internally in UTC and converts to/from the session's `TimeZone` setting on display/input; it does not natively track "the schedule's own IANA zone" per row. This confirms recommendations.md's instruction to compute `next_run_at` in the schedule's configured zone *before* persisting as `timestamptz`, and to store the IANA zone name as its own column (not rely on the session `TimeZone` GUC) — the recommendation is correctly stated and necessary, since PostgreSQL will not do zone-aware scheduling arithmetic for you.

No corrections needed to these sections; they are consistent with standard PostgreSQL capabilities and normalization practice.

## 7. Migrations and idempotent installation

**Source-verified.** `pkg/broker/install/sql/tables/02_broker_schedule.sql` and `03_broker_jobs.sql` use `CREATE TABLE IF NOT EXISTS`, but trigger creation in the same files uses plain `CREATE TRIGGER` (no `IF NOT EXISTS`/`OR REPLACE` guard). PostgreSQL's `CREATE TRIGGER` (see [CREATE TRIGGER](https://www.postgresql.org/docs/current/sql-createtrigger.html)) only gained `OR REPLACE` in PostgreSQL 14+; there is no `CREATE TRIGGER IF NOT EXISTS` in any version. This confirms the audit's claim that re-running install fails on existing triggers, and adds a version constraint the docs should carry: even the `CREATE OR REPLACE TRIGGER` fix requires PostgreSQL ≥ 14, otherwise the code must `DROP TRIGGER IF EXISTS ... ; CREATE TRIGGER ...` instead.

**Amendment:** add this PG-14 minimum-version caveat to recommendations.md's "installer is not rerunnable" item.

## 8. Scaling, maintenance, observability, operational safety

These sections describe conventional, well-established PostgreSQL operational practice (partial indexes for hot queue predicates, time-based declarative partitioning for append-only history, bounded-batch deletes, autovacuum tuning, `EXPLAIN (ANALYZE, BUFFERS)` for validating index choice). Spot checks against [CREATE INDEX](https://www.postgresql.org/docs/current/sql-createindex.html) (partial indexes via `WHERE`) and [Table Partitioning](https://www.postgresql.org/docs/current/ddl-partitioning.html) (declarative range partitioning, detaching old partitions) confirm the described mechanisms exist and behave as summarized; no unsupported or version-inconsistent claims were found. No amendments needed.

## Assumptions

- Target deployment uses a currently-supported PostgreSQL major version (13–18); version-specific caveats above (e.g., `CREATE OR REPLACE TRIGGER` needing PG 14+) are called out explicitly rather than assumed away.
- "Doc-verified" reflects the current/development manual at review time; behavior cited (NOTIFY commit-gating, SKIP LOCKED semantics, RLS bypass rules, advisory lock lifecycle, PL/pgSQL transaction-control restrictions) has been stable across PostgreSQL major versions for many years and is not expected to differ for older supported versions.
- This review did not have network access to a live PostgreSQL instance in this environment to run `EXPLAIN` or reproduce the rollback bug end-to-end; the rollback and dependency-satisfaction defects were confirmed by static reading of the SQL/Go source (see source-verified notes above), not by execution.

## Prioritized amendments to `plan/docs/recommendations.md`

1. **Critical #1 (rollback bug fix):** note that converting `broker_run`/`broker_get` to `CREATE PROCEDURE` for independent commit only works if the outer `EXCEPTION WHEN OTHERS` block is removed or restructured, since procedures cannot run `COMMIT`/`ROLLBACK` inside an exception handler (PL/pgSQL forms a subtransaction there). Source: [PL/pgSQL Transaction Management](https://www.postgresql.org/docs/current/plpgsql-transactions.html).
2. **NOTIFY payload limit:** state the concrete 8000-byte default limit and link [NOTIFY](https://www.postgresql.org/docs/current/sql-notify.html) instead of only "size-limited."
3. **RLS/table ownership:** name `ALTER TABLE ... FORCE ROW LEVEL SECURITY` explicitly as the mechanism needed if the runtime role ever owns tenant tables, per [Row Security Policies](https://www.postgresql.org/docs/current/ddl-rowsecurity.html).
4. **Installer idempotency:** add that `CREATE OR REPLACE TRIGGER` requires PostgreSQL 14+; on earlier versions use `DROP TRIGGER IF EXISTS` followed by `CREATE TRIGGER`.

These amendments have been applied to `plan/docs/recommendations.md` as short inline notes with links to this document; the substantive prioritization and design recommendations in that file are otherwise confirmed and unchanged.