Re: Replication slot stats misgivings

On Thu, Apr 15, 2021 at 4:35 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> Thank you for the update! The patch looks good to me.
>

I have pushed the first patch. Comments on the next patch
v13-0001-Use-HTAB-for-replication-slot-statistics:
1.
+ /*
+ * Check for all replication slots in stats hash table. We do this check
+ * when replSlotStats has more than max_replication_slots entries, i.e,
+ * when there are stats for the already-dropped slot, to avoid frequent
+ * call SearchNamedReplicationSlot() which acquires LWLock.
+ */
+ if (replSlotStats && hash_get_num_entries(replSlotStats) >
max_replication_slots)
+ {
+ PgStat_ReplSlotEntry *slotentry;
+
+ hash_seq_init(&hstat, replSlotStats);
+ while ((slotentry = (PgStat_ReplSlotEntry *) hash_seq_search(&hstat)) != NULL)
+ {
+ if (SearchNamedReplicationSlot(NameStr(slotentry->slotname), true) == NULL)
+ pgstat_report_replslot_drop(NameStr(slotentry->slotname));
+ }
+ }

Is SearchNamedReplicationSlot() so frequently used that we need to do
this only when the hash table has entries more than
max_replication_slots? I think it would be better if we can do it
without such a condition to reduce the chances of missing the slot
stats. We don't have any such restrictions for any other cases in this
function.

I think it is better to add CHECK_FOR_INTERRUPTS in the above while loop?

2.
/*
  * Replication slot statistics kept in the stats collector
  */
-typedef struct PgStat_ReplSlotStats
+typedef struct PgStat_ReplSlotEntry

I think the comment above this structure can be changed to "The
collector's data per slot" or something like that. Also, if we have to
follow table/function/db style, then probably this structure should be
named as PgStat_StatReplSlotEntry.

3.
- * create the statistics for the replication slot.
+ * create the statistics for the replication slot. In case where the
+ * message for dropping the old slot gets lost and a slot with the same is

/the same is/the same name is/.

Can we mention something similar to what you have added here in docs as well?

4.
+CREATE VIEW pg_stat_replication_slots AS
+    SELECT
+            s.slot_name,
+            s.spill_txns,
+            s.spill_count,
+            s.spill_bytes,
+            s.stream_txns,
+            s.stream_count,
+            s.stream_bytes,
+            s.total_txns,
+            s.total_bytes,
+            s.stats_reset
+    FROM pg_replication_slots as r,
+        LATERAL pg_stat_get_replication_slot(slot_name) as s
+    WHERE r.datoid IS NOT NULL; -- excluding physical slots
..
..

-/* Get the statistics for the replication slots */
+/* Get the statistics for the replication slot */
 Datum
-pg_stat_get_replication_slots(PG_FUNCTION_ARGS)
+pg_stat_get_replication_slot(PG_FUNCTION_ARGS)
 {
 #define PG_STAT_GET_REPLICATION_SLOT_COLS 10
- ReturnSetInfo *rsinfo = (ReturnSetInfo *) fcinfo->resultinfo;
+ text *slotname_text = PG_GETARG_TEXT_P(0);
+ NameData slotname;

I think with the above changes getting all the slot stats has become
much costlier. Is there any reason why can't we get all the stats from
the new hash_table in one shot and return them to the user?

-- 
With Regards,
Amit Kapila.