On Tue, Apr 20, 2021 at 6:59 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, Apr 20, 2021 at 9:08 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > I've attached the new version patch that fixed the compilation error
> > reported off-line by Amit.
> >
>
> I was thinking about whether we can someway avoid the below risk:
> In case where the
> + * message for dropping the old slot gets lost and a slot with the same
> + * name is created, the stats will be accumulated into the old slots since
> + * we use the slot name as the key. In that case, user can reset the
> + * particular stats by pg_stat_reset_replication_slot().
>
> What if we send a separate message for create slot such that the stats
> collector will initialize the entries even if the previous drop
> message is lost or came later? If we do that then if the drop message
> is lost, the create with same name won't accumulate the stats and if
> the drop came later, it will remove the newly created stats but
> anyway, later stats from the same slot will again create the slot
> entry in the hash table.
Sounds good to me. There is still little chance to happen if messages
for both creating and dropping slots with the same name got lost, but
it's unlikely to happen in practice.
>
> Also, I think we can include the test case prepared by Vignesh in the email [1].
>
> Apart from the above, I have made few minor modifications in the attached patch.
> (a) + if (slotent->stat_reset_timestamp == 0 || !slotent)
> I don't understand why second part of check is required? By this time
> slotent will anyway have some valid value.
>
> (b) + slotent = (PgStat_StatReplSlotEntry *) hash_search(replSlotStats,
> + (void *) &name,
> + create_it ? HASH_ENTER : HASH_FIND,
> + &found);
>
> It is better to use NameStr here.
>
> (c) made various changes in comments and some other cosmetic changes.
All the above changes make sense to me.
I'll submit the updated patch soon.
Regards,
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/