RE: Failed transaction statistics to measure the logical replication progress

On Monday, December 6, 2021 11:27 PM vignesh C <vignesh21(at)gmail(dot)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