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

On Fri, Oct 24, 2025 at 2:43 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> On Wed, Oct 22, 2025 at 10:25 AM Matheus Alcantara
> <matheusssilv97(at)gmail(dot)com> wrote:
> >
> > On Wed Oct 22, 2025 at 12:49 PM -03, Álvaro Herrera wrote:
> > > On 2025-Oct-22, Matheus Alcantara wrote:
> > >
> > >> I'm wondering if the 002_aborted_tx_notifies.pl is still needed with
> > >> this architecture being used. I think that it's not, but perhaps is a
> > >> good test to keep it?
> > >
> > > I'd rather have tests than not, but I'd think it needs to be behind
> > > PG_TEST_EXTRA because of things like
> > >
> > > +$node->safe_psql('postgres', 'select consume_xids(10000000);');
> > >
> > Attached v10 with wrapping into PG_TEST_EXTRA.
>
> Thank you for updating the patches!
>
> I've reviewed the patches and here are the comments.

Thank you for the review!

> I would expect to add 002_aborted_tx_notifies.pl in a separate patch
> since it's not related to this bug fix.
>
> ---
> +# Test checks that listeners do not receive notifications from aborted
> +# transaction even if notifications have been added to the listen/notify
> +# queue. To reproduce it we use the fact that serializable conflicts
> +# are checked after tx adds notifications to the queue.
>
> I wonder if we could implement this test using the isolation test
> instead of the tap test. Is there any reason why you used a tap test
> for that?
>

I agree it's less relevant to the patch now than it was with the new
'committed' field approach. And there is no particular reason why it
was implemented as a TAP test actually.. So +1 to move it to separate
patch (does it mean to separate thread as well or just separate patch
file?) and rewrite as an isolation test (IIUC it's better to use
isolation test infrastructure if it's possible). I can try to do it if
nobody else does it earlier.

Best regards,
Arseniy Mukhin