RE: Failed transaction statistics to measure the logical replication progress

On Monday, December 6, 2021 11:27 PM vignesh C <vignesh21@gmail.com> wrote:
> Thanks for the updated patch, few comments:
Thank you for your review !

> 1) We can keep the documentation similar to mention the count includes both
> table sync worker / main apply worker in case of commit_count/error_count
> and abort_count to keep it consistent.
> +       <structfield>commit_count</structfield> <type>bigint</type>
> +      </para>
> +      <para>
> +       Number of transactions successfully applied in this subscription.
> +       COMMIT and COMMIT PREPARED increments this counter.
> +      </para></entry>
> +     </row>
> +
> +     <row>
> +      <entry role="catalog_table_entry"><para role="column_definition">
> +       <structfield>error_count</structfield> <type>bigint</type>
> +      </para>
> +      <para>
> +       Number of transactions that failed to be applied by the table
> +       sync worker or main apply worker in this subscription.
> +       </para></entry>
> +     </row>
> +
> +     <row>
> +      <entry role="catalog_table_entry"><para role="column_definition">
> +       <structfield>abort_count</structfield> <type>bigint</type>
> +      </para>
> +      <para>
> +       Number of transactions aborted in this subscription.
> +       ROLLBACK PREPARED increments this counter.
> +      </para></entry>
> +     </row>
Yeah, you are right. Fixed.
Note that abort_count is not used by table sync worker.

 
> 2) Can this be changed:
> +       /*
> +        * If this is a new error reported by table sync worker,
> consolidate this
> +        * error count into the entry of apply worker.
> +        */
> +       if (OidIsValid(msg->m_subrelid))
> +       {
> +               /* Gain the apply worker stats */
> +               subwentry = pgstat_get_subworker_entry(dbentry,
> + msg->m_subid,
> +
>                     InvalidOid, true);
> +               subwentry->error_count++;
> +       }
> +       else
> +               subwentry->error_count++;       /* increment the apply
> worker's counter. */
> To:
> +       /*
> +        * If this is a new error reported by table sync worker,
> consolidate this
> +        * error count into the entry of apply worker.
> +        */
> +       if (OidIsValid(msg->m_subrelid))
> +               /* Gain the apply worker stats */
> +               subwentry = pgstat_get_subworker_entry(dbentry,
> + msg->m_subid,
> +
>                     InvalidOid, true);
> +
> +       subwentry->error_count++;       /* increment the apply
> worker's counter. */
Your suggestion looks better.
Also, I fixed some comments of this part
so that we don't need to add a separate comment at the bottom
for the increment of the apply worker.

 
> 3) Since both 026_worker_stats and 027_worker_xact_stats.pl are testing
> pg_stat_subscription_workers, can we move the tests to 026_worker_stats.pl.
> If possible the error_count validation can be combined with the existing tests.
> diff --git a/src/test/subscription/t/027_worker_xact_stats.pl
> b/src/test/subscription/t/027_worker_xact_stats.pl
> new file mode 100644
> index 0000000..31dbea1
> --- /dev/null
> +++ b/src/test/subscription/t/027_worker_xact_stats.pl
> @@ -0,0 +1,162 @@
> +
> +# Copyright (c) 2021, PostgreSQL Global Development Group
> +
> +# Tests for subscription worker statistics during apply.
> +use strict;
> +use warnings;
> +use PostgreSQL::Test::Cluster;
> +use PostgreSQL::Test::Utils;
> +use Test::More tests => 1;
> +
> +# Create publisher node
Right. I've integrated my tests with 026_worker_stats.pl.
I think error_count validations are combined as you suggested.
Another change I did is to introduce one function
to contribute to better readability of the stats tests.

Here, the 026_worker_stats.pl didn't look aligned by
pgperltidy. This is not a serious issue at all.
Yet, when I ran pgperltidy, the existing codes
that required adjustments came into my patch.
Therefore, I made a separate part for this.

Best Regards,
    Takamichi Osumi