Re: Failed transaction statistics to measure the logical replication progress

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