On Friday, October 20, 2023 11:24 AM vignesh C <vignes...@gmail.com> wrote:
>
> On Thu, 19 Oct 2023 at 16:16, Hayato Kuroda (Fujitsu)
> <kuroda.hay...@fujitsu.com> wrote:
> >
> > Dear Vignesh,
> >
> > Thanks for revieing! New patch can be available in [1].
> >
> > > Few comments:
> > > 1) Even if we comment 3rd point "Emit a non-transactional message",
> > > test_slot2 still appears in the
> > > invalid_logical_replication_slots.txt
> > > file. There is something wrong here.
> > > + # 2. Advance the slot test_slot2 up to the current WAL location,
> but
> > > + # test_slot1 still has unconsumed WAL records.
> > > + $old_publisher->safe_psql('postgres',
> > > + "SELECT pg_replication_slot_advance('test_slot2',
> > > + NULL);");
> > > +
> > > + # 3. Emit a non-transactional message. test_slot2 detects
> > > + the message
> > > so
> > > + # that this slot will be also reported by upcoming
> pg_upgrade.
> > > + $old_publisher->safe_psql('postgres',
> > > + "SELECT count(*) FROM
> > > + pg_logical_emit_message('false',
> > > 'prefix', 'This is a non-transactional message');"
> > > + );
> >
> > The comment was updated based on others. How do you think?
>
> I mean if we comment or remove this statement like in the attached patch, the
> test is still passing with 'The slot "test_slot2" has not consumed the WAL
> yet', in
> this case should the test_slot2 be still invalid as we have called
> pg_replication_slot_advance for test_slot2.
It's because we pass NULL to pg_replication_slot_advance(). We should pass
pg_current_wal_lsn() instead. I have fixed it in V55 version.
Best Regards,
Hou zj
