docs: validate broker recommendations against PostgreSQL
Integration Tests / integration-test (pull_request) Failing after 25s
Integration Tests / integration-test (pull_request) Failing after 25s
This commit is contained in:
@@ -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.
|
||||
|
||||
Reference in New Issue
Block a user