Horton

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

Joel Jacobson <joel@compiler.org>

View thread
From: "Joel Jacobson" <joel@compiler.org>
To: "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-18T05:43:15Z
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 Fri, Oct 17, 2025, at 22:50, Arseniy Mukhin wrote:
> On Fri, Oct 17, 2025 at 7:31 PM Álvaro Herrera <alvherre@kurilemu.de> wrote:
>>
>> I have the impression that this thread has lost focus on the idea of
>> producing a backpatchable bugfix.  The last proposed patch has a lot of
>> new mechanism that doesn't seem suitable for backpatch.  I could be
>> wrong of course.
>>
>
> Oops, I guess the TAP test that I added in the last version and that
> uses injection points is one of those things. PFA the new version
> without it. I also noticed that CheckSharedObjectLockedByMe() which
> was introduced in one of previous versions is unused, so I removed it
> too.

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.

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"?

/Joel

[1] https://www.postgresql.org/message-id/flat/6899c044-4a82-49be-8117-e6f669765f7e%40app.fastmail.com