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: Arseniy Mukhin <arseniy.mukhin.dev@gmail.com>
Cc: Álvaro Herrera <alvherre@kurilemu.de>, Joel Jacobson <joel@compiler.org>, Tom Lane <tgl@sss.pgh.pa.us>, pgsql-hackers <pgsql-hackers@postgresql.org>
Date: 2025-11-07T08:10:25Z
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

Attachments

On 06/11/2025 17:13, Arseniy Mukhin wrote:
> Let's say we have the queue:
> 
> (tail ... pos1 ... bad_entry_pos ... head)
> 
> bad_entry_pos - position of the entry where TransactionIdDidCommit fails.
> 
> We have the listener L1 with pos = pos1. It means every new listener
> should process the queue from pos1 (as it's max(listener's pos)) up to
> the queue head and when they try to do it, they will fail on
> 'bad_entry'.

Gotcha.

>> Another small change we could make is to check for listenChannels == NIL
>> before calling TransactionIdDidCommit.
> 
> There is a comment that describes why we need this initial reading in
> Exec_ListenPreCommit:
> 
>      /*
>       * This is our first LISTEN, so establish our pointer.
>       *
>       * We set our pointer to the global tail pointer and then move it forward
>       * over already-committed notifications.  This ensures we cannot miss any
>       * not-yet-committed notifications.  We might get a few more but that
>       * doesn't hurt.
> 
> It seems that with such a check we will skip not-yet-committed
> notifications and always get to the HEAD. If we decide that it's ok,
> maybe we can just set 'pos' of every new listener to the HEAD without
> reading?

Right, I didn't mean skipping asyncQueueReadAllNotifications() 
altogether. Just the TransactionIdDidCommit() calls in it, when 
listenChannels == NIL, see attached patch. The idea is that when 
'listenChannels == NIL', we're not going send the notification to the 
frontend regardless of what TransactionIdDidCommit says. If we don't 
call TransactionIdDidCommit, we won't fail on bad entries.

I'm not sure how much this matters. We really shouldn't have bad entries 
in the queue to begin with. And if we do, a broken entry could cause all 
kinds of trouble. For example, if the broken entry's (bogus) XID is 
higher than current XID, XidInMVCCSnapshot() will return true and we'll 
get stuck on that.

- Heikki