On Tue, Feb 22, 2022 at 9:22 PM [email protected]
<[email protected]> wrote:
>
> On Tuesday, February 22, 2022 2:45 PM Masahiko Sawada <[email protected]>
> wrote:
> > I've attached a patch that changes pg_stat_subscription_workers view.
> > It removes non-cumulative values such as error details such as error-XID and
> > the error message from the view, and consequently the view now has only
> > cumulative statistics counters: apply_error_count and sync_error_count.
> > Since
> > the new view name is under discussion I temporarily chose
> > pg_stat_subscription_activity.
> Hi, thank you for sharing the patch.
>
>
> Few minor comments for v1.
Thank you for the comments!
>
> (1) commit message's typo
>
> This commits changes the view so that it stores only statistics
> counters: apply_error_count and sync_error_count.
>
> "This commits" -> "This commit"
Will fix.
>
> (2) minor improvement suggestion for the commit message
>
> I suggest that we touch the commit id 8d74fc9
> that introduces the pg_stat_subscription_workers
> in the commit message, for better traceability. Below is an example.
>
> From:
> As the result of the discussion, we've concluded that the stats
> collector is not an appropriate place to store the error information of
> subscription workers.
>
> To:
> As the result of the discussion about the view introduced by 8d74fc9,...
Okay, will add the commit reference.
>
> (3) doc/src/sgml/logical-replication.sgml
>
> Kindly refer to commit id 85c61ba for the detail.
> You forgot "the" in the below sentence.
>
> @@ -346,8 +346,6 @@
> <para>
> A conflict will produce an error and will stop the replication; it must be
> resolved manually by the user. Details about the conflict can be found in
> - <link linkend="monitoring-pg-stat-subscription-workers">
> - <structname>pg_stat_subscription_workers</structname></link> and the
> subscriber's server log.
> </para>
>
> From:
> subscriber's server log.
> to:
> the subscriber's server log.
Will fix.
>
> (4) doc/src/sgml/monitoring.sgml
>
> <row>
> <entry role="catalog_table_entry"><para role="column_definition">
> - <structfield>last_error_time</structfield> <type>timestamp with time
> zone</type>
> + <structfield>sync_error_count</structfield> <type>uint8</type>
> </para>
> <para>
> - Last time at which this error occurred
> + Number of times the error occurred during the initial data copy
> </para></entry>
>
> I supposed it might be better to use "initial data sync"
> or "initial data synchronization", rather than "initial data copy".
"Initial data synchronization" sounds like the whole table
synchronization process including COPY and applying changes to catch
up. But sync_error_count is incremented only during COPY so I used
"initial data copy". What do you think?
>
> (5) src/test/subscription/t/026_worker_stats.pl
>
> +# Truncate test_tab1 so that table sync can continue.
> +$node_subscriber->safe_psql('postgres', "TRUNCATE test_tab1;");
>
> The second truncate is for apply, isn't it? Therefore, kindly change
>
> From:
> Truncate test_tab1 so that table sync can continue.
> To:
> Truncate test_tab1 so that apply can continue.
Right, will fix.
>
> (6) src/test/subscription/t/026_worker_stats.pl
>
> +# Insert more data to test_tab1 on the subscriber and then on the publisher,
> raising an
> +# error on the subscriber due to violation of the unique constraint on
> test_tab1.
> +$node_subscriber->safe_psql('postgres', "INSERT INTO test_tab1 VALUES (2)");
>
> Did we need this insert ?
> If you want to indicate the apply is working okay after the error of table
> sync is solved,
> waiting for the max value in the test_tab1 becoming 2 on the subscriber by
> polling query
> would work. But, I was not sure if this is essentially necessary for the
> testing purpose.
You're right, it's not necessary. Also, it seems better to change the
TAP test file name from 026_worker_stats.pl to 026_stats.pl. Will
incorporate these changes.
Regards,
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/
