Skip to content

fix: backport receive safety to 0.2.1 - #367

Merged
NikolayS merged 5 commits into
maintenance-0-2from
release-0-2-1
Oct 1, 2026
Merged

NikolayS merged 5 commits into
maintenance-0-2from
release-0-2-1

Conversation

@NikolayS

@NikolayS NikolayS commented Oct 1, 2026 •

Copy link
Copy Markdown
Owner

Summary

Refs #365. Backport the complete-batch-or-error receive fix to the latest stable release, v0.2.0, and prepare v0.2.1. This branch deliberately excludes v0.3 alpha features.

  • Guard plain and cooperative receives against silent truncation, with actionable overflow errors.
  • Backport regression tests for boundary values, consumer-state rollback and cooperative takeover.
  • Correct reference, examples and client guidance; ticker thresholds are not hard batch-size caps.
  • Bump server version and rebuild SQL/pg_tle installers; client package versions remain unchanged because this is a server-only fix.
  • Enable the existing CI suite for this maintenance branch.

Real pagination remains separate in #364.

Verification

Commands for a fresh database:

bash build/transform.sh
psql -X -v ON_ERROR_STOP=1 -d "$db_name" -f sql/pgque.sql
psql -X -v ON_ERROR_STOP=1 -d "$db_name" -f tests/run_all.sql
psql -X -v ON_ERROR_STOP=1 -d "$db_name" -f tests/acceptance/run_acceptance.sql

Required before merge/tag: full existing PG14–18 CI and integration/client jobs, exact-head SamoRev review, actual v0.2.0 upgrade/reapply with retained queue state and complete event recovery. Evidence will be posted on this PR. No package publishing or benchmarks required for the SQL-only fix.

Intentional scope

The owner requested fail-closed overflow, preserving existing ceiling defaults/signatures. An undersized consumer must stop with an observable error until its operator selects a sufficient resource-safe ceiling; automatic ceiling enlargement is deliberately out of scope. Real pagination remains #364. Overflow uses SQLSTATE 54000 so clients can classify the resource-limit condition without matching message text.

Both plain SQL and pg_tle populated-upgrade paths must preserve queue data before this maintenance release is published. The release tag will be cut from this maintenance line, not from v0.3 development.

pg_tle update verification

The v0.2.0→v0.2.1 wrapper now registers a function-only update edge via the documented pg_tle install_update_path API and sets the new default version. PostgreSQL can create the default version through the existing base install plus that edge; a redundant standalone install_extension_version_sql registration is not required. This was executed on pg_tle v1.5.2: both an in-place populated ALTER EXTENSION UPDATE and a fresh CREATE EXTENSION after registration pass. The regression also reruns wrapper registration, preserves active subscription state, and verifies both plain/cooperative overflow return SQLSTATE54000 followed by successful full recovery. The same test is wired into CI, with tagged v0.2.0 fetched via full checkout history.

The overflow SQLSTATE change was red/green tested (old P0001 fails the new assertion). Partitioned receive is absent from the 0.2 line; its audit and tests apply only to development PR #366. INT_MAX now explicitly checks zero returned rows and no active batch.

Draft GitHub release notes

The following notes will be published with the immutable maintenance tag after all gates pass.

Complete-batch receive safety

PgQue v0.2.1 is a maintenance release backported to v0.2.0. It does not include the v0.3 alpha features.

pgque.receive() and pgque.receive_coop() now return the complete batch or raise an error when the batch exceeds max_return. Exactly max_return events succeeds; an additional event raises before the call can return a successful partial result. The failed statement rolls back its consumer allocation/ownership changes. Overflow uses SQLSTATE 54000 (program_limit_exceeded) and includes recovery guidance.

This fixes a data-loss footgun: earlier receives could return only part of a batch, while ack(batch_id) finished the entire batch and skipped unreturned events. ack() remains a whole-batch acknowledgment; applications must process every event before calling it.

Compatibility and recovery

Before upgrading: the default SQL ceiling is 100, below the default ticker event-count threshold of 500. Ordinary batches can exceed 100. Direct Python/TypeScript receives and Go ReceiveCoop also default to 100; callers relying on these defaults must pass a sufficient resource-safe ceiling before upgrading. Alert on SQLSTATE 54000 and growing consumer lag. High-level consumer loops default to the Postgres integer maximum.

Older servers can silently truncate receive results; install this server update before relying on overflow protection.

  • Applications using a ceiling smaller than an actual batch will now receive an error. Roll back a failed transaction, then retry with a sufficient ceiling within your resource budget. Never acknowledge after the failed receive.
  • ticker_max_count is a tick-trigger threshold, not a maximum batch size. Repeated receives with an undersized ceiling are not pagination. Monitor overflow errors and consumer lag.
  • No client-library update is required for this server-side fix. Python, Go and TypeScript package versions remain unchanged.
  • Reference documentation, examples and client guidance are corrected. A proper paged-batch protocol is tracked separately in #364.

Install or upgrade the plain SQL installation

Use the installer from this tag, as its schema owner or a superuser:

psql --single-transaction -v ON_ERROR_STOP=1 -d mydb -f sql/pgque.sql

For a v0.2.0 plain SQL installation, this replaces functions while preserving queues, consumer positions, active batches, retry/DLQ rows and queue configuration. Reapplication is supported.

select pgque.version(); -- 0.2.1

Upgrade a pg_tle installation

Register the new version and its non-destructive update path, then update the installed extension:

psql -v ON_ERROR_STOP=1 -d mydb -f sql/pgque-tle.sql
psql -v ON_ERROR_STOP=1 -d mydb -c "alter extension pgque update to '0.2.1';"

The update replaces only the receive functions and version function. Do not uninstall a populated extension to apply this fix. The wrapper also supports fresh installations.

Verification

Release-gate evidence and SamoRev review are posted on the maintenance PR. The tests cover N-1/N/N+1, integer-maximum ceilings, complete retry payloads, consumer-state rollback, cooperative stale-worker takeover, fresh installation and populated v0.2.0 upgrade/reapplication.

Fixes #365.

Refs #365. Reject oversized plain and cooperative batches, correct documentation, and retain the 0.2 stable feature set.
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

Refs #365. Backport complete-batch retry assertions.
@NikolayS

NikolayS commented Oct 1, 2026 •

Copy link
Copy Markdown
Owner Author

samorev Code Review Report

Reviewed head: c167cfd93bc8353ac1333822db07591ddb0fe4f1 (round 1).

Pipeline Coverage
PASS Not reported

BLOCKING ISSUES (4)

MEDIUM [docs] The v0.2.0 → v0.2.1 upgrade note says to "re-run the installer", but the pg_tle installer can't do that. sql/pgque-tle.sql skips only when the registered version is exactly 0.2.1. For any other registered version, including 0.2.0, it raises and tells the user to run sql/pgque-tle-uninstall.sql first. No pgtle.install_update_path('pgque','0.2.0','0.2.1', ...) is provided. So pg_tle users get no in-place upgrade, and the guide doesn't warn that uninstalling or re-creating the extension may lose queue state. That contradicts "does not change tables or queue state".

if existing_version = '0.2.1' then ... skipping / 'Run sql/pgque-tle-uninstall.sql first to remove the existing registration' vs docs/upgrading.md: "Re-run the installer using the command above ... does not change tables or queue state."
Fix: Add a pg_tle update path from 0.2.0 to 0.2.1 (pgtle.install_update_path plus alter extension pgque update), or document a separate pg_tle procedure in docs/upgrading.md that keeps state. Include the pg_tle path in the "actual v0.2.0 upgrade" verification.

MEDIUM [bugs] Every overflow raises the generic P0001 (raise_exception) error code. Clients and consumer loops can only tell an overflow apart from other P0001 errors by matching the message text, and the new Go and Python tests do exactly that. Without a distinct error code, an automatic "retry with a larger ceiling" policy is fragile. For the same reason, a Consumer with a fixed max_messages that keeps hitting the same oversized batch is hard to alert on separately; it fails on every poll and the consumer stalls.

raise exception 'pgque.receive: batch exceeds max_return of %', i_max_return using hint = ... (no errcode)
Fix: Add a dedicated error code, for example using errcode = 'PQ001' or a standard one such as '54000' (program_limit_exceeded). Add a detail with the batch_id. Document the code in reference.md so clients can match on SQLSTATE instead of the message.

LOW [tests] The "Exactly N" section of tests/test_receive_overflow.sql is split up and partly unchecked. The recv_nminus1 setup, tick and assertions sit between recv_exact's setup and its tick, and the global pgque.ticker() call in that block may tick recv_exact too. The comment also says the test covers "the largest int ceiling on an empty batch", but that call (perform * from pgque.receive('recv_exact','c1',2147483647)) has no assertion: nothing checks that it returns zero rows or that the empty batch was finished.

-- Exactly N remains valid, including the largest int ceiling on an empty batch. ... perform * from pgque.receive('recv_exact', 'c1', 2147483647);
Fix: Put the recv_exact setup, tick and checks back together. Count the rows from the INT_MAX call and assert the count is 0, and that sub_batch is null afterward.

