On Fri, Sep 25, 2020 at 4:33 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Thu, Sep 24, 2020 at 5:44 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Sat, Sep 19, 2020 at 1:48 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Tue, Sep 8, 2020 at 7:02 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > >
> > > > On Tue, Sep 8, 2020 at 7:53 AM Masahiko Sawada
> > > > <masahiko.sawada@2ndquadrant.com> wrote:
> > >
> > > I have fixed these review comments in the attached patch.
> > >
> > >
> > > Apart from the above,
> > > (a) fixed one bug in ReorderBufferSerializeTXN() where we were
> > > updating the stats even when we have not spilled anything.
> > > (b) made changes in pgstat_read_db_statsfile_timestamp to return false
> > > when the replication slot entry is corrupt.
> > > (c) move the declaration and definitions in pgstat.c to make them
> > > consistent with existing code
> > > (d) made another couple of cosmetic fixes and changed a few comments
> > > (e) Tested the patch by using a guc which allows spilling all the
> > > changes. See v4-0001-guc-always-spill
> > >
> >
> > I have found a way to write the test case for this patch. This is
> > based on the idea we used in stats.sql. As of now, I have kept the
> > test as a separate patch. We can decide to commit the test part
> > separately as it is slightly timing dependent but OTOH as it is based
> > on existing logic in stats.sql so there shouldn't be much problem. I
> > have not changed anything apart from the test patch in this version.
> > Note that the first patch is just a debugging kind of tool to test the
> > patch.
> >
>
> I have done some more testing of this patch especially for the case
> where we spill before streaming the transaction and found everything
> is working as expected. Additionally, I have changed a few more
> comments and ran pgindent. I am still not very sure whether we want to
> display physical slots in this view as all the stats are for logical
> slots but anyway we can add stats w.r.t physical slots in the future.
> I am fine either way (don't show physical slots in this view or show
> them but keep stats as 0). Let me know if you have any thoughts on
> these points, other than that I am happy with the current state of the
> patch.
IMHO, It will make more sense to only show the logical replication
slots in this view.
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
