On Wed, Apr 21, 2021 at 3:09 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, Apr 20, 2021 at 7:54 PM Masahiko Sawada <sawada.mshk@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/
