Skip to content

feat: add pgque-specific schema additions#7

Merged
NikolayS merged 2 commits intomainfrom
feat/3-pgque-additions
Apr 13, 2026
Merged

feat: add pgque-specific schema additions#7
NikolayS merged 2 commits intomainfrom
feat/3-pgque-additions

Conversation

@NikolayS
Copy link
Copy Markdown
Owner

Summary

  • Add sql/pgque-additions/ with pgque-specific schema objects layered on top of transformed PgQ core
  • config.sql: pgque.config singleton table storing ticker_job_id, maint_job_id, installed_at (for pg_cron integration)
  • queue_max_retries.sql: idempotently adds queue_max_retries int4 column to pgque.queue
  • roles.sql: creates pgque_reader, pgque_writer, pgque_admin roles with granular function-level grants matching actual PgQ function signatures
  • lifecycle.sql: pgque.start(), pgque.stop(), pgque.uninstall(), pgque.version() stubs (Sprint 2 pg_cron integration); all SECURITY DEFINER with pinned search_path
  • notify.sql: documents LISTEN/NOTIFY ticker integration plan
  • tests/test_pgque_additions.sql: acceptance tests for all additions

Design notes

  • Role grants use exact function signatures from vendor/pgq/functions/ (e.g., pgque.insert_event(text, text, text), pgque.next_batch_custom(text, text, interval, int4, interval)) rather than wildcards for reader/writer; admin uses grant execute on all functions as catch-all
  • All schema additions are idempotent (create table if not exists, do $$ ... if not exists ..., on conflict do nothing)
  • Follows CLAUDE.md style: lowercase SQL keywords, SECURITY DEFINER functions pin search_path

Test plan

Closes #3

🤖 Generated with Claude Code

Add SQL files for pgque schema objects layered on top of PgQ core:
- config.sql: singleton config table for pg_cron job IDs
- queue_max_retries.sql: add queue_max_retries column to pgque.queue
- roles.sql: pgque_reader/writer/admin roles with granular grants
- lifecycle.sql: start/stop/uninstall/version function stubs
- notify.sql: LISTEN/NOTIFY ticker integration placeholder
- tests/test_pgque_additions.sql: acceptance tests

Closes #3

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@NikolayS
Copy link
Copy Markdown
Owner Author

REV Code Review Report

  • PR: NikolayS/pgque#7 — feat: add pgque-specific schema additions
  • Author: @NikolayS
  • Branch: feat/3-pgque-additions
  • CI: ⚪ no CI configured yet

BLOCKING ISSUES (1)

HIGH sql/pgque-additions/roles.sql:10-11GRANT role TO role not idempotent on PG14/15

grant pgque_reader to pgque_writer;
grant pgque_writer to pgque_admin;

PostgreSQL 14 and 15 raise ERROR: role X is already a member of role Y on duplicate GRANT role TO role. The IF NOT EXISTS clause for role grants was added only in PG16. Since pgque requires PG14+, re-running this file fails on PG14/15.
Fix: Wrap each in DO $$ BEGIN ... EXCEPTION WHEN duplicate_object THEN null; END $$; or guard with a pg_auth_members check.


NON-BLOCKING (2)

MEDIUM sql/pgque-additions/roles.sql:64 + lifecycle.sql:28-29pgque_admin can invoke pgque.uninstall()

The blanket grant execute on all functions in schema pgque to pgque_admin includes uninstall() which runs DROP SCHEMA pgque CASCADE. This may be too permissive.
Suggestion: Add REVOKE EXECUTE ON FUNCTION pgque.uninstall() FROM pgque_admin; after the catch-all grant, or add a superuser guard inside uninstall().

MEDIUM sql/pgque-additions/lifecycle.sql:22-26 — Subtransaction in uninstall()

BEGIN ... EXCEPTION WHEN OTHERS THEN NULL; END; — CLAUDE.md rule 4 discourages subtransactions. Also silently swallows all errors from stop().
Suggestion: Since stop() is currently a stub that just raises NOTICE, remove the exception block. When Sprint 2 implements stop(), it should handle its own errors gracefully.


