Re: Introduce wait_for_subscription_sync for TAP tests

On Wed, Jul 27, 2022 at 8:54 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Tue, Jul 26, 2022 at 1:12 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> >
> > On Tue, Jul 26, 2022 at 2:01 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > >
> > > 2.
> > > + # wait for the replication to catchup if required.
> > > + if (defined($publisher))
> > > + {
> > > + croak 'subscription name must be specified' unless defined($subname);
> > > + $publisher->wait_for_catchup($subname, 'replay');
> > > + }
> > > +
> > > + # then, wait for all table states to be ready.
> > > + print "Waiting for all subscriptions in \"$name\" to synchronize data\n";
> > > + my $query = qq[SELECT count(1) = 0
> > > + FROM pg_subscription_rel
> > > + WHERE srsubstate NOT IN ('r', 's');];
> > > + $self->poll_query_until($dbname, $query)
> > > + or croak "timed out waiting for subscriber to synchronize data";
> > >
> > > In the tests, I noticed that a few places did wait_for_catchup after
> > > the subscription check, and at other places, we did that check before
> > > as you have it here. Ideally, I think wait_for_catchup should be after
> > > confirming the initial sync is over as without initial sync, the
> > > publisher node won't be completely in sync with the subscriber.
> >
> > What do you mean by the last sentence? I thought the order doesn't
> > matter here. Even if we do wait_for_catchup first then the
> > subscription check, we can make sure that the apply worker caught up
> > and table synchronization has been done, no?
> >
>
> That's right. I thought we should first ensure the subscriber has
> finished operations if possible, like in this case, it can ensure
> table sync has finished and then we can ensure whether publisher and
> subscriber are in sync. That sounds more logical to me.

Make sense. I've incorporated it in the v3 patch.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/