Horton

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

Matheus Alcantara <matheusssilv97@gmail.com>

View thread
From: "Matheus Alcantara" <matheusssilv97@gmail.com>
To: "Joel Jacobson" <joel@compiler.org>, "Arseniy Mukhin" <arseniy.mukhin.dev@gmail.com>, Álvaro Herrera <alvherre@kurilemu.de>
Cc: "Matheus Alcantara" <matheusssilv97@gmail.com>, "Masahiko Sawada" <sawada.mshk@gmail.com>, "Rishu Bagga" <rishu.postgres@gmail.com>, "Yura Sokolov" <y.sokolov@postgrespro.ru>, "Daniil Davydov" <3danissimo@gmail.com>, "Alexandra Wang" <alexandra.wang.oss@gmail.com>, "pgsql-hackers" <pgsql-hackers@postgresql.org>, "Tom Lane" <tgl@sss.pgh.pa.us>
Date: 2025-10-20T12:37:24Z
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 Sat Oct 18, 2025 at 2:43 AM -03, Joel Jacobson wrote:
> On Fri, Oct 17, 2025, at 22:50, Arseniy Mukhin wrote:
> What a funny coincidence that the approach in this patch,
> has one similarity with the "Direct advancement" approach
> in the patch in the "Optimize LISTEN/NOTIFY" [1] thread,
> namely that we're both interested in QUEUE_HEAD before/after
> we push the notifications into the queue, in PreCommit_Notify().
>
> It looks to me like our new data structures are Interchangeable,
> so I guess we probably want both patches to eventually settle
> on one and the same?
>
> The differences I note between our queue head before/after code are:
>
> - In this patch, you are palloc'ing a struct with two fields.
>   In [1], we're using two separate static QueuePosition variables.
>
> - In this patch, you are taking/releasing a shared lock before/after
>   the loop to read QUEUE_HEAD and set previousHead/head.
>   In [1], we avoid the need of the shared lock, by doing the reads
>   within the existing exclusive lock inside the loop, but instead
>   therefore need a firstIteration bool, to know which is the first
>   iteration, and need to overwrite the after-var in each iteration.
>
> I don't think the noted differences above matter, both seems fine.
>
Yeah, I also think that both approach seems fine. I keep the v8 version
with the palloc, if someone has any concern about this I'm open to
switch to another approach.

> Another thing I noticed in your patch that made me wonder,
> is the naming of the new AsyncQueueEntry bool field,
> which is given the name "committed".
>
> I think this name is not entirely faithful, since when set to true,
> the entry has not been committed yet.
>
> How about negating the meaning of this boolean field?
> To instead indicate when the entry has been rollbacked.
> Then, it would clearly communicate just that.
>
> Maybe naming it something like "rollbacked" or "aborted"?
>
Good point. I've renamed this field on the attached v8 version.

--
Matheus Alcantara