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: "Arseniy Mukhin" <arseniy.mukhin.dev@gmail.com>
Cc: "Masahiko Sawada" <sawada.mshk@gmail.com>, "Rishu Bagga" <rishu.postgres@gmail.com>, "Yura Sokolov" <y.sokolov@postgrespro.ru>, "Daniil Davydov" <3danissimo@gmail.com>, Álvaro Herrera <alvherre@kurilemu.de>, "Alexandra Wang" <alexandra.wang.oss@gmail.com>, "PostgreSQL Hackers" <pgsql-hackers@postgresql.org>, "Tom Lane" <tgl@sss.pgh.pa.us>, "Joel Jacobson" <joel@compiler.org>
Date: 2025-09-24T19:23:08Z
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 Wed Sep 24, 2025 at 10:59 AM -03, Arseniy Mukhin wrote:
> Thank you! Speaking of the scenario, my understanding is that it's
> impossible as we hold the global lock, so QueuePosition head should
> always be equal to QUEUE_HEAD when we get into At_AbortNotify(), but
> maybe I'm wrong.
>
Let's see if anyone else has any thoughts about this.

> Patch looks great. Some minor points:
>
> I have a warning when using git am with the patch:
>      warning: 1 line adds whitespace errors.
>
I don't think that this is a problem? And I don't know how to remove
this warning, I've just created the patch with git format-patch @~1

> There is a comment in the head of the async.c file about some
> listen/notify internals. Maybe it's worth adding a comment about how
> aborted transactions do clean up.
>
Added

> What do you think about a Assert in asyncQueueRollbackNotifications()
> that other backends still see us as 'in progress'? So we can be sure
> that they can't process our notifications before we mark notifications
> as 'committed=false'. Not sure how to do it correctly, maybe
>
>           Assert(TransactionIdIsValid(MyProc->xid));
>
> will work? The TAP test that I tried to write also should test it.
>
Sounds resanable to me. I think that we could use the following assert
when iterating over the entries, what do you think?
			Assert(TransactionIdIsInProgress(qe->xid));

> There are several comments where the word "crash" is used. What do you
> think about using "abort" instead? "Crash" sounds more like PANIC
> situation where we don't care about notifications because they don't
> survive restart.
>
Good point, fixed

> Thank you!
>
Thanks for reviewing this!

--
Matheus Alcantara