LOW [docs] The reference tells users they can "use the low-level full-batch interface" but never names it (for example next_batch + get_batch_events + finish_batch). The cooperative section also says an oversized batch "rolls back allocation or takeover" but doesn't say that, without an explicit savepoint, the caller's whole transaction is aborted.

or use the low-level full-batch interface.
Fix: Name and link the low-level functions. Add a note that the error aborts the enclosing transaction unless the call is wrapped in a savepoint.


Summary

Area Findings Potential Filtered
CI/Pipeline 0 0 0
Security 0 0 0
Bugs 0 1 0
Tests 0 1 0
Guidelines 0 0 0
Docs 0 2 0
Metadata 0 0 0

Note:

  • Findings: High-confidence issues (8-10/10) - blocking or non-blocking per severity
  • Potential: Medium-confidence issues (4-7/10) - review manually
  • Filtered: Low-confidence issues (0-3/10) - excluded as likely false positives
Review metadata
provider=github
kind=pr
project=NikolayS/PgQue
number=367
target=github:NikolayS/PgQue#367
state=OPEN
draft=false
diff_lines=943
diff_added=512
diff_removed=46
diff_bytes=40072
comments_count=1
commits_count=2
ci_status=success
ci_summary=total=28 success=28 failure=0 pending=0 other=0
prompt=.claude/commands/review-mr.md
blocking=true
samorev_review=1
review_round=1/10
escalated=false
posted_by=local
no_comment=true
live_posting=not-run
defender_dropped=1

Contested / dropped

  • [tests] test_receive_overflow.sql is included without its own \echo 'Running: ...' line. Every other suite in run_all.sql has one, so if this suite fails, the CI log will attribute the failure to test_api_receive. — The misattribution claim is wrong. When psql runs a script through -f/\i, it puts the failing file and line in front of each error (e.g. psql:tests/test_receive_overflow.sql:42: ERROR:), so the CI log already names the suite that failed. A missing \echo only affects how consistent the log looks.

samorev-assisted review (AI analysis by Tanya301/samorev)

Refs #365. Register a function-only update edge, classify overflow as SQLSTATE54000, and tighten regression assertions.
@NikolayS

NikolayS commented Oct 1, 2026

Copy link
Copy Markdown
Owner Author

samorev Code Review Report — full AI review

Reviewed head: 22e0b240e18c60eb46f7f525fff97bb4460a96f3 (round 2, full multi-agent review; base maintenance-0-2 @ e8ee488d2c1d87ab09eed2581ec4bbc1e68315f6).

Pipeline Coverage
✅ success — 28/28 check runs on the exact head SHA (PG 14–18, v0.1.0→HEAD upgrade, pg_tle, pg_cron, pg_timetable, Go/Python/TypeScript clients, verify) Not reported

Verdict: ❌ CHANGES REQUESTED — 2 blocking issues. This is not a PASS.


BLOCKING ISSUES (2)

MEDIUM docs/upgrading.md:15-27 - The upgrade notes don't warn that the default ceiling of 100 is itself undersized. (confidence 8/10; Bugs + Docs + round-2 CLI gate — previously flagged, still unresolved)

  • receive() and receive_coop() still default max_return to 100.
  • The same default of 100 applies to the direct client calls: Python receive/receive_coop (clients/python/pgque/client.py:222, :396), TypeScript receive/receiveCoop (clients/typescript/src/client.ts:159, :409) and Go ReceiveCoop (clients/go/options.go:120).
  • The ticker cuts a batch at queue_ticker_max_count = 500 events or queue_ticker_max_lag = 3 seconds (sql/pgque.sql:110-111). That is a trigger threshold, not a cap.
  • So any queue running above roughly 34 events/s routinely produces batches larger than 100. After this upgrade, every default-argument caller gets SQLSTATE 54000. Retrying with the same ceiling can never succeed, so the consumer stalls, lag grows, and the stalled consumer also holds back event-table rotation.
  • The upgrade section says "Applications do not need client-library updates" and mentions only "an undersized receive ceiling". The reference, README and examples still show pgque.receive('orders', 'processor', 100) as the example call.
  • The high-level Consumers (Go, Python, TS) default to 2147483647 and are not affected.

Fix: Changing the default stays out of scope, since keeping defaults was the owner's choice. Instead:

  • In the v0.2.0→v0.2.1 section, add an explicit pre-upgrade warning: the default max_return of 100 (SQL and the direct client calls listed above) is below normal burst batch sizes. Callers relying on the default must pass an explicit, sufficient ceiling before upgrading.
  • Tell operators to alert on SQLSTATE 54000 and on consumer lag.
  • Stop presenting 100 as a safe example value, or add a caveat next to it.

MEDIUM clients/typescript/README.md:77 (+ clients/typescript/src/client.ts:152-158,376-378, clients/typescript/src/types.ts:59-68, clients/go/options.go:132, clients/python/pgque/client.py:229-233) - The TypeScript client docs and the Go/Python in-code docs still describe the old truncation behavior. (confidence 8/10; Docs 9 + Bugs 6)

  • TS README: "Fetch up to max (default 100)… PgQue finishes the whole underlying batch, including rows beyond max".
  • types.ts: "unreturned rows are skipped after ack".
  • Go WithCoopMaxMessages comment: "a low limit can therefore drop rows. Match this to ticker_max_count".
  • Python receive docstring: "including rows beyond max_messages".

These are the docstrings and README rows users actually read, and they now contradict the server. Issue #365 and the PR description both list "correct … client guidance" in scope, but only the Go and Python READMEs were updated. The Go and Python READMEs also document the coop default of 100 (clients/go/README.md:242, clients/python/README.md:145-147) without mentioning overflow.

Fix: Rewrite these strings to match the updated Go/Python README wording: max is a complete-batch ceiling; an oversized batch raises 54000; roll back and retry with a larger ceiling; never ack after the error. Drop the "match ticker_max_count" advice.


NON-BLOCKING (1)

LOW PR description - The description mentions a test that isn't in this PR. (confidence 8/10; Metadata)

Suggestion:

  • The description says "The multi-slot partition test establishes that the probe sees only matching-slot rows". No partition test is in this diff, and the 0.2 line has no partitioned receive (only pgque.receive and pgque.receive_coop exist), so the text appears to have been carried over from the development-line PR. Remove it, and state explicitly that the "audit partitioned receive" item from issue fix: reject truncated receive batches #365 does not apply to 0.2.
  • Separately, the description says the actual v0.2.0 upgrade/reapply evidence "will be posted on this PR". It has not been posted yet. CLAUDE.md step 3 requires that evidence before merge.

POTENTIAL ISSUES (12)

These have moderate confidence (4–7/10). Review them manually; some may be false positives.

MEDIUM sql/pgque-tle.sql:937-941 / build/transform.sh Step 3 / docs/upgrading.md:30-41 - pg_tle users registered at any version other than 0.2.0 have no documented path. (confidence 7/10; Docs + Bugs)

The old raise pointed at sql/pgque-tle-uninstall.sql. The new message only says "this script only supports a managed update from 0.2.0 to %". Users registered at 0.1.x or 0.2.0-rc.x (both tags exist) hit a dead end, and the obvious workaround (uninstall plus drop extension) destroys queue state.
Suggestion: In upgrading.md, state that the managed pg_tle update supports exactly 0.2.0 only. For other registered versions, give the procedure and an explicit state-loss warning, and put a pointer to it back in the exception text.

MEDIUM .github/workflows/ci.yml - No CI job covers a plain-SQL v0.2.0→HEAD reinstall over a populated database. (confidence 6/10; Tests)

The plain-SQL claims — "re-run the installer… replaces functions only; does not change tables or queue state", and that the 54000 guard works on a batch already allocated under 0.2.0 — are untested in CI. Only upgrade v0.1.0 → HEAD and the new pg_tle 0.2.0 test exist.
Suggestion: Add a v0.2.0→HEAD job, or parametrize the existing one. Install v0.2.0:sql/pgque.sql, leave an active batch with 3 events, reinstall HEAD, then assert: state unchanged, receive(...,2) raises 54000, receive(...,3) returns all 3. Also run test_upgrade_grants.sql. Posting manual evidence on the PR would also satisfy CLAUDE.md step 3.

LOW build/transform.sh:60-62,145 - The pg_tle update generation is hard-wired to 0.2.0→0.2.1. (confidence 5/10; Bugs)

On the next version bump the build still succeeds. It emits pgque--0.2.0--0.2.1.sql with a newer version(), the '${PGTLE_VERSION}' = '0.2.1' branch silently goes dead, and 0.2.1-registered databases hit the raise.
Suggestion: Fail the build when PGTLE_VERSION != 0.2.1 until the update tooling is generalized, or derive source and target versions.

LOW tests/run_all.sql:1026 - The new suite is included without an \echo 'Running: test_receive_overflow' line, unlike every other entry. (confidence 5/10; Guidelines + Tests + Bugs; round-2 CLI gate — still unresolved)

psql already prefixes errors with file:line, so failures stay traceable. This is a consistency issue, not a misattribution bug.
Suggestion: Add the \echo line.

LOW clients/typescript/test/client.test.ts - The TypeScript client has no overflow integration test, unlike Go and Python. (confidence 5/10; Tests)

