samorev Code Review Report

• PR: fix: lock dead_letter rows in dlq_replay #287 - fix: lock dead_letter rows in dlq_replay
• Author: @NikolayS
• AI-Assisted: Unknown

BLOCKING ISSUES (1)

HIGH MR/PR state - Review target is draft

The review target is still marked as draft.
Fix: Mark it ready for review before merge.

Summary

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

provider=github
kind=pr
project=NikolayS/PgQue
number=287
target=github:NikolayS/PgQue#287
state=OPEN
draft=true
diff_lines=330
diff_added=225
diff_removed=3
diff_bytes=13004
comments_count=0
commits_count=1
ci_status=success
ci_summary=total=14 success=14 failure=0 pending=0 other=0
prompt=.claude/commands/review-mr.md
blocking=false
posted_by=gh
no_comment=false
live_posting=posted

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

Post-merge verification (PR-lifecycle steps 1–3)

Verified on a fresh local PostgreSQL 18.3 install. Note: this PR was already merged into main (a4bd61c, after #292 cd3b71d) by the time this ran, so "red" is demonstrated against the pre-fix parent commit rather than current main.

1. CI — all 14 checks green on HEAD; mergeable.

2. Diff review — locking change only (for update of dl in dlq_replay, for update of dl skip locked in dlq_replay_all). security definer + set search_path = pgque, pg_catalog unchanged; no new subtransactions introduced (the pre-existing begin/exception in dlq_replay_all is untouched, and neither function is a hot path). bash build/transform.sh after init'ing the pgq submodule produced zero drift — sql/pgque.sql and sql/pgque-tle.sql match the source edit in sql/pgque-additions/dlq.sql.

3. Real testing

Race reproduced against the pre-fix build (cd3b71d):

FAIL: session2 replay succeeded; expected 'dead letter entry not found' after waiting on session1
 s1_new_eid=2003
 s2_new_eid=2004          <- event re-enqueued twice

Same test against the merged fix build:

PASS: concurrent dlq_replay serialized; second caller got 'dead letter entry not found' and the event was re-enqueued exactly once

Full regression suite on the fix build: tests/run_all.sql → === ALL TESTS PASSED === (153 PASS, exit 0).

Fix confirmed correct.

Post-merge code review — PR #287 (fix: lock dead_letter rows in dlq_replay)

Thorough post-merge review (the review bot may not have run properly on merge). Reviewed the full diff at merge commit a4bd61c, read the surrounding DLQ source, and verified the fix empirically on Postgres 17. Verdict: the fix is correct and complete. No blocking findings. Two nits below, neither warranting a follow-up on its own.

Empirical verification

Ran the new harness against both the merged build and the pre-fix parent (cd3b71d) on a fresh postgres:17:

• Red (parent cd3b71d, new harness against old install): FAILS — both sessions re-enqueue the dead letter (s1_new_eid=2003, s2_new_eid=2004). The bug reproduces exactly as described.
• Green (merged a4bd61c): PASSES — session 2 blocks, then raises dead letter entry not found, event re-enqueued exactly once, DLQ empty.
• Full suite (tests/run_all.sql, fresh DB, merged build): exit 0, === ALL TESTS PASSED ===, 156 PASS.
• Generated-file consistency: bash build/transform.sh on the merged commit produces zero diff — sql/pgque.sql, sql/pgque-tle.sql, and the sql/pgque-additions/dlq.sql source are in sync.

(a) Concurrency correctness — correct

• for update of dl (sql/pgque-additions/dlq.sql:139) locks only the dead_letter row, not the joined pgque.queue row. This is the right scope: locking the queue row would create needless contention with queue/tick operations.
• The READ COMMITTED re-check is sound. When session 2's for update waits on session 1's locked row and session 1 commits a delete of that row, PostgreSQL's EvalPlanQual drops the row from the result set — so into v_dl finds nothing, if not found fires, and the existing dead letter entry not found error is raised. No replay-of-deleted-row anomaly. Confirmed empirically (green).
• Disjoint event sets still run concurrently: dlq_replay(dl_id) locks a single row selected by dl.dl_id, so two calls for different dl_ids never contend. No false serialization.
• No deadlock / lock-ordering issue. dlq_replay only ever locks one dead_letter row, then inserts + deletes that same row; it never waits for a second dead_letter row lock, so it cannot form a cycle with dlq_purge (bulk delete) or a second dlq_replay. dlq_replay_all uses skip locked and therefore never blocks on a dead_letter row at all. event_dead() only inserts new rows (on conflict do nothing), touching different tuples than a replay of an existing row.

(b) Coverage of replay entry points — complete

The only two re-enqueue paths are dlq_replay (single) and dlq_replay_all (batch); both are fixed. dlq_inspect is read-only, dlq_purge is a bulk delete, event_dead is the insert side. No uncovered replay path exists.

(c) Test quality — genuine concurrency regression test

tests/two_session_dlq_replay_race.sh uses two real backends (backgrounded psql holding an open transaction in pg_sleep), and gates session 2 on observing session 1 inside PgSleep via pg_stat_activity — deterministic, not a fixed sleep. It fails on pre-fix code and passes on the fix (verified above). It asserts all three properties: the loser errors with the exact message, exactly one event is re-enqueued, and the DLQ ends empty. Follows the established tests/two_session_receive_lock.sh pattern.

Nits (non-blocking)

• nit — dlq_replay_all under-counts on a concurrent rollback. sql/pgque-additions/dlq.sql:198 (for update of dl skip locked): if a row is locked by a concurrent dlq_replay() that later rolls back, this call skips it — the row is counted as neither replayed nor failed, and remains in the DLQ for a later pass. This is the correct choice for replay-everything semantics and is documented in the function's docstring, so callers relying on replayed/failed totals just see a transient under-report. No action needed; flagging only so it's on record.
• nit — comment block style. CLAUDE.md prefers /* ... */ for comments spanning 2+ lines. The new 5–6 line header comments use -- per line. However they match the dominant convention already used throughout dlq.sql (e.g. the pre-existing event_dead / dlq_replay_all headers), so matching surrounding style arguably wins here. Cosmetic.

Reviewed with REV methodology; SOC2 findings intentionally out of scope per repo policy.

samorev Code Review Report

• PR: fix: lock dead_letter rows in dlq_replay #287 - fix: lock dead_letter rows in dlq_replay
• Author: @NikolayS
• AI-Assisted: Yes

This report supersedes the earlier samorev review --fetch comment on this PR. That comment was a CI/draft gate check only and performed no code analysis (it hardcoded zeros in every area). The results below come from a genuine per-area review of the diff, verified empirically with a two-session race reproduction on PostgreSQL 17.

No blocking issues. The fix is correct and complete; two non-blocking notes below.

What was verified

• Red/green race repro: against the pre-fix parent, the new harness tests/two_session_dlq_replay_race.sh fails — both sessions re-enqueue the dead letter (s1_new_eid=2003, s2_new_eid=2004). Against the merged fix it passes — session 2 blocks, raises dead letter entry not found, and the event is re-enqueued exactly once.
• Concurrency correctness: for update of dl locks only the dead_letter row (not the joined pgque.queue row) — correct scope. The READ COMMITTED re-check is sound: when the waiting session's for update resolves against a row session 1 deleted, EvalPlanQual drops it, if not found fires, and the existing error is raised. Disjoint dl_ids never contend; no lock-ordering cycle with dlq_purge/event_dead.
• Coverage: the only two re-enqueue paths (dlq_replay, dlq_replay_all) are both fixed; dlq_inspect/dlq_purge/event_dead are not replay paths.
• Full suite: tests/run_all.sql → === ALL TESTS PASSED === (156 PASS) on a fresh DB with the merged build.
• Generated-file consistency: bash build/transform.sh at the merge commit produces zero drift; sql/pgque.sql, sql/pgque-tle.sql, and sql/pgque-additions/dlq.sql are in sync. security definer + set search_path unchanged; no new subtransactions in any hot path.

NON-BLOCKING (2)

LOW sql/pgque-additions/dlq.sql:198 - dlq_replay_all under-counts on a concurrent rollback

for update of dl skip locked skips a row locked by a concurrent dlq_replay() that later rolls back; the row is counted as neither replayed nor failed and stays in the DLQ for a later pass.
Suggestion: This is the correct choice for replay-everything semantics and is documented in the docstring — no change needed. Callers relying on the totals just see a transient under-report. Flagged for the record.

INFO sql/pgque-additions/dlq.sql - multi-line comment style

The new 5-6 line header comments use -- per line where CLAUDE.md prefers /* ... */ for 2+ lines.
Suggestion: Matches the dominant convention already in dlq.sql (event_dead/dlq_replay_all headers), so matching surrounding style is defensible. Cosmetic.

Summary

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

provider=github
kind=pr
project=NikolayS/pgque
number=287
target=github:NikolayS/pgque#287
state=MERGED
draft=false
diff_lines=330
diff_added=225
diff_removed=3
diff_bytes=13012
comments_count=3
commits_count=1
ci_status=success
ci_summary=total=14 success=14 failure=0 pending=0 other=0
prompt=.claude/commands/review-mr.md
blocking=false
posted_by=gh
no_comment=false
live_posting=posted

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