On Mon, Sep 27, 2021 at 11:20 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Mon, Sep 27, 2021 at 12:46 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Fri, Sep 24, 2021 at 7:01 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > >
> > > On Fri, Sep 3, 2021 at 4:33 AM Mark Dilger <mark.dilger@enterprisedb.com> wrote:
> > >
> > > > I am attaching a version of such a function, plus some tests of your patch (since it does not appear to have
any). Would you mind reviewing these and giving comments or including them in your next patch version?
> > > >
> > >
> > > I've looked at the patch and here are some comments:
> > >
> > > +
> > > +-- no errors should be reported
> > > +SELECT * FROM pg_stat_subscription_errors;
> > > +
> > >
> > > +
> > > +-- Test that the subscription errors view exists, and has the right columns
> > > +-- If we expected any rows to exist, we would need to filter out unstable
> > > +-- columns. But since there should be no errors, we just select them all.
> > > +select * from pg_stat_subscription_errors;
> > >
> > > The patch adds checks of pg_stat_subscription_errors in order to test
> > > if the subscription doesn't have any error. But since the subscription
> > > errors are updated in an asynchronous manner, we cannot say the
> > > subscription is working fine by checking the view only once.
> > >
> >
> > One question I have here is, can we reliably write few tests just for
> > the new view patch? Right now, it has no test, having a few tests will
> > be better. Here, because the apply worker will keep on failing till we
> > stop it or resolve the conflict, can we rely on that fact? The idea
> > is that even if one of the entry is missed by stats collector, a new
> > one (probably the same one) will be issued and we can wait till we see
> > one error in view. We can add additional PostgresNode.pm
> > infrastructure once the main patch is committed.
>
> Yes, the new tests added by 0003 patch (skip_xid patch) use that fact.
> After the error is shown in the view, we fetch the XID from the view
> to specify as skip_xid. The tests just for the
> pg_stat_subscription_errors view will be a subset of these tests. So
> probably we can add it in 0001 patch and 0003 patch can extend the
> tests so that it tests skip_xid option.
>
This makes sense to me.
--
With Regards,
Amit Kapila.