Nothing verifies that SQLSTATE 54000 propagates through the TS error wrapper or that a complete retry works.
Suggestion: Add a DB-gated test for receive and receiveCoop: ceiling 3 on 5 events rejects with 54000; ceiling 5 returns 5 messages with one batch id; ack succeeds.

LOW tests/test_tle_upgrade_v0_2.sql:1556 - Two branches of the pg_tle wrapper are untested. (confidence 5/10; Tests)

(a) The unsupported-version branch: a version other than 0.2.0/0.2.1 is registered, so the wrapper must raise. (b) The update path is already registered but the default version is still 0.2.0. The double \i only exercises the early 0.2.1 return.
Suggestion: Add scratch-database cases for both. Optionally compare pg_get_functiondef for receive, receive_coop and version between an updated database and a fresh 0.2.1 create.

LOW build/transform.sh header / sql/pgque-tle.sql \echo / README.md pg_tle section - The upgrade usage isn't documented where users look first. (confidence 5/10; Docs)

The generated header's Usage section lists only install and uninstall. The final \echo ("apply ALTER EXTENSION UPDATE as described in the upgrade documentation") is vague, and it also prints on no-op reruns and fresh installs. The README's pg_tle section doesn't link to docs/upgrading.md.
Suggestion: Print the exact command, alter extension pgque update to '0.2.1';, only on the 0.2.0 branch, and add the README link.

LOW tests/test_receive_overflow.sql:1214,1437 - Some overflow tests catch when others without checking SQLSTATE. (confidence 4/10; Tests)

The already-allocated-batch and stale-takeover cases accept any error. Separately, receive_coop has no tests for exactly-N, NULL or INT_MAX, and plain INT_MAX is tested only on an empty batch.
Suggestion: Use when sqlstate '54000' and add the missing coop boundary cases.

INFO clients/python/README.md:78, clients/go/README.md:96 - These READMEs use version-history wording ("Older servers can truncate… upgrade before relying on the guard", "Servers with the overflow guard"). (confidence 5/10; Guidelines)

CLAUDE.md says README and docs describe current behavior and that migration notes belong in release notes.
Suggestion: State the current behavior and move the old-server caveat to the CHANGELOG or release notes. This may conflict with the first blocking finding's need for an upgrade warning; resolve that by putting the warning in upgrading.md and the release notes.

INFO docs/upgrading.md:15-27 - The new section uses "now… rather than returning a truncated result" history framing. (confidence 4/10; Guidelines)

The per-version heading itself follows the precedent of the existing "v0.1.0 to v0.2.0" section, so this is low priority.

INFO .github/workflows/ci.yml:12,15 - The short-lived branch release-0-2-1 is hardcoded in the CI triggers. (confidence 4/10; Guidelines)

Suggestion: Use release-*, or rely on maintenance-0-2 plus workflow_dispatch.

INFO Overall - The dollar-quote collision guard in transform.sh scans only pgque.sql, not the generated update body. (confidence 4/10; Security; downgraded by the orchestrator)

The update body is extracted from the same sources compiled into pgque.sql (verified byte-identical), and its fixed header contains no dollar-quote tags, so the guard covers it indirectly today.
Suggestion: Run the same grep -qF loop over ${PGTLE_UPDATE_FILE} as defense in depth.


Summary

Area Findings Potential Filtered
CI/Pipeline 0 0 0
Security 0 1 0
Bugs 1 (merged w/ Docs) 2 0
Tests 0 5 0
Guidelines 0 3 1
Docs 1 2 0
Metadata 1 0 0

Note: "Findings" are high-confidence (8–10/10) issues, "Potential" are moderate-confidence (4–7/10) and "Filtered" are 0–3/10. Duplicate findings from different agents were merged into one row at the highest validated confidence. The table counts each consolidated finding once, under its primary area.

Filtered (1): Guidelines — "PR bundles fix + pg_tle update path + version bump". Issue #365 explicitly requires the backport, release prep, and installation/upgrade tests in this change, so the bundling is in scope (validated 3/10).

Issue #365 requirements check

Requirement Status
Fail closed on N+1; exactly N valid; integer maximum must not overflow ✅ Implemented (cnt = i_max_return check runs before the increment; NULL = no ceiling) and tested (N-1/N/N+1, NULL, INT_MAX on an empty batch)
Preserve consumer state on failure; retry, coop allocation and takeover ✅ Tested (rollback of sub_batch/sub_last_tick/sub_next_tick, active batch, coop allocation, stale takeover)
Correct reference, examples and batch-size guidance ⚠️ Partial — see both blocking findings (default-100 warning; stale TS/Go/Python in-code docs)
Backport only this fix to v0.2.0 line; release v0.2.1 after CI, SamoRev and real install/upgrade tests ⚠️ CI is green and this review is complete. Real v0.2.0 upgrade evidence is not yet posted on the PR. Plain-SQL v0.2.0→HEAD is not in CI. Function delta vs v0.2.0 verified: only receive, receive_coop and version changed, matching the update body exactly
Audit partitioned receive (no pagination) N/A on the 0.2 line (no partitioned receive exists). The description should say so instead of citing a partition test (see Non-blocking)

Validation of the round-2 deterministic CLI gate (pr-367-samorev-gate.md)

  • MEDIUM [docs] default max_return 100: Confirmed and widened. It also affects the direct client calls, not just SQL callers, and it holds back rotation. Promoted to blocking at 8/10.
  • LOW [guidelines] run_all.sql missing \echo: Confirmed as a consistency issue. The misattribution claim is weak because psql prefixes errors with file:line. Kept as potential at 5/10.
  • Defender-dropped: when others in test_api_receive step 6: agree with the drop. The SQLSTATE is asserted elsewhere. A related, different weakness in test_receive_overflow.sql is kept as potential.
  • Defender-dropped: hardcoded /tmp/pgque-tle-v0.2.0.sql: agree with the drop. The precondition is documented in the test header and the test runs only in CI.
  • Round 1 (head c167cfd): the pg_tle update path, SQLSTATE 54000, the "Exactly N" test structure and the naming of the low-level functions plus the transaction-abort note are all resolved at this head.

Specialized agent results (all 5 completed)

Agent 1 — Security Reviewer (opus): 1 finding, LOW, confidence 4
  • LOW (4) build/transform.sh:1003: the dollar-quote collision check scans only INSTALL_FILE, not the new PGTLE_UPDATE_FILE, and runs before that file is generated. This is a missing guard, not a live exploit.
  • Checked clean:
    • Update via create or replace keeps owner and ACLs. Signatures are identical to v0.2.0, so there is no new PUBLIC execute. The grants/revokes carry through both ALTER EXTENSION UPDATE and fresh create via base plus update.
    • All SECURITY DEFINER functions keep set search_path = pgque, pg_catalog.
    • PGTLE_VERSION is regex-validated before interpolation. No SQL or command injection.
    • CI uses pull_request, not pull_request_target. fetch-depth: 0 only reads the trusted tag. No untrusted ${{ }} in run: steps.
  • Coverage: full diff, plus v0.2.0 function signatures, grants and bodies. The agent could not regenerate artifacts (sandbox); the orchestrator did that instead (see Coverage).
Agent 2 — Bug Hunter (opus): 3 findings (MEDIUM 7, LOW 6, LOW 5) + run_all re-flag
  • MEDIUM (7) docs/upgrading.md:24: the default ceiling of 100 will error under ordinary load, for SQL callers and the direct client calls alike. Batches grow past 100 above about 34 events/s at the 3 s lag default. Default callers stall and hold back rotation. The upgrade notes say client updates are "not needed". Previously flagged, still unresolved.
  • LOW (6) clients/typescript/src/types.ts:62 (+ TS README and JSDoc, Go options.go, Python client.py docstrings): in-code docs still describe truncate-then-skip.
  • LOW (5) build/transform.sh:62: the update generation is hard-wired to 0.2.0→0.2.1. The next bump goes silently stale, and the raise gives no remedy for other registered versions.
  • LOW (5) tests/run_all.sql:87: missing \echo. Previously flagged, still unresolved.
  • Checked clean:
    • SRF semantics: rows are collected before return, so a raise after return next sends zero rows. No next_batch side effect survives the rollback (only sequence gaps).
    • NULL and INT_MAX handling.
    • The v0.2.0→head SQL delta is only the header, version(), receive() and receive_coop(), and the update body is byte-identical to those functions.
    • Generated files reproduce byte-for-byte.
    • All wrapper branches.
    • CI tag availability.
    • Go pgque.SQLError.SQLSTATE exists. Python raise … from e sets __cause__.
  • Coverage: full diff. pg_tle API behavior is not executed locally; it relies on the CI pg_tle job, which passed.
Agent 3 — Test Analyzer (sonnet): 5 findings (MEDIUM 6, LOW 6, LOW 5, LOW 5, LOW 4)
  • MEDIUM (6) .github/workflows/ci.yml: no plain-SQL v0.2.0→HEAD populated reinstall job.
  • LOW (6) tests/run_all.sql:1026: missing \echo. Previously flagged, still unresolved.
  • LOW (5) clients/typescript/test/client.test.ts: no TS overflow/54000 integration test. The old truncation behavior isn't replaced with a new assertion.
  • LOW (5) tests/test_tle_upgrade_v0_2.sql: the unsupported-version branch and the pre-registered-path/default-not-set branch are untested.
  • LOW (4) tests/test_receive_overflow.sql:1214,1437: when others without a SQLSTATE check. No coop exactly-N, NULL or INT_MAX cases. Plain INT_MAX tested only on an empty batch.
  • Checked clean: round-1 "Exactly N" structure fixed. @> assertions are paired with count = 3. Global ticker() calls cause no isolation problem. Fresh 0.2.1 pg_tle install is covered by test_tle_install.sql plus the upgrade test's drop/create.
