RE: Failed transaction statistics to measure the logical replication progress

On Wednesday, November 10, 2021 7:13 PM vignesh C <vignesh21@gmail.com> wrote:
> 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;
> +
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