RE: Resetting spilled txn statistics in pg_stat_replication

Sawada-san, Thank you your comments.

The attached patch reflects the comment.
I also made a fix for the regression test.

Regards,
Noriyoshi Shinoda

-----Original Message-----
From: Masahiko Sawada [mailto:masahiko.sawada@2ndquadrant.com] 
Sent: Monday, October 12, 2020 8:12 PM
To: Shinoda, Noriyoshi (PN Japan A&PS Delivery) <noriyoshi.shinoda@hpe.com>
Cc: Amit Kapila <amit.kapila16@gmail.com>; Dilip Kumar <dilipbalaut@gmail.com>; Magnus Hagander <magnus@hagander.net>;
TomasVondra <tomas.vondra@2ndquadrant.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; Ajin Cherian
<itsajin@gmail.com>
Subject: Re: Resetting spilled txn statistics in pg_stat_replication

On Mon, 12 Oct 2020 at 18:29, Shinoda, Noriyoshi (PN Japan A&PS
Delivery) <noriyoshi.shinoda@hpe.com> wrote:
>
> Hi, thank you for the awesome feature.
>

Thank you for reporting!

> As it may have been discussed, I think the 'name' column in pg_stat_replication_slots is more consistent with the
columnname and data type matched to the pg_replication_slots catalog.
 
> The attached patch changes the name and data type of the 'name' column to slot_name and 'name' type, respectively.

It seems a good idea to me. In other system views, we use the name data type for object name. When I wrote the first
patch,I borrowed the code for pg_stat_slru which uses text data for the name but I think it's an oversight.
 

> Also, the macro name PG_STAT_GET_..._CLOS has been changed to PG_STAT_GET_..._COLS.

Good catch!

Here is my comments on the patch:

--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -798,7 +798,7 @@ CREATE VIEW pg_stat_replication AS

 CREATE VIEW pg_stat_replication_slots AS
     SELECT
-            s.name,
+            s.name AS slot_name,
             s.spill_txns,
             s.spill_count,
             s.spill_bytes,

I think we should modify 'proargnames' of
pg_stat_get_replication_slots() in pg_proc.dat instead.

---
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -7094,7 +7094,7 @@ pgstat_replslot_index(const char *name, bool create_it)
    Assert(nReplSlotStats <= max_replication_slots);
    for (i = 0; i < nReplSlotStats; i++)
    {
-       if (strcmp(replSlotStats[i].slotname, name) == 0)
+       if (strcmp(replSlotStats[i].slotname.data, name) == 0)
            return i;           /* found */
    }

@@ -7107,7 +7107,7 @@ pgstat_replslot_index(const char *name, bool create_it)

    /* Register new slot */
    memset(&replSlotStats[nReplSlotStats], 0, sizeof(PgStat_ReplSlotStats));
-   memcpy(&replSlotStats[nReplSlotStats].slotname, name, NAMEDATALEN);
+   memcpy(&replSlotStats[nReplSlotStats].slotname.data, name, 
+ NAMEDATALEN);

    return nReplSlotStats++;
 }

We can use NameStr() instead.

---
Perhaps we need to update the regression test as well.

Regards,

--
Masahiko Sawada            http://www.2ndQuadrant.com/ 
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services