Re: Replication slot stats misgivings

On Tue, Apr 20, 2021 at 9:08 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)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.

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.

[1] - https://www.postgresql.org/message-id/CALDaNm3yBctNFE6X2FV_haRF4uue9okm1_DVE6ZANWvOV_CvYw%40mail.gmail.com

--
With Regards,
Amit Kapila.