RE: Failed transaction statistics to measure the logical replication progress

On Wednesday, November 10, 2021 7:13 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> 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;
> +
Fixed. Adopted your suggested names.

> 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 */
> + }
I preferred prepared_xact_entry. Fixed.

> 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
Fixed.

> 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); }
Fixed.

> 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');
Conducted pgindent and pgperltidy.

> 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);
Fixed. I used strlcpy for new additional functions I made.
An exception is pgstat_read_db_statsfile().
In this function, we use memcpy() consistently in other places.

Please have a look at [1]

[1] - https://www.postgresql.org/message-id/TYCPR01MB8373FEB287F733C81C1E4D42ED989%40TYCPR01MB8373.jpnprd01.prod.outlook.com

Best Regards,
Takamichi Osumi