Prevent double frees when retrying interrupted dequeues

[serprex]

Clear each slot reference before resolving and releasing its string,
so dequeue recovery cannot free error text or release query text twice

Attach to DSA before clearing references, since attachment can raise ERROR

Delay storing enqueue string references until just before advancing head

Consolidate DSA attachment helpers and remove ineffective exception,
handling around noexcept exporter teardown

[Copilot]

🟡 Changes recommended

Interrupted producer cleanup can still leak allocations and needs a recoverable cleanup mechanism.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR improves shared-memory queue string cleanup during interrupted operations and simplifies exporter shutdown.

Changes:

• Adds DSA and query-reference reclamation APIs.
• Detaches queue references and strengthens synchronization.
• Simplifies exporter shutdown handling.

File summaries

File	Summary	Review Notes
src/queue/shmem.c	Queue cleanup, reference detachment, and barriers	Critical: interrupted producer cleanup can still leak allocations. Nit: add a regression test.
src/queue/query_intern.h	Declares query-reference release API	No findings.
src/queue/query_intern.c	Implements query-reference release	No findings.
src/queue/psch_dsa.h	Declares DSA string cleanup API	No findings.
src/queue/psch_dsa.c	Implements DSA string cleanup	No findings.
src/export/stats_exporter.cc	Simplifies exporter shutdown	No findings.

Review details

Suppressed comments (2)

src/queue/shmem.c:485

• Clearing both pointers before either release makes an interrupted dequeue leak the resources permanently. If the worker exits after these assignments (for example while resolving the error message or query), the replacement worker sees invalid pointers and cannot call PschDsaFreeString or PschQueryInternRelease; the query interner reference/HTAB entry and DSA allocations remain live. Repeated worker restarts can therefore exhaust the bounded DSA pool and fill the interner. Please add a recoverable in-progress/cleanup protocol or another restart-time reclamation path instead of dropping the only ownership record.

  slot->err_message_dsa = InvalidDsaPointer;
  slot->query_dsa = InvalidDsaPointer;
  pg_write_barrier();

src/queue/shmem.c:140

• Please add a regression test for this recovery path. The existing TAP/isolation tests cover normal slot reuse, interner OOM, and concurrent acquisition, but none forces an enqueue to abort after storing DSA/query references and then reuses that slot; double-free and leak regressions in these new frees and barriers can therefore pass unnoticed.

  ReclaimSlotStrings(slot);

• Files reviewed: 6/6 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

Unresolved interrupted-worker and retry-safe cleanup paths can leak DSA allocations or interner references.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (3)

src/queue/shmem.c:492

• Clearing both references before resolving makes the replacement worker skip the frees, but it also loses the only handles needed to reclaim them. If this worker is terminated after these assignments and before the resolve/release calls (the PR explicitly relies on restart with shared memory preserved), the error allocation remains allocated and the interner refcount remains elevated; repeated restarts can exhaust the DSA and fill the interner. Keep an in-flight recovery record or otherwise reclaim these references on worker exit while still making the slot safe for the replacement worker.

  dsa_pointer err_message_dsa = slot->err_message_dsa;
  dsa_pointer query_dsa = slot->query_dsa;
  slot->err_message_dsa = InvalidDsaPointer;
  slot->query_dsa = InvalidDsaPointer;
  pg_write_barrier();

src/queue/shmem.c:101

• The new recovery path is not covered by the existing TAP tests: the current slot-reuse and interner tests exercise successful dequeue and DSA OOM, but never force an error after an error/query reference has been written and then retry the same slot. Add fault-injection coverage for both references and an interrupted-worker restart case so this cleanup cannot regress silently.

