Re: Replication slot stats misgivings

On Fri, Apr 2, 2021 at 9:57 AM vignesh C <vignesh21@gmail.com> wrote:
> Thanks for the comments, I will fix the comments and provide a patch
> for this soon.

Here are some comments:
1) How about something like below
+                            (errmsg("skipping \"%s\" replication slot
statistics as the statistic collector process does not have enough
statistic slots",
instead of
+                            (errmsg("skipping \"%s\" replication
slot's statistic as the statistic collector process does not have
enough statistic slots",

2) Does it mean "pg_statistic slots" when we say "statistic slots" in
the above warning? If yes, why can't we use "pg_statistic slots"
instead of "statistic slots" as with another existing message
"insufficient pg_statistic slots for array stats"?

3) Should we change the if condition to max_replication_slots <=
nReplSlotStats instead of max_replication_slots == nReplSlotStats? In
the scenario, it is mentioned that "one of the replication slots is
dropped", will this issue occur when multiple replication slots are
dropped?

4) Let's end the statement after this and start a new one, something like below
+                 * this. To avoid writing beyond the max_replication_slots
instead of
+                 * this, to avoid writing beyond the max_replication_slots

5) How about something like below
+                 * this. To avoid writing beyond the max_replication_slots,
+                 * this replication slot statistics information will
be skipped.
+                 */
instead of
+                 * this, to avoid writing beyond the max_replication_slots
+                 * these replication slot statistic information will
be skipped.
+                 */

6) Any specific reason to use a new local variable replSlotStat and
later memcpy into replSlotStats[nReplSlotStats]? Instead we could
directly fread into &replSlotStats[nReplSlotStats] and do
memset(&replSlotStats[nReplSlotStats], 0,
sizeof(PgStat_ReplSlotStats)); before the warnings. As warning
scenarios seem to be less frequent, we could avoid doing memcpy
always.
-                if (fread(&replSlotStats[nReplSlotStats], 1,
sizeof(PgStat_ReplSlotStats), fpin)
+                if (fread(&replSlotStat, 1, sizeof(PgStat_ReplSlotStats), fpin)

+                memcpy(&replSlotStats[nReplSlotStats], &replSlotStat,
sizeof(PgStat_ReplSlotStats));

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com