Re: Failed transaction statistics to measure the logical replication progress

On Tue, Nov 9, 2021 at 5:05 PM osumi(dot)takamichi(at)fujitsu(dot)com
<osumi(dot)takamichi(at)fujitsu(dot)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