MIME-Version: 1.0
References: 
 <TYCPR01MB8373DC922E549FF3F031E9DAED8E9@TYCPR01MB8373.jpnprd01.prod.outlook.com>
 <CAJcOf-cMU_1VJ1c+9fwEBgimkj+=hHcCjj_SUfbHvKTuN=1tuw@mail.gmail.com>
 <TYCPR01MB8373B1614522B90BA96883E0ED929@TYCPR01MB8373.jpnprd01.prod.outlook.com>
In-Reply-To: 
 <TYCPR01MB8373B1614522B90BA96883E0ED929@TYCPR01MB8373.jpnprd01.prod.outlook.com>
From: vignesh C <vignesh21@gmail.com>
Date: Wed, 10 Nov 2021 15:43:20 +0530
Message-ID: 
 <CALDaNm3-QTZnd4rxbrUfKMM4u_vgb23LO+KW3O9LX7X6b8mr8A@mail.gmail.com>
Subject: Re: Failed transaction statistics to measure the logical replication
 progress
To: "osumi.takamichi@fujitsu.com" <osumi.takamichi@fujitsu.com>
Cc: Greg Nancarrow <gregn4422@gmail.com>,
 Masahiko Sawada <sawada.mshk@gmail.com>,
	Amit Kapila <amit.kapila16@gmail.com>,
 "houzj.fnst@fujitsu.com" <houzj.fnst@fujitsu.com>,
	"pgsql-hackers@lists.postgresql.org" <pgsql-hackers@lists.postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Archived-At: 
 <https://www.postgresql.org/message-id/CALDaNm3-QTZnd4rxbrUfKMM4u_vgb23LO%2BKW3O9LX7X6b8mr8A%40mail.gmail.com>
Precedence: bulk

On Tue, Nov 9, 2021 at 5:05 PM osumi.takamichi@fujitsu.com
<osumi.takamichi@fujitsu.com> wrote:
> Yes. I've rebased and updated the patch, paying attention to this point.
> Attached the updated version.

Thanks for the updated patch, few comments:
1) you could rename PgStat_StatSubWorkerPreparedXact to
PgStat_SW_PreparedXactKey or a simpler name which includes key and
similarly change PgStat_StatSubWorkerPreparedXactSize to
PgStat_SW_PreparedXactEntry

+/* prepared transaction */
+typedef struct PgStat_StatSubWorkerPreparedXact
+{
+       Oid                     subid;
+       char            gid[GIDSIZE];
+} PgStat_StatSubWorkerPreparedXact;
+
+typedef struct PgStat_StatSubWorkerPreparedXactSize
+{
+       PgStat_StatSubWorkerPreparedXact key; /* hash key */
+
+       Oid                     subid;
+       char            gid[GIDSIZE];
+       PgStat_Counter xact_size;
+} PgStat_StatSubWorkerPreparedXactSize;
+

2) You can change prepared_size to sw_prepared_xact_entry or
prepared_xact_entry since it is a hash entry with few fields
+       if (subWorkerPreparedXactSizeHash)
+       {
+               PgStat_StatSubWorkerPreparedXactSize *prepared_size;
+
+               hash_seq_init(&hstat, subWorkerPreparedXactSizeHash);
+               while((prepared_size =
(PgStat_StatSubWorkerPreparedXactSize *) hash_seq_search(&hstat)) !=
NULL)
+               {
+                       fputc('P', fpout);
+                       rc = fwrite(prepared_size,
sizeof(PgStat_StatSubWorkerPreparedXactSize), 1, fpout);
+                       (void) rc;                      /* we'll check
for error with ferror */
+               }

3) This need to be indented
-        w.relid,
-        w.command,
-        w.xid,
-        w.error_count,
-        w.error_message,
-        w.last_error_time
+       w.commit_count,
+       w.commit_bytes,
+       w.error_count,
+       w.error_bytes,
+       w.abort_count,
+       w.abort_bytes,
+       w.last_error_relid,
+       w.last_error_command,
+       w.last_error_xid,
+       w.last_error_count,
+       w.last_error_message,
+       w.last_error_time

4) Instead of adding a function to calculate the size, can we move
PartitionTupleRouting from c file to the header file and use sizeof at
the caller function?
+/*
+ * PartitionTupleRoutingSize - exported to calculate total data size
+ * of logical replication mesage apply, because this is one of the
+ * ApplyExecutionData struct members.
+ */
+size_t
+PartitionTupleRoutingSize(void)
+{
+       return sizeof(PartitionTupleRouting);
+}

5) You could run pgindent and pgperltidy for the code and test code to
fix the indent issues.
+
subWorkerPreparedXactSizeHash = hash_create("Subscription worker stats
of prepared txn",
+
                                         PGSTAT_SUBWORKER_HASH_SIZE,
+
                                         &hash_ctl,
+
                                         HASH_ELEM | HASH_STRINGS |
HASH_CONTEXT);

+# There's no entry at the beginning
+my $result = $node_subscriber->safe_psql('postgres',
+"SELECT count(*) FROM pg_stat_subscription_workers;");
+is($result, q(0), 'no entry for transaction stats yet');

6) Few places you have used strlcpy and few places you have used
memcpy, you can keep it consistent:
+       msg.m_command = command;
+       strlcpy(msg.m_gid, gid, sizeof(msg.m_gid));
+       msg.m_xact_bytes = xact_size;

+       key.subid = subid;
+       memcpy(key.gid, gid, sizeof(key.gid));
+       action = (create ? HASH_ENTER : HASH_FIND);

Regards,
Vignesh