From 8b3d3cc4ba133b99610de00c373159aaff3b98d5 Mon Sep 17 00:00:00 2001 From: SG Command Date: Tue, 15 Sep 2026 05:24:00 +0200 Subject: [PATCH] docs: validate broker recommendations against PostgreSQL --- plan/docs/recommendations.md | 10 ++-- plan/docs/research_validation.md | 90 ++++++++++++++++++++++++++++++++ 2 files changed, 96 insertions(+), 4 deletions(-) create mode 100644 plan/docs/research_validation.md diff --git a/plan/docs/recommendations.md b/plan/docs/recommendations.md index 33b88a3..d8d277a 100644 --- a/plan/docs/recommendations.md +++ b/plan/docs/recommendations.md @@ -3,6 +3,8 @@ **Review date:** 2026-09-14 **Scope:** `pkg/broker/install/sql` tables and functions, including their interaction with the Go worker. +> Validated against authoritative PostgreSQL documentation on 2026-09-15; see [research_validation.md](./research_validation.md) for sources, verification method, and assumptions. Inline notes below marked "(validated: ...)" summarize that document's amendments. + ## Conclusion The current schema is a credible internal prototype, but it is not yet a reliable production job queue. It has correctness problems that can strand jobs, lacks a safe RLS/execution-identity design, and needs standard operational queue features such as leases, retries, and migrations. @@ -13,13 +15,13 @@ It is suitable only for a controlled environment with a single trusted operator | Priority | Problem | Recommendation | | --- | --- | --- | -| Critical | A failed job is marked failed inside `broker_run`, then the function returns a nonzero result. The Go worker rolls back its transaction on nonzero results, so the failed update is rolled back and the job remains `running` forever. | Treat an expected job failure as a committed job outcome, while reserving nonzero function errors for infrastructure failures; alternatively persist the failure in a separate transaction. Add stale-running-job recovery. | +| Critical | A failed job is marked failed inside `broker_run`, then the function returns a nonzero result. The Go worker rolls back its transaction on nonzero results, so the failed update is rolled back and the job remains `running` forever. | Treat an expected job failure as a committed job outcome, while reserving nonzero function errors for infrastructure failures; alternatively persist the failure in a separate transaction. Add stale-running-job recovery. (validated: converting `broker_run` to a `CREATE PROCEDURE` to commit independently only works if the outer `EXCEPTION WHEN OTHERS` block is removed/restructured — PL/pgSQL cannot run `COMMIT`/`ROLLBACK` inside an exception handler.) | | Critical | The notification handler passes a newly pending job directly to `broker_run`, but `broker_run` requires status `running`. The notification attempt fails and polling eventually processes the job. | Use NOTIFY only to wake workers. Every work attempt must claim a job through `broker_get` and `FOR UPDATE SKIP LOCKED`. | | Critical | Arbitrary queued SQL executes with the broker role. This cannot be made safe for multi-tenant/RLS use without an explicit execution model. | Prefer approved job procedures or job types with structured arguments. Add immutable tenant and execution-principal references. Use a least-privileged runtime role that does not own tenant tables and lacks `BYPASSRLS`. | | High | Active-instance registration is race-prone and the Go process can reuse another process's instance ID. | Fail a second startup. Use a session-held PostgreSQL advisory lock for exclusive ownership, with the instance table retained for observability and heartbeats. | | High | Jobs have no retry policy, lease expiry, dead-letter state, idempotency key, attempt counter, or backoff. | Add `attempt_count`, `max_attempts`, `available_at`, `leased_at`, `lease_expires_at`, `lease_token`, and terminal/dead-letter handling. | | High | Dependencies use mutable, non-unique names and treat running, failed, or cancelled dependencies as satisfied. | Replace `depends_on text[]` with `broker_job_dependency(job_id, depends_on_job_id)`. Permit execution only when all dependencies completed successfully, and define behaviour for failed dependencies. | -| High | Schema installation is not idempotent because existing triggers cause reinstallation failures. There is no migration/version tracking. | Use ordered, transactional migrations with a schema-version table. Make trigger creation idempotent or recreate triggers safely. | +| High | Schema installation is not idempotent because existing triggers cause reinstallation failures. There is no migration/version tracking. | Use ordered, transactional migrations with a schema-version table. Make trigger creation idempotent or recreate triggers safely. (validated: `CREATE OR REPLACE TRIGGER` requires PostgreSQL 14+; on earlier versions use `DROP TRIGGER IF EXISTS` followed by `CREATE TRIGGER`.) | | Medium | Job execution is inside the claim transaction. It is atomic but holds locks through arbitrary job SQL and prevents transaction-controlling work. | Explicitly choose a short lease/ack design with idempotent jobs, or document the atomic transaction limitation and restrict job types accordingly. | | Medium | `run_as` is unused. `broker_set` permits persistent session changes and its `search_path` branch is unsafe. | Remove identity controls until fully designed. Use only whitelisted transaction-local settings via `SET LOCAL` or `set_config(..., true)`. | | Medium | Existing indexes do not exactly serve queue claiming by queue, pending status, descending priority, and creation time. | Add and validate a partial claim index: `(job_queue, job_priority DESC, created_at, id_broker_jobs) WHERE complete_status = 0`. | @@ -60,7 +62,7 @@ Use a dedicated `broker` schema and separate roles: 2. A least-privileged broker runtime role, with no `BYPASSRLS` and no ownership of tenant tables. 3. Narrowly scoped enqueue/application roles. -RLS must use immutable `tenant_id` and a transaction-local, trusted tenant context. It must not be driven by arbitrary queued SQL or an unvalidated `run_as` field. +RLS must use immutable `tenant_id` and a transaction-local, trusted tenant context. It must not be driven by arbitrary queued SQL or an unvalidated `run_as` field. If the runtime role ever owns the tenant tables (e.g. because it also ran migrations), it must also run `ALTER TABLE ... FORCE ROW LEVEL SECURITY`, since table owners otherwise bypass RLS by default ([Row Security Policies](https://www.postgresql.org/docs/current/ddl-rowsecurity.html)). ## Delivery order @@ -161,7 +163,7 @@ The leader lock must protect only leader duties such as schedule creation and re ### PostgreSQL operating requirements 1. Use a dedicated, session-persistent connection for `LISTEN`; transaction-pooling proxies cannot safely carry listener state. Use separate pooled connections for claims and execution. -2. Treat `NOTIFY` as a low-latency wake-up only. Its payload is size-limited and notification delivery must not be the sole source of truth; periodic/bounded polling remains necessary. +2. Treat `NOTIFY` as a low-latency wake-up only. Its payload must be shorter than 8000 bytes by default ([NOTIFY](https://www.postgresql.org/docs/current/sql-notify.html)) and notification delivery must not be the sole source of truth; periodic/bounded polling remains necessary. 3. Tune connection-pool sizes per database capacity, rather than multiplying workers and connections without a budget. 4. Monitor queue depth, oldest-ready-job age, claim latency, execution latency, lock waits, dead tuples, autovacuum progress, index size, WAL volume, replication lag, and database connection saturation. 5. Load-test realistic payload sizes, tenant distributions, job durations, retry rates, and failure modes before selecting partitions, worker counts, or autovacuum settings. diff --git a/plan/docs/research_validation.md b/plan/docs/research_validation.md new file mode 100644 index 0000000..0278b12 --- /dev/null +++ b/plan/docs/research_validation.md @@ -0,0 +1,90 @@ +# 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.