Agent 4 — Guidelines Checker (sonnet): 5 findings (all INFO, confidence 4–5)
  • INFO (5) tests/run_all.sql:87: missing \echo (consistency issue).
  • INFO (5) clients/python/README.md:78, clients/go/README.md: "Older servers can truncate" history framing (CLAUDE.md Documentation rule).
  • INFO (4) docs/upgrading.md:15: "now… rather than" migration framing. The version heading has precedent.
  • INFO (4) .github/workflows/ci.yml:6: short-lived branch release-0-2-1 hardcoded in triggers.
  • INFO (4) build/transform.sh:1033: PR bundles fix, update path and version bump ("surgical" rule). Filtered by the orchestrator because issue fix: reject truncated receive batches #365 requires this scope.
  • Checked clean:
    • Lowercase, left-aligned SQL.
    • SECURITY DEFINER search_path pinned.
    • transform.sh shell style.
    • Copyright and PgQ attribution in the new files.
    • Conventional Commits with subjects under 50 characters (cosmetic typo "SQLSTATE54000" in the 22e0b24 body).
  • SOC2 ignored per PgQue CLAUDE.md. Org rules were loaded from postgres-ai/rules through the GitLab API (25 .mdc files; the submodule is absent in this checkout).
Agent 5 — Docs Reviewer (sonnet): 4 findings (MEDIUM 8, MEDIUM 9, MEDIUM 7, LOW 5)
  • MEDIUM (8) docs/upgrading.md / reference.md / README / tutorial / examples: the round-2 default-100 finding holds. SQLSTATE 54000 is not named as the alert signal.
  • MEDIUM (9) clients/typescript/README.md:77 + TS JSDoc/types; Go WithCoopMaxMessages and Python coop default-100 entries: stale truncation semantics.
  • MEDIUM (7) docs/upgrading.md:58-69: no path for pg_tle registrations other than 0.2.0. Vague, always-printed \echo. No verification query for the update path.
  • LOW (5) build/transform.sh:1062-1090: generated header Usage lacks the upgrade flow. The README pg_tle section doesn't link to upgrading.md.
  • Checked clean:
    • receive() does auto-finish empty batches (the new reference claim is accurate).
    • Round-1 doc items fixed.
    • The tutorial's receive(…, 1) limit 1 blocks are safe (one-event batches).
  • Minor gap folded into the blocking item: 54000 is named only in the receive section, not under receive_coop.

Not applicable: Agent 6 (Sqitch; postgres-ai/platform only) and Agent 7 (Visual evidence). The changed files include no UI or visual paths; only Markdown/SQL/Go/Python/YAML/Bash changed, so IS_VISUAL=0 and the GitHub file list was not truncated (21/21 files).


Coverage and limitations

  • Diff: the full diff was read in full: 21 files, +1120/−93, 1674 lines (merge-base e8ee488, matching gh pr diff). No reviewer worked from clipped context. Source files were inspected directly at head 22e0b24 where needed.
  • Generated artifacts: the orchestrator re-ran build/transform.sh on a clean git archive of the head in a temp directory. sql/pgque.sql, sql/pgque-tle.sql and sql/pgque-tle-updates/pgque--0.2.0--0.2.1.sql match byte-for-byte.
  • Not executed by this review: the SQL regression suite, the pg_tle upgrade test and the Go/Python/TS client tests were not run locally. For runtime behavior this review relies on the 28/28 green CI checks on the exact head. The plain-SQL v0.2.0→HEAD reinstall over populated state is covered neither by CI nor by this review (see Potential).
  • Validation (Step 5.5): done by the orchestrator, which checked each finding against source (client defaults, options.go:132, client.py:229, ticker defaults at sql/pgque.sql:110-111, run_all.sql, TS DEFAULT_MAX_MESSAGES = 2_147_483_647). Separate validator subagents were not used.
  • SOC2: skipped, as PgQue CLAUDE.md requires.
  • Completeness: all five specialized reviewers completed. No reviewer timed out or was incomplete.

samorev-assisted review (AI analysis by Tanya301/samorev); full multi-agent /review-mr run, not the deterministic CLI gate alone. Exit status: BLOCKING (1).

Resolve review findings on SDK documentation and default ceilings. Refs #365.
@NikolayS

NikolayS commented Oct 1, 2026

Copy link
Copy Markdown
Owner Author

samorev Code Review Report — round 3 (delta review)

Reviewed head: dc0b698afce03bd4d9043d9a75b6d30584e78253

Scope: delta review. This round covers the changes from the previous full-source review at 22e0b240e18c60eb46f7f525fff97bb4460a96f3 (round 2) to the head above. It is not a fresh full-source review of the PR. See the Coverage section for exactly what is newly reviewed and what is carried forward from round 2.

Pipeline Coverage
✅ success: 28/28 check runs on dc0b698 (PG 14–18, v0.1.0→HEAD upgrade, pg_tle install + pg_tle 0.2.0 update, pg_cron, pg_timetable, Go/Python/TypeScript clients, verify) Not reported

Verdict: ✅ PASSED. No blocking findings at dc0b698: nothing at confidence 8+ with MEDIUM or higher severity. Both round-2 blocking findings are resolved; one has non-blocking leftovers.

