Re: subscriptioncheck failure

On Thu, May 13, 2021 at 7:06 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> On Thu, May 13, 2021 at 4:41 PM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
> >
> > On Thu, May 13, 2021 at 04:14:55PM +0530, vignesh C wrote:
> > > +$node_publisher->wait_for_catchup('tap_sub');
> > > +
> > > # Ensure a transactional logical decoding message shows up on the slot
> > > $node_subscriber->safe_psql('postgres', "ALTER SUBSCRIPTION tap_sub DISABLE");
> > >
> > > # wait for the replication connection to drop from the publisher
> > > $node_publisher->poll_query_until('postgres',
> > > - 'SELECT COUNT(*) FROM pg_catalog.pg_stat_replication', 0);
> > > + "SELECT COUNT(*) FROM pg_catalog.pg_replication_slots WHERE slot_name = 'tap_sub' AND active='f'", 1);
> >
> > There are three places in this test where a slot is disabled, followed
> > by a wait to make sure that the slot is gone. Perhaps it would be
> > better to wrap that in a small-ish routine?
>
> Thanks for the comments, Please find the attached patch having the changes.
>

Few comments:
1.
+ # Ensure a transactional logical decoding message shows up on the slot
+ $node_subscriber->safe_psql('postgres', "ALTER SUBSCRIPTION tap_sub DISABLE");

After you have encapsulated this command in the function, the above
comment doesn't make sense because we do this for both transactional
and non-transactional messages. I suggest we can change it to
something like: "This is done to ensure a logical decoding message is
shown up on the slot".

2.
+# Setup the subscription before checking pg_logical_slot_peek_binary_changes
+sub setup_subscription

I think here the functionality is more for the catchup of
subscription, so it might be better to name the function as
subscription_catchup or catchup_subscription. I think you can expand
the comments atop this function a bit as suggested by Michael.

--
With Regards,
Amit Kapila.