Horton

Re: LISTEN/NOTIFY bug: VACUUM sets frozenxid past a xid in async queue

Heikki Linnakangas <hlinnaka@iki.fi>

View thread
From: Heikki Linnakangas <hlinnaka@iki.fi>
To: Joel Jacobson <joel@compiler.org>, pgsql-hackers <pgsql-hackers@postgresql.org>
Date: 2025-10-29T11:40:34Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Clear 'xid' in dummy async notify entries written to fill up pages

  2. Fix remaining race condition with CLOG truncation and LISTEN/NOTIFY

  3. Fix bug where we truncated CLOG that was still needed by LISTEN/NOTIFY

  4. Escalate ERRORs during async notify processing to FATAL

  5. Limit the size of TID lists during parallel GIN build

On 25/10/2025 21:01, Joel Jacobson wrote:
> On Sat, Oct 25, 2025, at 15:08, Arseniy Mukhin wrote:
>> I reimplemented the test in 0002 as an isolation test and added the
>> commit message. PFA the new version.
> 
> I've rebased the patches so they can be applied on top of v12:
> - v12-vacuum_notify_queue_cleanup-without-code-dup.txt
> - v12-vacuum_notify_queue_cleanup-with-code-dup.txt
> 
> I also want to share a new idea that Heikki Linnakangas came up with
> during PgConf25 in Riga.
> 
> The idea is basically to scan the notification queue from tail to head,
> and update xids that precedes newFrozenXid, to either
> FrozenTransactionId if committed, or InvalidTransactionId if not.
> 
> I've implemented this by adding a new extern function
> AsyncNotifyFreezeXids to async.h, called from vac_update_datfrozenxid.
> 
> This way, we don't need to hold back the advancement of newFrozenXid in
> vac_update_datfrozenxid.
> 
> Attempt of implementing Heikki's idea:
> - async_notify_freeze_xids.txt

Thanks!

> This idea has the benefit of never holding back newFrozenXid,
> however, I see some problems with it:
> 
> - If there is a bug in the code, we could accidentally overwrite
>    xid values we didn't intend to overwrite, and maybe we would never
>    find out that we did.
> 
> - We wouldn't know for sure that we've understood the cause of
>    this bug.
> 
> With v12-vacuum_notify_queue_cleanup, we instead have the downside of
> possibly holding back newFrozenXid, but with the advantage of giving us
> higher confidence in that we've correctly identified the cause of the
> bug.

Meh. Robustness is good and all, and in heap tuples we don't overwrite 
the xmin/xmax but just set a FROZEN flag, precisely so that we preserve 
evidence if something goes wrong. But I can't get too worked up about 
that for the async notification queue.

That said, we could add COMMITTED/INVALID hint bits to AsyncQueueEntry, 
similar to heap tuples, and set the hint bit instead of replacing the 
original xid. That might be good for performance too: If the first 
backend to call TransactionIdDidCommit() on an AsyncQueueEntry would set 
the hint bit, that would save the effort for other listening backends.

- Heikki