This is a review verdict, not merge approval. One merge prerequisite is still open: neither the PR description nor CLAUDE.md step 3 is satisfied yet, because the real v0.2.0 upgrade/reapply evidence has not been posted on the PR. CI does not cover the plain-SQL v0.2.0→HEAD path (see Potential #1).


Prior blocking findings (round 2 at 22e0b24): status at dc0b698

# Round-2 finding Status Evidence
B1 MEDIUM docs/upgrading.md: the default ceiling of 100 is undersized and the docs gave no pre-upgrade warning ✅ Resolved (core). Leftovers are non-blocking. Done:
• docs/upgrading.md:23-30 warns before upgrade. It names the SQL default (100) against the ticker threshold (500), the direct Python/TS receive and Go ReceiveCoop defaults, says a same-ceiling retry cannot progress, says to alert on SQLSTATE 54000 and lag, and says the high-level consumers are unaffected.
• docs/reference.md:125 adds "Choose a ceiling explicitly", and the example changes from 100 to 1000 marked as illustrative.
• reference.md:181 adds a note on the cooperative default.
All claims were checked against source: receive.sql:30, sql/pgque.sql:110, client.py:222,396, client.ts:158,409, options.go:119, and the consumer defaults pgque.go:384, consumer.py:26, consumer.ts:12. The coop consumers pass the integer maximum (consumer.go:121, consumer.py:250, consumer.ts:85). Go Receive has no default, so leaving it out is correct.
Leftovers: docs/pgq-concepts.md:67 (Non-blocking #1) and the demo examples (Potential #2).
B2 MEDIUM: stale truncation wording in the TS README, client.ts, types.ts, Go options.go and Python client.py ✅ Resolved All listed strings now describe complete-batch-or-54000. Also fixed: the Go pgque.go Receive/ReceiveCoop comments, the Go/TS README coop rows, and the old-server wording in the Python/Go READMEs. A repo-wide grep finds no "up to max", "rows beyond", "skipped after ack", "drop rows", "row cap" or "match ticker_max_count" wording left in clients/, docs/ or README. One exception: the Python README coop example still uses max_messages=100 with no note (Potential #2).
— Round-2 non-blocking: the PR description cited a partition test that doesn't exist ✅ Resolved The description now says "Partitioned receive is absent from the 0.2 line; its audit and tests apply only to development PR #366."
— Round-2 non-blocking: v0.2.0 upgrade evidence not posted ❌ Still open No evidence comment on the PR. The only comments are the Codex usage-limit notice and the samorev round 1/2 reports.

CLI gate report (pr-367-samorev-gate.md, run against dc0b698): validation

  • MEDIUM [tests] "no CI test for plain-SQL v0.2.0 → HEAD reinstall over a populated database". The CLI gate marked this BLOCKING. Confirmed as a real gap, but scored at 7/10 (potential), not blocking.
    • .github/workflows/ci.yml at head has only upgrade v0.1.0 to HEAD (git show v0.1.0:sql/pgque.sql, line 126) and the pg_tle 0.2.0 update test (line 224). Nothing runs v0.2.0:sql/pgque.sql.
    • The Bug Hunter and Test Analyzer each validated it independently at 7/10. Mitigation: the v0.1 job already exercises a populated reinstall, and the 0.2.0→0.2.1 SQL delta is function-only create or replace. Round 2 verified that only receive, receive_coop and version change. The delta does not touch sql/ or build/.
    • Posted manual evidence would also satisfy CLAUDE.md step 3. See Potential Set up PgQ git submodule (vendor/pgq/) #1.
  • Defender-dropped "resource-safe" wording item: agree with the drop. The delta keeps the wording, and it still describes the operator's choice of ceiling, not bounded server materialization.
  • Defender-dropped when others in test_api_receive: agree with the drop. A related, separate weakness in test_receive_overflow.sql is carried as Potential Add pgque-specific schema additions (config table, roles, queue_max_retries) #3.

NON-BLOCKING (1)

LOW docs/pgq-concepts.md:67 - ticker_max_count is still described as "force tick at N events (batch-size cap)". (confidence 8/10; Bugs 8 + Docs 7; pre-existing, in #365 scope)

  • This directly contradicts what this PR now says everywhere else: reference.md:135, examples.md:70, three-latencies.md:47, tick-frequency.md:67 and every client doc ("Ticker thresholds do not cap batch size").
  • With the overflow guard in place, a reader who sizes max_return to ticker_max_count from this page gets SQLSTATE 54000 during bursts.

Suggestion: Reword to "event-count threshold for creating a tick; not a batch-size cap", matching tick-frequency.md:67.


POTENTIAL ISSUES (9)

These have moderate confidence (4–7/10). Review them manually; some may be false positives.

MEDIUM .github/workflows/ci.yml - No CI job reinstalls plain-SQL v0.2.0 → HEAD over a populated database. (confidence 7/10; Tests 7 + Bugs 7; CLI gate's blocking item; carried forward and still unresolved)

Suggestion: Add a v0.2.0 variant of the upgrade job:

  • install v0.2.0:sql/pgque.sql;
  • create an active batch of 3 events plus a coop member;
  • reapply HEAD;
  • assert version()='0.2.1', unchanged subscription rows, receive(...,2) → 54000, receive(...,3) → 3 events, and test_upgrade_grants.sql.

Posting manual v0.2.0 reapply evidence on the PR is in any case required before merge (CLAUDE.md step 3).

LOW README.md:314,330,405,423, docs/tutorial.md:102,147,168,189,228,260, docs/examples.md:46,47,59,80, clients/python/README.md:73,144 - The most-copied snippets still pass 100 with no caveat. (Docs reported this at 8; the orchestrator validated it at 6 and downgraded it from MEDIUM; leftover of B1, pre-existing)

  • Every one of these calls was checked: each is a forced-tick, one-event demo, so 100 is correct where it appears. examples.md:70 already carries a complete-batch caveat after the exactly-once pattern.
  • The risk is copy-paste into production. The Python README coop example (max_messages=100) and the README TS snippet have no nearby note. Python README line 73 still says "per-receive limit".

Suggestion: Add a single line after the first README and tutorial receive, e.g. "100 is a demo ceiling; size it for your bursts (see reference: Choose a ceiling explicitly)". Reword Python README line 73 to "complete-batch safety ceiling".

LOW tests/test_receive_overflow.sql:109,334 - Two overflow assertions accept any error. (confidence 6/10; Tests; carried forward)

The already-allocated-batch and stale-takeover cases use when others then v_raised := true with no SQLSTATE check, confirmed by direct inspection. There is also still no coop exactly-N, NULL or INT_MAX case.

Suggestion: Use when sqlstate '54000' and add at least a coop exactly-N case.

LOW docs/upgrading.md:48-51 + sql/pgque-tle.sql:7277-7280 - The new pg_tle origin paragraph is accurate but only partly actionable. (confidence 6/10; Docs; partially resolves a round-2 potential)

  • The claim "rejected without changing the installation" is correct: the raise happens inside the DO block before any pg_tle change.
  • However, the wrapper keys on pgtle.available_extensions().default_version, and the doc gives no query to check it.
  • "Plan and test a separate migration" gives no procedure, and the exception text still does not point to the docs.

Suggestion: Add select default_version from pgtle.available_extensions() where name = 'pgque';, and point the exception text at docs/upgrading.md.

LOW tests/test_tle_upgrade_v0_2.sql - The unsupported-origin and already-registered-path branches of the wrapper are untested. (confidence 5/10; Tests; carried forward)

The delta's new claim "Other origins are rejected without changing the installation" raises the value of a test.

Suggestion: Add a scratch-database case asserting that the raise leaves extversion and the pg_tle registration unchanged.

LOW clients/typescript/test/ - There is no TypeScript overflow/54000 integration test, unlike Go and Python. (confidence 5/10; Tests; carried forward)

LOW docs/tutorial.md:247,285 - The receive(...,1) limit 1 then nack+ack pattern is presented as "the natural pattern". (confidence 5/10; Bugs; pre-existing)

It only works because each tutorial batch holds one event. Copied code now raises 54000 on a batch of more than one event. Before this change it dropped the rest on ack.

Suggestion: Note the single-event assumption.

LOW clients/go/concurrency_test.go:48-49 - The new comment says "maximum producer count", but the ceiling 2*expected is sized against the total event count (goroutines * perGoroutine). (confidence 5/10; Bugs; new in delta, comment only)

Suggestion: "…knows its total event count, so use a ceiling above it."

LOW sql/pgque-tle.sql:7292-7294 / README.md:219-234 - Two round-2 potential items are still unresolved. (confidence 5/10; Docs; carried forward)

The final \echo is vague and prints on every path, and the README pg_tle section does not link to docs/upgrading.md.

INFO (Guidelines, confidence 4–5; not counted above):

  • docs/upgrading.md:19,33: "rather than returning a truncated result" and "now causes an error" are history framing (CLAUDE.md Documentation). This is acceptable inside the per-version upgrade section.
  • .github/workflows/ci.yml:6,8: the short-lived branch release-0-2-1 is hardcoded in the triggers. Remove it after the release.
  • Commit dc0b698 has a body line of about 78 characters, over the 72-character limit in development__git-commit-standards.mdc. Do not amend.

Security (confidence 4, carried forward, still present): build/transform.sh:1003-1008: the dollar-quote collision guard scans pgque.sql but not the generated pg_tle update body. Not exploitable today, because the body is extracted from the same sources. Defense-in-depth fix: repeat the grep -qF check on ${PGTLE_UPDATE_FILE}.


Summary

Area Findings Potential Filtered
CI/Pipeline 0 0 0
Security 0 1 0
Bugs 1 (merged w/ Docs) 2 0
Tests 0 4 0
Guidelines 0 3 (INFO) 0
Docs 0 3 0
Metadata 0 (prior item resolved; evidence item tracked as merge prerequisite) 0 0

Note: "Findings" are high-confidence (8–10/10) issues, "Potential" are moderate (4–7/10) and "Filtered" are 0–3/10. Duplicate findings from different agents were merged at the highest validated confidence and are counted once, under their primary area.


Specialized agent results (all 5 completed on dc0b698)

Agent 1 — Security Reviewer (opus): NO_FINDINGS at confidence 4+ in the delta; 1 carried-forward LOW(4)
  • No executable change in the delta:
    • git diff -U0 -w on the .go/.py/.ts files leaves only comment, JSDoc or docstring lines.
    • Signatures, defaults, queries and the n <= 0 panics are unchanged.
    • In concurrency_test.go only the comment changed.
  • No SQL/build changes in the delta: build/ and sql/ are untouched (git diff --stat is empty), so SECURITY DEFINER, search_path, grants and the generated artifacts are as reviewed in round 2.
  • Delta content:
    • No secrets or DSNs.
    • The only literal change is example 100 → 1000.
    • The doc guidance is safe: it says not to drop a populated extension, to never ack after 54000, and it adds no advice to pass NULL to disable the ceiling.
  • Prior finding: the dollar-quote guard gap is still present at build/transform.sh:1003-1008 (LOW 4, missing guard, not a live exploit).
Agent 2 — Bug Hunter (opus): 5 findings (MEDIUM 7, LOW 8, LOW 6, LOW 5, LOW 5)
  • Findings:
    • MEDIUM (7) ci.yml: no plain-SQL v0.2.0→HEAD test. This is the gate item, unresolved.
    • LOW (8) docs/pgq-concepts.md:67: "batch-size cap".
    • LOW (6): uncaveated 100 examples in README, tutorial, examples and the Python README.
    • LOW (5) docs/tutorial.md:247,285: the limit 1 pattern.
    • LOW (5) concurrency_test.go:48: comment says "producer count" where it means event count.
  • Prior status:
    • B1: partially resolved; the core warning is done.
    • B2: resolved.
    • run_all.sql \echo: resolved.
    • pg_tle origin documentation: resolved, but the exception text has no pointer.
    • Gate CI item: unresolved.
  • Checked clean:
    • Delta byte-identical to git diff 22e0b240 dc0b698 (12 files, +75/−61).
    • All new numeric and default claims match source.
    • 54000 errcode is present in both functions in every built artifact.
    • "No partial result": PL/pgSQL return next buffers rows in a tuplestore, so the raise aborts the statement before any row is sent.
    • "Same ceiling cannot make progress" holds.
    • The wrapper rejects other origins before any pg_tle change.
    • Below threshold: role pre-creation runs before the check, a no-op on a populated install; an unregistered (NULL) origin goes to install_extension.
Agent 3 — Test Analyzer (sonnet): 5 findings (MEDIUM 7, LOW 6, LOW 5, LOW 5, LOW 5)
  • Findings:
    • MEDIUM (7): no CI v0.2.0 plain-SQL reinstall. Validated as the gate item; the agent rated it MEDIUM rather than a hard block given the v0.1 reinstall job and the function-only delta.
    • LOW (6) test_receive_overflow.sql:109,334: when others without a SQLSTATE check.
    • LOW (5): no coop exactly-N/NULL/INT_MAX cases.
    • LOW (5): no TS 54000 test.
    • LOW (5): untested wrapper branches, now more relevant because of the new doc claim.
  • Delta:
    • The run_all.sql \echo is correct and closes the prior item.
    • The concurrency_test.go change is comment-only.
    • No new-in-delta test findings.
Agent 4 — Guidelines Checker (sonnet): 3 INFO (confidence 4–5)
  • Rules read from local postgres-ai rules (/pgai-rules/rules): development__core-principles, development__ai-coding-guidelines, development__git-commit-standards, writing__professional-communication, writing__terminology-consistency, writing__title-capitalization (plus development__db-sql-style-guide; the delta has no SQL logic). Also PgQue CLAUDE.md.
  • Findings:
    • INFO (5): dc0b698 body line about 78 characters, over 72.
    • INFO (5): upgrading.md:19,33 history framing; tolerated in the per-version section.
    • INFO (4): release-0-2-1 hardcoded in the CI triggers.
  • Closed:
    • run_all.sql \echo.
    • "Older servers can truncate" framing in the Python and Go READMEs.
  • Checked clean:
    • Terminology is consistent across the Go, Python, TS and docs ("complete-batch safety ceiling", "SQLSTATE 54000", "resource-safe larger ceiling").
    • No PR or issue numbers in the docs.
    • Sentence-case headings.
    • SOC2 ignored per CLAUDE.md.
Agent 5 — Docs Reviewer (sonnet): 5 findings (MEDIUM 8 → validated 6 by orchestrator, LOW 7, LOW 6, LOW 5, INFO 5)
  • Findings:
    • MEDIUM (8): the README, tutorial, examples and Python README still show 100 without a caveat. The orchestrator downgraded this to LOW/6 potential after checking that every listed call is a one-event demo, and that examples.md:70 already carries the complete-batch caveat.
    • LOW (7) pgq-concepts.md:67: "batch-size cap". Merged with the Bug Hunter finding at 8.
    • LOW (6) upgrading.md:48-51: accurate, but gives no check query and no exception pointer.
    • LOW (5): the pg_tle \echo and the missing README link are unresolved.
    • INFO (5): upgrading.md history wording.
  • Prior status:
    • B1: partially resolved; the core is done and the leftovers are above.
    • B2: resolved, except for the Python README coop example.
  • Checked clean:
    • Every new delta sentence was checked against source defaults and receive.sql:53-54.
    • No stale truncation wording in the client docs, reference.md, upgrading.md or three-latencies.md.

Not applicable:

  • Agent 6 (Sqitch): postgres-ai/platform only.
  • Agent 7 (Visual): the PR touches no UI or visual paths, so IS_VISUAL=0. The GitHub file list was not truncated (28/28 files).

Coverage and limitations

  • Newly reviewed in this round:
    • The complete delta 22e0b240..dc0b698: 12 files, +75/−61, 338 diff lines, read in full by every agent. It was confirmed byte-identical to git diff.
    • Source files inspected directly at dc0b698 wherever a claim needed checking:
      • the SQL defaults and the 54000 raise sites;
      • the client defaults and consumer paths;
      • the pg_tle wrapper;
      • ci.yml;
      • the overflow tests;
      • every remaining receive(...) example in the README, tutorial and examples.
    • No reviewer worked from clipped diff context.
  • Carried forward from round 2 (full-source multi-agent review at 22e0b24, saved at pr-367-samorev-round2.md) — the delta does not touch sql/ or build/ (verified with an empty git diff --stat), so the round-2 runtime-relevant results stay valid:
    • generated sql/pgque.sql, sql/pgque-tle.sql and pgque--0.2.0--0.2.1.sql reproduce byte-for-byte from build/transform.sh;
    • the v0.2.0→head function delta is only receive, receive_coop and version;
    • all wrapper branches were read and checked;
    • SECURITY DEFINER search_path and grants were checked;
    • SRF and rollback semantics were checked.
  • Runtime: this review did not run the SQL, pg_tle or client test suites locally. Runtime behavior rests on the 28/28 green CI checks on the exact head dc0b698. CI does not cover the plain-SQL v0.2.0→HEAD reinstall over populated state, and neither does this review (Potential Set up PgQ git submodule (vendor/pgq/) #1). No posted manual evidence covers it yet either.
  • Validation (Step 5.5): done by the orchestrator, which checked each finding against source and merged cross-agent duplicates. It did not use separate validator subagents.
  • Org rules: loaded locally from pgai-rules/rules.
  • SOC2: skipped, as PgQue CLAUDE.md requires.
  • Completeness: all 5 specialized reviewers completed. None timed out or was incomplete.

samorev-assisted review (AI analysis by Tanya301/samorev); full multi-agent /review-mr delta run, round 3. Exit status: 0 (no blocking findings). This is not an approval and nothing was merged.

Clarify ticker thresholds and consumer comments. Refs #365.
@NikolayS

NikolayS commented Oct 1, 2026

Copy link
Copy Markdown
Owner Author

samorev Code Review Report — round 4 (delta review)

Reviewed head: 034f3f6b353caeedd00d3f8cb1041cde58520692

Scope: delta review. This round covers only the delta from the previous reviewed head dc0b698afce03bd4d9043d9a75b6d30584e78253 (round 3, delta review, PASSED) to the head above. That delta is one commit, 034f3f6 docs: remove remaining batch-cap wording: 4 files, +5/−5, comment and doc wording only. This is not a fresh full-source review. The Coverage section lists what was newly reviewed and what is carried forward.

Pipeline Coverage
✅ success: 28/28 check runs on the exact head 034f3f6, per the GitHub check-runs API (PG 14–18, v0.1.0→HEAD upgrade, pg_tle install path, pg_cron, pg_timetable, Go/Python/TypeScript clients, verify) Not reported

Verdict: ✅ PASSED. There are no blocking findings at 034f3f6, and none was introduced in this delta. All 5 specialized reviewers completed.

This is a review verdict, not merge approval. The gates below are unchanged and still apply:

  • Merge/tag prerequisite (still open): The PR body requires real v0.2.0 upgrade/reapply evidence, with retained queue state and complete event recovery, posted on this PR. It is still not posted. The PR comments contain only the Codex usage-limit notice and the samorev rounds 1–3. CI does not cover the plain-SQL v0.2.0→HEAD path (see Potential Set up PgQ git submodule (vendor/pgq/) #1).
  • Release notes gate (preserved): The draft v0.2.1 GitHub release notes are in this PR's body, to be published with the immutable maintenance tag after all gates pass. That body is unchanged by this delta.
  • Tag-match operator gate (preserved): The PR fix: reject truncated receive batches #366 body requires the release operator to verify that the published v0.2.1 tag exists, run git diff --exit-code v0.2.1 -- sql/pgque.sql sql/pgque-tle.sql sql/pgque-tle-updates/pgque--0.2.0--0.2.1.sql, and post the exact tag/head and result before fix: reject truncated receive batches #366 merges. PR fix: reject truncated receive batches #366 states this is an explicit operator gate, not something CI enforces.

Delta reviewed (dc0b698..034f3f6, complete)

File Change Verified against source at head
clients/go/concurrency_test.go:49 Comment: "maximum producer count" → "maximum message count" Correct. expected := goroutines * perGoroutine (10×20 = 200 events). The ceiling is 2*expected. The test logic is unchanged (git diff -w -U0).
clients/python/README.md:73 "controls the per-receive limit" → "sets the complete-batch safety ceiling for each receive" Correct. consumer.py:26 sets _DEFAULT_MAX_MESSAGES = 2_147_483_647. It is passed to receive/receive_coop at consumer.py:250,255, and the wording matches the client.py docstrings.
clients/typescript/src/consumer.ts:9-10 JSDoc rationale: "so a subsequent pgque.ack(batch_id) does not strand events" → "so ordinary bursts do not exceed a smaller receive ceiling (SQLSTATE 54000)" Correct. The old rationale was stale: under complete-batch-or-54000, ack can no longer strand unseen events. DEFAULT_MAX_MESSAGES is unchanged and used at lines 46, 85 and 88.
docs/pgq-concepts.md:67 "force tick at N events (batch-size cap)" → "event-count threshold for creating a tick; not a batch-size cap" Correct. Matches tick-frequency.md:67, reference.md:135, three-latencies.md:47 and examples.md:70.

No executable code changed. git diff --stat dc0b698 034f3f6 -- sql build is empty, and .github/workflows/ci.yml and tests/ are untouched.

Prior findings: status at 034f3f6

Prior item Round-3 status Status now
Round-2 B1 (MEDIUM, default ceiling 100 undersized, no pre-upgrade warning) ✅ Resolved (core) ✅ Still resolved. Nothing in the delta reverts it.
Round-2 B2 (MEDIUM, stale truncation wording in clients) ✅ Resolved ✅ Still resolved. Head-tree grep: "batch-size cap" appears only in negated form, and "producer count" has no hits. "per-receive limit" appears only in the untracked, gitignored clients/python/pgque_py.egg-info/PKG-INFO (.gitignore:5).
Round-3 Non-blocking LOW(8) docs/pgq-concepts.md:67 "batch-size cap" Open ✅ Resolved, with wording that matches the suggestion.
Round-3 Potential LOW(5) concurrency_test.go:48-49 "producer count" Open ✅ Resolved
Round-3 Potential LOW(6) 100 demo examples / Python README line 73 "per-receive limit" Open ◐ Partially resolved. Python README line 73 is reworded. The uncaveated 100 demo snippets in README/tutorial/examples carry forward (Potential #2).
Round-3 Potential MEDIUM(7) no CI plain-SQL v0.2.0→HEAD reinstall Open Carried forward, unchanged (Potential #1)
All other round-3 potentials (tests, pg_tle docs, tutorial pattern, transform.sh guard) Open Carried forward, unchanged. Those files are not in the delta.
Round-2/3 metadata: v0.2.0 upgrade evidence not posted ❌ Open ❌ Still open. This is a merge prerequisite (see above).

There were no blocking findings in round 3, so no blocking item needed re-validation beyond round-2 B1/B2, which remain resolved.


POTENTIAL ISSUES (8, all carried forward; none new in this delta)

These have moderate confidence (4–7/10). Review them manually. Full text for each is in the round-3 report on this PR.

  1. MEDIUM .github/workflows/ci.yml: no CI job reinstalls plain-SQL v0.2.0 → HEAD over a populated database. (7/10; Tests + Bugs; unchanged) Posting manual v0.2.0 reapply evidence on the PR is required before merge in any case.
  2. LOW README.md:314,330,405,423, docs/tutorial.md:102,147,168,189,228,260, docs/examples.md: one-event demo snippets pass 100 with no "demo ceiling" caveat. (5–6/10; Docs + Bugs) The Python README line 73 part of this item is now resolved.
  3. LOW tests/test_receive_overflow.sql:109,334: when others without a SQLSTATE 54000 check, and there is no coop exactly-N/NULL/INT_MAX case. (6/10; Tests)
  4. LOW docs/upgrading.md:48-51 + sql/pgque-tle.sql exception text: no pgtle.available_extensions() check query, and no pointer to the docs. (6/10; Docs)
  5. LOW tests/test_tle_upgrade_v0_2.sql: the unsupported-origin and already-registered wrapper branches are untested. (5/10; Tests)
  6. LOW clients/typescript/test/: no TypeScript 54000 integration test. (5/10; Tests)
  7. LOW docs/tutorial.md:247,285: the receive(...,1) limit 1 pattern relies on single-event batches. (5/10; Bugs)
  8. LOW sql/pgque-tle.sql:7292-7294 / README.md pg_tle section: the final \echo is vague, and the pg_tle section does not link docs/upgrading.md. (5/10; Docs)

Security (carried forward, confidence 4): build/transform.sh:1003-1008: the dollar-quote collision guard does not re-scan the generated pg_tle update body. This is defense in depth and not exploitable today. The file is unchanged in this delta.

INFO (Guidelines, carried forward):

  • docs/upgrading.md:19,33 uses history framing. This is tolerated in the per-version section.
  • .github/workflows/ci.yml:6,8 hardcodes release-0-2-1 in its triggers. Remove it after the release.
  • Commit dc0b698 has a body line of about 78 characters. Do not amend. The new commit 034f3f6 complies: a 40-character subject and a 59-character body.

Filtered (1):

  • The Docs reviewer flagged INFO(4) on clients/go/concurrency_test.go:253-256, where the test comment says the old hardcoded Receive(...,100) was "stranding any extras".
  • The orchestrator validated it at 3/10 and filtered it. The comment is explicitly historical ("prior to WithMaxMessages") and accurately describes pre-fix server behavior. The Bug Hunter independently judged it accurate history. It is also pre-existing and outside the delta.

Summary

Area Findings Potential Filtered
CI/Pipeline 0 0 0
Security 0 1 (carried) 0
Bugs 0 1 (carried) 0
Tests 0 4 (carried) 0
Guidelines 0 3 INFO (carried) 0
Docs 0 3 (carried) 1
Metadata 0 (v0.2.0 evidence tracked as a merge prerequisite) 0 0

New-in-delta findings: 0. The round-3 non-blocking finding and one round-3 potential are resolved. A second round-3 potential is partially resolved.


Specialized agent results (all 5 completed on 034f3f6)

Agent 1 — Security Reviewer (opus): NO_FINDINGS in the delta; 1 carried-forward LOW(4). Complete.
  • git diff -w -U0 shows only Go comment, JSDoc and Markdown lines. DEFAULT_MAX_MESSAGES = 2_147_483_647 is untouched (checked in source).
  • sql/ and build/ are untouched, so the round-2/3 runtime-relevant security results carry forward.
  • No secrets in the diff (grep for password/secret/token/key).
  • No unsafe guidance: no advice to use NULL or unlimited ceilings, and no advice to ack after 54000. The new wording strengthens the "not a batch-size cap" guidance.
  • Carried forward: build/transform.sh:1003-1008 dollar-quote guard gap, LOW(4). The file is unchanged.
Agent 2 — Bug Hunter (opus): NO_FINDINGS in the delta. Complete for the delta scope.
  • Each of the 4 changed lines was verified against source at head:
    • Go: 200 events with a ceiling of 400.
    • Python: consumer.py:26,250,255 and the client.py:238,411 docstrings.
    • TypeScript: consumer.ts:46,85,88 and client.ts:153 ("returns no partial result and fails").
    • pgq-concepts: consistent with tick-frequency/reference/three-latencies/examples.
  • Head-tree grep:
    • "batch-size cap" appears only in negated form.
    • "producer count": no hits.
    • "per-receive limit" appears only in the untracked egg-info.
    • "strand" appears only in a historical Go test comment at line 255, outside the delta and accurate as history.
  • Prior items:
    • pgq-concepts LOW(8): resolved.
    • concurrency_test LOW(5): resolved.
    • 100 examples LOW(6): partially resolved (Python README fixed).
    • Tutorial LOW(5): carried forward.
    • CI v0.2.0 MEDIUM(7): carried forward.
    • Round-2 B1/B2: not reverted.
  • Noted limits: it did not re-audit unchanged code, did not open the round-3 file (prior context was supplied in its prompt), and ran no tests.
Agent 3 — Test Analyzer (sonnet): NO_FINDINGS in the delta; 5 carried-forward potentials. Complete.
  • The Go test change is comment-only (git diff -w -U0), and the test logic is unchanged.
  • No tests/, sql/, build or CI files changed, so no behavior change needs test updates.
  • Prior items, all unchanged and carried forward:
    • CI v0.2.0 plain-SQL reinstall, MEDIUM(7)
    • test_receive_overflow.sql:109,334 when others, LOW(6)
    • coop exactly-N/NULL/INT_MAX, LOW(5)
    • TS 54000 test, LOW(5)
    • pg_tle wrapper branches, LOW(5)
Agent 4 — Guidelines Checker (sonnet): NO_FINDINGS in the delta; 3 carried-forward INFO. Complete for the delta.
  • Rules read (locally from pgai-rules/rules): PgQue CLAUDE.md (Documentation and Git Commits sections), development__git-commit-standards, writing__terminology-consistency.
  • Rules not opened in full: writing__professional-communication, writing__title-capitalization, development__core-principles, development__ai-coding-guidelines. The agent judged that none of them applies to 5 lines of prose and comments with no headings. This is a disclosed scope limit, not an incomplete run.
  • Commit 034f3f6: the subject is 40 characters, Conventional Commits docs:, present tense. The body is 59 characters with Refs #365. Both comply, and the orchestrator re-measured them independently.
  • Terminology: "complete-batch safety ceiling" and "SQLSTATE 54000" are consistent across clients and docs. No PR/issue numbers or version tags were added to the docs, and no history framing was added.
  • Python README line 73 is 92 characters, longer than its neighbors. No rule sets a Markdown line width, so this is not a finding.
  • Prior INFO items carried forward: dc0b698 body length (do not amend), upgrading.md history framing, and release-0-2-1 CI trigger.
  • SOC2: ignored per PgQue CLAUDE.md.
Agent 5 — Docs Reviewer (sonnet): no new findings above the filter; 1 INFO(4) filtered by the orchestrator; carried-forward LOWs. Complete.
  • All 5 changed lines are accurate at head. The removed TS "strand events" rationale was stale under complete-batch-or-54000, and removing it loses no information.
  • Grep found no live doc that contradicts complete-batch-or-54000. upgrading.md:19 "truncated result" is correct history. The TRUNCATE-rotation and edge_test.go truncation hits are unrelated. The stale PKG-INFO is an untracked, gitignored build artifact.
  • INFO(4) concurrency_test.go:253-256 "stranding any extras": filtered by the orchestrator. It is a historical comment, accurate for pre-fix servers, pre-existing and outside the delta.
  • Prior items:
    • pgq-concepts LOW(8): resolved.
    • Python README line 73: resolved.
    • concurrency_test "producer count": resolved.
    • 100 README/tutorial examples: carried forward.
    • upgrading.md:48-51 check query/exception pointer: carried forward.
    • pg_tle \echo and README link: carried forward.

Not applicable:

  • Agent 6 (Sqitch): postgres-ai/platform only.
  • Agent 7 (Visual): the delta touches no UI or visual paths (IS_VISUAL=0). The 4-file delta list is complete and not truncated.

Coverage and limitations

  • Newly reviewed in this round:
    • The complete delta dc0b698..034f3f6: 1 commit, 4 files, +5/−5. It was read in full by all 5 reviewers from git diff, with no clipped diff context.
    • Source inspected directly at 034f3f6 wherever a claim needed checking: Python consumer.py/client.py, TS consumer.ts/client.ts, the Go test, the tick and threshold docs, .gitignore, and build/transform.sh.
    • The PR fix: reject truncated receive batches #366 body was checked for the tag-match gate text.
  • Carried forward from round 3 (delta review at dc0b698, pr-367-samorev-round3.md) and round 2 (full-source multi-agent review at 22e0b24, pr-367-samorev-round2.md). These results stay valid because sql/, build/, tests/ and .github/ are untouched in this delta (verified with an empty git diff --stat):
    • generated sql/pgque.sql, sql/pgque-tle.sql and pgque--0.2.0--0.2.1.sql reproduce byte-for-byte from build/transform.sh;
    • the v0.2.0→head function delta is only receive, receive_coop and version;
    • the pg_tle wrapper branches were checked;
    • SECURITY DEFINER search_path and grants were checked;
    • SRF and rollback semantics were checked;
    • the round-3 doc/default claims were verified against source.
  • Not re-audited: unchanged code outside the delta, beyond the targeted greps and source checks listed above.
  • Runtime: this review ran no SQL, pg_tle or client test suites locally. Runtime behavior rests on the 28/28 green CI check runs on the exact head 034f3f6 and on the carried-forward round-2 analysis. Neither CI nor this review covers the plain-SQL v0.2.0→HEAD reinstall over populated state (Potential Set up PgQ git submodule (vendor/pgq/) #1), and no manual evidence for it has been posted yet.
  • Validation (Step 5.5): done by the orchestrator, which checked each finding against source and merged cross-agent duplicates. It did not use separate validator subagents.
  • Org rules: loaded locally from pgai-rules/rules. The Guidelines reviewer's rule subset is disclosed above.
  • SOC2: skipped, as PgQue CLAUDE.md requires.
  • Completeness: all 5 specialized reviewers completed. None timed out or was incomplete.

samorev-assisted review (AI analysis by Tanya301/samorev); multi-agent /review-mr delta run, round 4, reviewed head 034f3f6b353caeedd00d3f8cb1041cde58520692. Exit status: 0 (no blocking findings). This is not an approval. Nothing was approved, merged, committed or pushed.

@NikolayS

NikolayS commented Oct 1, 2026

Copy link
Copy Markdown
Owner Author

Post-review real-test evidence

Exact reviewed/tested head: 034f3f6b353caeedd00d3f8cb1041cde58520692. SamoRev round 4 PASS is posted above; no source changes followed it. All commands below completed after that review with exit status 0.

Environment: isolated Postgres 18.6 containers, no published ports, localhost listening, connection logging enabled. pg_tle upgrade uses v1.5.2.

Commands and results

From the candidate checkout, the regression harness executes:

psql -X -v ON_ERROR_STOP=1 -f sql/pgque.sql
psql -X -v ON_ERROR_STOP=1 -f tests/run_all.sql
psql -X -v ON_ERROR_STOP=1 -f tests/acceptance/run_acceptance.sql
psql -X -v ON_ERROR_STOP=1 -f sql/pgque.sql -f tests/test_install_idempotency.sql

Outputs: ALL TESTS PASSED, ALL ACCEPTANCE TESTS PASSED, and PASS: stable post-review regression, acceptance, reapply.

Additional executable harnesses (all exit 0):

  • bash doc-real-test/run.sh: six snippets extracted byte-for-byte from current main/stable reference and examples; reference receive, fan-out, exactly-once transaction; 1,001-event burst against the documented 1,000 ceiling. Asserts SQLSTATE 54000, explicit rollback preserves the cursor, then retry processes all 1,001 distinct event identities, range 1–1,001, before acknowledgment. Both surfaces PASS.
  • bash upgrade-smoke.sh: installs tagged v0.2.0, creates populated normal/cooperative queues, active batch, retry/DLQ/config state; applies the maintenance installer twice; asserts state retained, version 0.2.1, overflow and complete recovery. Output: PASS: isolated v0.2.0 to v0.2.1 upgrade smoke test.
  • bash tle-upgrade-smoke.sh: installs tagged v0.2.0 pg_tle wrapper, registers the candidate update path twice, updates populated extension, checks both versions and preserved active batch/subscription state, verifies plain/cooperative 54000 plus full retry. Also validates fresh CREATE EXTENSION through the registered default update edge. Output: test_tle_upgrade_v0_2: ALL PASSED.

The upgrade harnesses use the maintenance candidate; its three frozen release SQL files are byte-identical to this PR's frozen files. No throughput benchmark or client package publication is claimed.

Non-blocking review dispositions

  • Generated-file provenance: the maintenance build/transform.sh does generate the pg_tle wrapper and function-only update edge. Main's frozen files are copied from that maintenance build; they are not independently hand-maintained. The published-tag comparison remains mandatory before fix: reject truncated receive batches #366 merges.
  • The plain-SQL v0.2.0 CI-matrix gap is covered for this release by the populated post-review test above. Optional additional per-variant boundaries, TypeScript/consumer-loop error tests and wrapper-branch coverage remain non-blocking test-hardening work; existing SQL coverage checks all receive paths, event recovery and SQLSTATE.
  • One-event tutorial/demo ceilings are valid for their fixtures; the reference and release notes explicitly warn about default 100, ticker thresholds, and operator-selected resource limits. Real pagination is feat: design safe paged batch consumption #364.
  • Automatic statement rollback in pooled/autocommit APIs requires no explicit client rollback; the general rollback instruction applies to an explicit failed transaction. No runtime change is needed.
  • Reusing an already registered pg_tle update path requires a trusted pg_tle administrator; additional catalog-integrity checks are optional defense in depth. The generated dollar-quote update body is extracted from the already scanned installer sources.
  • Historical commit formatting is retained (no amend). Remaining wording/style and optional origin-introspection suggestions do not change the verified contract.

Evidence integrity

Logs retained in pgque-overflow-evidence/; SHA-256:

b64d2472bd14e297a4d3de1f7a429bf07a61d9a86cee138fa1d14e630570833e  post-review-stable-regression.log
b867a9c516d88b3028c33feb5185774042a66aa2e775347e272d6cccdeb68395  post-review-doc-real-test.log
8377c976e4837577be296a8972a6f7c80e70d55ea08bd38d3bd109224d84b2eb  post-review-upgrade-smoke.log
413aad50bf41aa3217f9985e39041a34e79a1723040d8be7e095bcb087db7d1b  post-review-tle-upgrade.log

Release sequencing remains: merge maintenance, verify exact merge CI, publish and verify v0.2.1, prove frozen artifacts match the tag, then merge development.

@NikolayS
NikolayS merged commit 32bfbe2 into maintenance-0-2 Oct 1, 2026
28 checks passed
@NikolayS

NikolayS commented Oct 1, 2026

Copy link
Copy Markdown
Owner Author

Published-tag gate: PASS

  • Maintenance PR fix: backport receive safety to 0.2.1 #367 is MERGED at 32bfbe2119c8fcadf6d49efe886435fdde12500d; its tree exactly matches reviewed head 034f3f6b353caeedd00d3f8cb1041cde58520692.
  • Exact-merge CI run 36883000305: 14/14 jobs passed.
  • v0.2.1 is published, not draft/prerelease, and confirmed by GitHub's latest-release endpoint.
  • git rev-parse 'v0.2.1^{commit}' = 32bfbe2119c8fcadf6d49efe886435fdde12500d.
  • Downloaded all three SQL release assets plus SHA256SUMS; sha256sum -c SHA256SUMS reports OK for all three, and each file matches the exact merge artifact.

At development PR #366 head e8074faf4112f768b5f0e21e161c64d751b5ad85:

git diff --exit-code v0.2.1 -- sql/pgque.sql sql/pgque-tle.sql sql/pgque-tle-updates/pgque--0.2.0--0.2.1.sql

Exit 0, no diff. The frozen SQL/tag-match release-operator gate is satisfied.

A fresh detached checkout of the published tag also passed (all exit 0):

  • fresh SQL install, pgque.version() = '0.2.1', and the complete overflow regression;
  • populated v0.2.0 plain-SQL upgrade/reapply, retained state and complete recovery;
  • populated pg_tle v1.5.2 update plus fresh CREATE EXTENSION through the registered update edge.

Output markers: PASS: tagged fresh SQL install, version 0.2.1, overflow regression; PASS: isolated v0.2.0 to v0.2.1 upgrade smoke test; test_tle_upgrade_v0_2: ALL PASSED.

No client packages were published. Real pagination remains tracked separately in #364.

@NikolayS
NikolayS deleted the release-0-2-1 branch October 1, 2026 15:21
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.

2 participants