static void ReclaimSlotStrings(PschRingEntry* slot) {
  dsa_pointer err_message_dsa = slot->err_message_dsa;
  dsa_pointer query_dsa = slot->query_dsa;

  if (!DsaPointerIsValid(err_message_dsa) && !DsaPointerIsValid(query_dsa)) {

src/queue/shmem.c:110

• Clearing both references before either cleanup makes an interrupted enqueue lose ownership of the query reference. PschQueryInternRelease acquires a partition LWLock and can be interrupted while waiting; if that happens after PschDsaFreeString returns, head is still unchanged, but the next producer sees invalid pointers and overwrites the slot, leaving the interner reference/object retained indefinitely. Use a retry-safe per-reference cleanup protocol so an error cannot make an incomplete release unrecoverable.

  PschDsaFreeString(err_message_dsa);
  PschQueryInternRelease(query_dsa);

• Files reviewed: 7/7 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Queue cleanup remains unsafe when attachment, allocation, or worker interruption occurs.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (5)

src/queue/shmem.c:142

• 🔴 MUST FIX: PschDsaAttach() can return NULL, and both cleanup helpers intentionally leave their allocations untouched when no handle is available. Ignoring that result lets a slot with valid stale pointers get cleared and overwritten without freeing the error allocation or releasing the interner reference, causing a permanent leak. Preserve the slot and retry/propagate the unavailable-cleanup case instead of proceeding with reuse.

  PschDsaAttach();
  ReclaimSlotStrings(slot);

src/queue/shmem.c:475

• 🔴 MUST FIX: The nullable attach result is also ignored on dequeue. If attachment is unavailable, the following code clears valid references, the resolve/release helpers leave the allocations intact, and line 499 still consumes the slot, permanently leaking both strings. Check the handle and leave tail unchanged until cleanup can run.

  PschDsaAttach();

src/queue/shmem.c:475

• Because PschDsaAttach() is documented as able to raise ERROR, moving the first attach here makes that longjmp occur while PschExportBatch() owns a live std::vector<PschEvent>. PostgreSQL longjmp bypasses C++ destructors, so an attach failure leaks the vector on every retry. Keep the attach before entering the C++ export/RAII path, or otherwise isolate it from DequeueEvents.

  PschDsaAttach();

src/queue/shmem.c:109

• The same failure mode exists for an aborted enqueue: clearing both fields before PschDsaFreeString() and PschQueryInternRelease() means an ERROR during the first cleanup loses the second reference permanently, and the next enqueue overwrites the slot. This helper needs exception-safe cleanup/per-reference state rather than one all-or-nothing pointer clear.

  slot->err_message_dsa = InvalidDsaPointer;
  slot->query_dsa = InvalidDsaPointer;
  pg_write_barrier();

  PschDsaFreeString(err_message_dsa);

src/queue/shmem.c:489

• The new restart/aborted-enqueue protocol has no regression test: existing queue tests cover normal interleaving, slot reuse, and OOM, but none terminates the worker after references are detached or injects an ERROR after a DSA/interner allocation. Add a TAP or isolation test (with fault injection if needed) that verifies the queue drains without double release or leaked references after those interruptions.

  // 2. Detach string references before consuming them.  A FATAL exits with
  //    status 1, which postmaster does not treat as a crash, so shmem survives
  //    into the replacement worker: a slot left pointing at freed memory would
  //    double-free the error message and drop a second interner reference.
  dsa_pointer err_message_dsa = slot->err_message_dsa;
  dsa_pointer query_dsa = slot->query_dsa;
  slot->err_message_dsa = InvalidDsaPointer;
  slot->query_dsa = InvalidDsaPointer;
  pg_write_barrier();

• Files reviewed: 7/7 changed files
• Comments generated: 1
• Review effort level: Lite

[cursor]

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit d296d40. Configure here.}

[serprex]

split out formatting change #128

[Copilot]

🟡 Changes recommended

Interrupted enqueue/dequeue regression coverage is still needed, and one stale function reference should be corrected.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

src/queue/query_intern.c:337

• The helper was renamed from ReleaseRef to PschQueryInternRelease, but the explanatory comment immediately above still names ReleaseRef, which no longer exists. Update that reference so the documented independent-release behavior points to the actual function.

  PschQueryInternRelease(ref);

• Files reviewed: 7/7 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

Copilot review overview

🟡 Changes recommended

Critical and moderate queue ownership issues remain unresolved, and retry cleanup lacks regression coverage.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 · 1

Open (2)

• Add read barrier before publishing shared slot pointers · New
• Free error message allocation on enqueue failure · New

Resolved since last review (1)

• The new interruption-safety path is not covered by the existing queue/interner TAP tests: those…