Feedback-ID: ic6394509:Fastmail
MIME-Version: 1.0
Date: Sat, 18 Oct 2025 07:43:15 +0200
From: "Joel Jacobson" <joel@compiler.org>
To: "Arseniy Mukhin" <arseniy.mukhin.dev@gmail.com>,
 =?UTF-8?Q?=C3=81lvaro_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>
Message-Id: <01fbde80-0ff6-4bf4-b942-cd60b76380d1@app.fastmail.com>
In-Reply-To: 
 <CAE7r3MKcd0xHdGL-47MkgumZ0Ebwib5V2+3TkG4ei6zKXXyJfg@mail.gmail.com>
References: 
 <CAE7r3MLoc9uDO2b542i_ZiksrB_iyvO1NKSzS7MOLdGzMuG=Kw@mail.gmail.com>
 <202510171347.twmkbitfsgfh@alvherre.pgsql>
 <CAE7r3MKcd0xHdGL-47MkgumZ0Ebwib5V2+3TkG4ei6zKXXyJfg@mail.gmail.com>
Subject: Re: LISTEN/NOTIFY bug: VACUUM sets frozenxid past a xid in async
 queue
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/01fbde80-0ff6-4bf4-b942-cd60b76380d1%40app.fastmail.com>
Precedence: bulk

On Fri, Oct 17, 2025, at 22:50, Arseniy Mukhin wrote:
> On Fri, Oct 17, 2025 at 7:31=E2=80=AFPM =C3=81lvaro 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]=C2=A0thread,
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-e=
6f669765f7e%40app.fastmail.com