Skip to content

fix: multiple bug fixes and dead code cleanup#75

Open
hobostay wants to merge 2 commits intoNikolayS:mainfrom
hobostay:fix/multiple-bugs-and-improvements
Open

fix: multiple bug fixes and dead code cleanup#75
hobostay wants to merge 2 commits intoNikolayS:mainfrom
hobostay:fix/multiple-bugs-and-improvements

Conversation

@hobostay
Copy link
Copy Markdown

Summary

  • Go consumer fixes: Add missing Nack() method; use per-message nack for handler failures instead of skipping the entire batch; log warning for unregistered event types instead of silently dropping messages
  • Security hardening: Use quote_ident() in maint() for VACUUM target; add explicit queue existence check in insert_event_raw() with clear error message
  • Atomicity fix: dlq_replay_all() now wraps per-event replay in exception blocks so a single failure doesn't abort the entire operation
  • Role hierarchy fix: upgrade_schema() had inverted role membership — pgque_admin was created as member of reader/writer instead of the reverse; also fixed return value (was hardcoded 0, now returns actual count)
  • Dead code cleanup: Remove PG 8.3 pg_autovacuum code path (PgQue requires PG14+); remove references to non-existent schemas (pgq_ext, pgq_node, londiste, txid) from maint_tables_to_vacuum()
  • Typo fixes: consubersconsumers, poinpoint, oldesoldest, wuithwith, accitentallyaccidentally, QueteQuote

Test plan

  • Go code compiles and passes go vet
  • Run existing SQL test suite against modified pgque.sql
  • Verify upgrade_schema() role hierarchy is correct (pgque_reader → pgque_writer → pgque_admin)
  • Test dlq_replay_all() with a concurrent queue drop to verify exception handling
  • Verify insert_event_raw() raises clear error for non-existent queue

🤖 Generated with Claude Code

1. Go consumer: add Nack() method and use per-message nack instead of
   skipping the entire batch on handler failure. Log warning for
   unregistered event types instead of silently dropping messages.

2. maint(): use quote_ident() for VACUUM target to prevent potential
   SQL injection via dynamic SQL construction.

3. insert_event_raw(): add explicit queue existence check with clear
   error message instead of NULL pointer crash on missing queue.

4. dlq_replay_all(): wrap per-event replay in exception block so a
   single failure (e.g. concurrent queue drop) doesn't abort the
   entire replay operation.

5. upgrade_schema(): fix inverted role membership — pgque_admin was
   created as a member of reader/writer instead of the reverse.
   Also return cnt instead of hardcoded 0.

6. tune_storage(): remove dead PG 8.3 code path (pg_autovacuum catalog)
   and unnecessary version check — PgQue requires PG14+.

7. maint_tables_to_vacuum(): remove references to non-existent schemas
   (pgq_ext, pgq_node, londiste, txid) left over from PgQ fork.

8. Fix typos in comments: consubers→consumers, poin→point, oldes→oldest,
   wuith→with, accitentally→accidentally, Quete→Quote.

Co-Authored-By: Claude Opus 4.7 <[email protected]>
@NikolayS
Copy link
Copy Markdown
Owner

@hobostay thank you

It would be great to split this PR into pieces - I see two different classes things here:

  1. Typos in the original PgQ code, as well as "dead code" (for pre-14) – we aim to keep the original code as unchanged as possible, incorporating it into a single .sql. I think it would be great to move typo fixes upstream: https://github.com/pgq/pgq
  2. Everything else – I'm going to process separately

Remove Class 1 changes (typos, PG 8.3 dead code removal, non-existent
schema references) as requested by maintainer. These should be pushed
upstream to pgq/pgq instead. Only bug fixes remain in this PR.

Co-Authored-By: Claude Opus 4.7 <[email protected]>
@hobostay
Copy link
Copy Markdown
Author

@NikolayS Thank you for the feedback! I've updated the PR to remove all typo fixes and dead code cleanup (PG 8.3 pg_autovacuum code path, non-existent schema references in maint_tables_to_vacuum()). These changes have been reverted in commit 1830ca2.

This PR now only contains the substantive bug fixes:

  • Go consumer: per-message nack for handler failures, log warning for unregistered event types
  • Go client: add missing Nack() method
  • dlq_replay_all(): wrap per-event replay in exception blocks
  • maint(): use quote_ident() for VACUUM target
  • upgrade_schema(): fix inverted role hierarchy, fix return value
  • insert_event_raw(): add queue existence check with clear error message

I'll look into submitting the typo fixes upstream to pgq/pgq separately.

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