On Mon, Sep 27, 2021 at 12:46 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Fri, Sep 24, 2021 at 7:01 AM Masahiko Sawada <sawada.m...@gmail.com> wrote: > > > > On Fri, Sep 3, 2021 at 4:33 AM Mark Dilger <mark.dil...@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. Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/
