On Wed, Apr 21, 2021 at 3:09 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Tue, Apr 20, 2021 at 7:54 PM Masahiko Sawada <sawada.m...@gmail.com> wrote: > > > > > > I've attached the patch. In addition to the test Vignesh prepared, I > > added one test for the message for creating a slot that checks if the > > statistics are initialized after re-creating the same name slot. > > > > I am not sure how much useful your new test is because you are testing > it for slot name for which we have removed the slot file. It is not > related to stat messages this patch is sending. I think we can leave > that for now.
I might be missing something but I think the test is related to the message for creating a slot that initializes all counters. No? If there is no that message, we will end up getting old stats if a message for dropping slot gets lost (simulated by dropping slot file) and the same name slot is created. > One other minor comment: > > - * create the statistics for the replication slot. > + * create the statistics for the replication slot. In the cases where the > + * message for dropping the old slot gets lost and a slot with the same > + * name is created, since the stats will be initialized by the message > + * for creating the slot the statistics are not accumulated into the > + * old slot unless the messages for both creating and dropping slots with > + * the same name got lost. Just in case it happens, the user can reset > + * the particular stats by pg_stat_reset_replication_slot(). > > I think we can change it to something like: " XXX In case, the > messages for creation and drop slot of the same name get lost and > create happens before (auto)vacuum cleans up the dead slot, the stats > will be accumulated into the old slot. One can imagine having OIDs for > each slot to avoid the accumulation of stats but that doesn't seem > worth doing as in practice this won't happen frequently.". Also, I am > not sure after your recent change whether it is a good idea to mention > something in docs. What do you think? Both points make sense to me. I'll update the comment and remove the mention in the doc in the next version patch. Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/