Mime-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset=UTF-8
Date: Fri, 19 Sep 2025 11:36:43 -0300
Message-Id: <DCWUPJ39T21K.VY51UY0K8LE1@gmail.com>
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>, =?utf-8?q?=C3=81lvaro_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>
Subject: Re: LISTEN/NOTIFY bug: VACUUM sets frozenxid past a xid in async
 queue
References: 
 <CAK98qZ3wZLE-RZJN_Y+TFjiTRPPFPBwNBpBi5K5CU8hUHkzDpw@mail.gmail.com>
 <202508061044.ptcyt7aqsaaa@alvherre.pgsql>
 <DBZN3PVLZ1UW.25XLKCUXYBVXV@gmail.com>
 <CAJDiXgg1ArRB1-6wLtXfVVnQ38P9Y+CDfEc9M2TXiOf_4kfBMg@mail.gmail.com>
 <CAFY6G8cnDOMZCfSvc62vER_9+HOs4EM52XromhOZki+=Ytv2sg@mail.gmail.com>
 <CAJDiXgheRHTnK8RJuBBcF0VjQA83wi81oX2s-GvBV3x+eSwBbA@mail.gmail.com>
 <CAFY6G8dyimRikP3nEK5gqMPnMZe5RTZjFvKvCFLJAvsajxK1fg@mail.gmail.com>
 <CAJDiXgjQZgHLFziG6sS6D5CGYqohMio673ydiRkkffW3WfXmEg@mail.gmail.com>
 <DC7KGTXW3QSG.OZA24HONT78J@gmail.com>
 <CAD21AoCD+HXoc2QZCAS9d8ahDeikNqbnU0i6cQzpMFOEurkPPg@mail.gmail.com>
 <DC8HKDZVIDYG.12H0X267CHMO5@gmail.com>
 <CAFY6G8cJm73_MM9SuynZUqtqcaTuepUDgDuvS661oLW7U0dgsg@mail.gmail.com>
 <d186fba0-dc65-4274-aa96-3906bbb2e530@postgrespro.ru>
 <CAE7r3M+Xwf0A_aNhu7pYQd7nDQaqaEnyCjtvqg8XsgManmPOUA@mail.gmail.com>
 <DCJH5HX8X3B4.O9P9P8VKKO1F@gmail.com>
 <CAK80=jhxduhJmgSWY8JK4Aen8qshPGHFLE-J_ix6EN9AK6+sQQ@mail.gmail.com>
 <CAFY6G8f0JEF3HE4Q+ZZYjpPiFfRREzJ+r6zbJh2ZUFb2jhqm7g@mail.gmail.com>
 <CAE7r3MKiENVC==S8riqNPiMcgOp41npCDsNZFg=QXANd1Tajfg@mail.gmail.com>
 <DCLS3QKXDT0W.364G3WTVMLGYR@gmail.com>
 <CAD21AoBAcr6nAx5C4soYO3iR9dda_k8hFLok1Psd0R070qs9hg@mail.gmail.com>
 <CAFY6G8fui7omKfUXF+JJ82B34ExrwK6J7vKe61S-DhEmr_jBhA@mail.gmail.com>
 <CAE7r3M+=oOhDSmSihqGvdFzfgekF+6KibEXUJdCK7DdFTA8uPQ@mail.gmail.com>
In-Reply-To: 
 <CAE7r3M+=oOhDSmSihqGvdFzfgekF+6KibEXUJdCK7DdFTA8uPQ@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/DCWUPJ39T21K.VY51UY0K8LE1%40gmail.com>
Precedence: bulk

On Fri Sep 19, 2025 at 9:34 AM -03, Arseniy Mukhin wrote:
> Hi,
>
Thanks for taking a look at this!

> I like this approach. We got rid of dependency on clog and don't limit
> vacuum. Several points about the fix:
>
> Is it correct to remember and reuse slru slots here? IIUC we can't do
> it if we don't hold SLRU bank lock, because by the time we get in
> AtAbort_Notify() the queue page could be already evicted. Probably we
> need to use SimpleLruReadPage after we acquire the lock in
> AtAbort_Notify()?
>
Yeah, I think it make sense. Maybe we could just store the page number
and offset and call SimpleLruReadPage() inside AtAbort_Notify() to get
the slotno?

> I think adding a boolean 'committed' is a good approach, but what do
> you think about setting the queue head back to the position where
> aborted transaction notifications start? We can do such a reset in
> AtAbort_Notify(). So instead of marking notifications as
> 'commited=3Dfalse' we completely erase them from the queue by moving the
> queue head back. From listeners perspective if there is a notification
> of completed transaction in the queue - it's always a committed
> transaction, so again get rid of TransactionIdDidCommit() call. It
> seems like a simpler approach because we don't need to remember all
> notifications positions in the queue and don't need the additional
> field 'committed'. All we need is to remember the head position before
> we write anything to the queue, and reset it back if there is an
> abort. IIUC Listeners will never send such erased notifications:
> - while the aborted transaction is looking like 'in progress',
> listeners can't send its notifications.
> - by the time the aborted transaction is completed, the head is
> already set back so erased notifications are located after the queue
> head and listeners can't read it.
>
I think that with this approach we may have a scenario where when we
enter the AtAbort_Notify() the queue head may not be the notification
from the transaction being aborted but it can be a notification from
another committed transaction, so resetting the queue head back to the
position before the aborted transaction would make us lose the
notification from the committed transaction. Is that make sense? What do
you think?

> I think it's a good test to have. FWIW there is a way to reproduce the
> test condition without the injection point. We can use the fact that
> serializable conflicts are checked after tx adds notifications to the
> queue. Please find the attached patch with the example tap test. Not
> sure if using injections points is more preferable?
>
Great, I think that if we can have a way to reproduce this without an
injection point would be better. I dind't look the test yet but I'll try
to incorporate this on the next patch version. Thanks!

--
Matheus Alcantara