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. 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:

• 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: "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: "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 (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 and Advisory Lock Functions:

• 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 (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) 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 (partial indexes via WHERE) and Table Partitioning (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.
2. NOTIFY payload limit: state the concrete 8000-byte default limit and link NOTIFY 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.
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.