On Fri, Sep 25, 2020 at 4:33 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Thu, Sep 24, 2020 at 5:44 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > > > On Sat, Sep 19, 2020 at 1:48 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > > > > > On Tue, Sep 8, 2020 at 7:02 PM Amit Kapila <amit.kapil...@gmail.com> > > > wrote: > > > > > > > > On Tue, Sep 8, 2020 at 7:53 AM Masahiko Sawada > > > > <masahiko.saw...@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