POTENTIAL ISSUES (2)

LOW tests/test_pgque_additions.sql — Tests only verify existence, not behavior (confidence: 7/10)

Tests check that config table, columns, and roles exist but don't test role inheritance, idempotency, or that grants actually work (e.g., pgque_reader can SELECT but not INSERT). More coverage needed in Issue #5.

LOW sql/pgque-additions/notify.sql — Design note as SQL file with no implementation path (confidence: 5/10)

This is a comment-only file. Consider adding a -- TODO(#2) reference or moving to blueprints/.


Summary

Area Findings Potential Filtered
Security 1 (non-blocking) 0 0
Bugs 1 (blocking) 0 0
Tests 0 1 0
Guidelines 1 (non-blocking) 0 0
Docs 0 1 0

Verdict: Changes requested. Must fix the PG14/15 GRANT role TO role idempotency issue (blocking). The uninstall privilege and subtransaction issues are recommended fixes.


REV-assisted review (AI analysis by postgres-ai/rev)

- Make role grants idempotent on PG14/15 (wrap in exception handler)
- Revoke uninstall() from pgque_admin to prevent accidental schema drop
- Remove subtransaction in uninstall(); stop() handles its own errors

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@NikolayS
Copy link
Copy Markdown
Owner Author

All three review findings addressed in d83fbb6:

  1. Role grants idempotent on PG14/15grant pgque_reader to pgque_writer and grant pgque_writer to pgque_admin are now wrapped in do $$ begin ... exception when duplicate_object then null; end $$ so re-runs don't fail on PG14/15 (which lack IF NOT EXISTS for role grants).

  2. pgque_admin can no longer invoke uninstall() — Added revoke execute on function pgque.uninstall() from pgque_admin after the catch-all grant. Only the schema owner / superuser can drop everything.

  3. Subtransaction removed from uninstall() — Replaced the BEGIN ... EXCEPTION WHEN OTHERS block with a direct perform pgque.stop() call. Added a comment that Sprint 2's stop() must handle its own errors when pg_cron integration lands.

@NikolayS
Copy link
Copy Markdown
Owner Author

REV Re-review

All blocking and recommended fixes verified:

  • ✅ Role grants wrapped in EXCEPTION WHEN duplicate_object for PG14/15 idempotency
  • REVOKE EXECUTE ON FUNCTION pgque.uninstall() FROM pgque_admin added
  • ✅ Subtransaction removed from uninstall(), direct perform pgque.stop() call

Verdict: PASSED. Ready to merge.


REV-assisted review (AI analysis by postgres-ai/rev)

@NikolayS NikolayS merged commit 447960d into main Apr 13, 2026
@NikolayS NikolayS deleted the feat/3-pgque-additions branch April 16, 2026 20:47
NikolayS pushed a commit that referenced this pull request May 3, 2026
The rule is consistently called out in test files (test_api_receive.sql,
test_core_events.sql, test_core_retry.sql, etc.) but invisible in user
docs, client READMEs, and SQL function comments. I hit it myself while
implementing #134 — three operations in one do$$ block silently returned
zero rows on the next receive() because the ticker's snapshot predated
the maint_retry_events insert's commit.

Documents the three operation chains that require separate transactions:
  send/insert_event → ticker/force_tick → receive/next_batch
  maint_retry_events → ticker → receive
  maint_rotate_tables_step1 → maint_rotate_tables_step2

Notes that client default modes (pgxpool, pg.Pool, psycopg autocommit)
satisfy the rule transparently, and flags Pool() / rawPool / client.conn
as the footgun for users who reach for explicit transactions.

User-facing docs (tutorial, examples, reference, client READMEs) still
need the same callout — left as a separate docs-only PR per the audit.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add pgque-specific schema additions (config table, roles, queue_max_retries)

1 participant