Mime-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset=UTF-8
Date: Sat, 06 Sep 2025 11:14:54 -0300
Message-Id: <DCLS3QKXDT0W.364G3WTVMLGYR@gmail.com>
Cc: "Rishu Bagga" <rishu.postgres@gmail.com>, "Yura Sokolov"
 <y.sokolov@postgrespro.ru>, "Masahiko Sawada" <sawada.mshk@gmail.com>,
 "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
From: "Matheus Alcantara" <matheusssilv97@gmail.com>
To: "Arseniy Mukhin" <arseniy.mukhin.dev@gmail.com>
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>
In-Reply-To: 
 <CAE7r3MKiENVC==S8riqNPiMcgOp41npCDsNZFg=QXANd1Tajfg@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/DCLS3QKXDT0W.364G3WTVMLGYR%40gmail.com>
Precedence: bulk

On Fri Sep 5, 2025 at 9:56 AM -03, Arseniy Mukhin wrote:
>> Ok, I agree that that this may happen but I don't see this as a common
>> case to fix the issue based on this behaviour. I think that we check the
>> transaction status also to skip notifications that were added on the
>> queue by transactions that are not fully committed yet, and I see this
>> scenario as a more common case but I could be wrong.
>>
>
> IIUC we don't need clog to check if the notification transaction is
> still in progress, we use a snapshot for that (XidInMVCCSnapshot()).
> If we see that the notification transaction is still in progress, we
> postpone processing of that notification. If we see that notification
> transaction is not in progress, then we check its status with
> TransactionIdDidCommit() (using clog) and only then fail if clog
> doesn't have notification transcation data (as a quick check, we can
> see in the stack trace that the failure occurs during the call to
> TransactionIdDidCommit, not XidInMVCCSnapshot).
>
> So we need to check notification transaction status only to understand
> if a transaction was committed and we can send notification or it was
> aborted (somewhere in between PreCommit_Notify and AtCommit_Notify)
> and we should skip notification. And the solution that Yura Sokolov
> suggested is to introduce a 'commited' flag, so we can check it
> instead of calling TransactionIdDidCommit. Another option is to make
> aborted transactions to clean up after themselves in AtAbort_Notify(),
> so if listener see notification in the queue and transaction that
> wrote it is not in progress, then such notification was always created
> by committed transaction and we also don't need to call
> TransactionIdDidCommit. One way for aborted transactions to clean up
> is to set its notifications dbOid to 0, so listeners skip them.
> Another option I think is to set queue HEAD back to the position where
> it was when the aborted transaction started to write to the queue. It
> also makes such aborted transaction notifications 'invisible' for
> listeners. We have a global lock and there is always only one writer,
> so it sounds possible. And there is another option which is the patch
> that Rishu Bagga is working on. In the patch all transactions write to
> the listen/notify queue only after they wrote a commit record to the
> WAL, so all notifications in the queue are always by committed
> transactions and we don't need to call TransactionIdDidCommit.
>
You are right, I missed the XidInMVCCSnapshot() call, sorry about that
and thanks very much for all the explanation!

I'll keep on hold the development of new patch versions for this thread
and focus on review and test the patch from Rishu at [1] to see if we
can make progress using the WAL approach.


[1] https://www.postgresql.org/message-id/CAK80%3DjipUfGC%2BUQSzeA4oCP9daRt=
HZGm2SQZWLxC9NWmVTDtRQ%40mail.gmail.com

--
Matheus Alcantara