MIME-Version: 1.0
References: <20220215182641.2cejma6tcgeoqyw2@alap3.anarazel.de>
 <CAA4eK1Jwn5hhgHJWZ9n+wYyQEk+brfR1zkuqLjh=waji8RYWAw@mail.gmail.com>
 <CAD21AoAqEt=TbUDY96Uhf9pNuh-NDR_3-CByd+xR=tFEq==1NA@mail.gmail.com>
 <CAD21AoA-MVZcMA1iYwbtvTCOHx1SwG8amQUTT8j2ssh2zuc6OQ@mail.gmail.com>
 <20220218203255.d3evl6qi2n6gxy6b@alap3.anarazel.de> <CAD21AoDTO3bJvrc39Tv3VCeyUzZpimXnJuRhHQhKyjOWorHeOA@mail.gmail.com>
 <20220219160203.y5w2ktc2utthn7pe@alap3.anarazel.de> <CAD21AoA_r+Wc-zJZdy4Lii3oOvvdGcLwrY=2Y4y63HueYSd6Eg@mail.gmail.com>
 <20220221053453.lo6fzmji23ennvgj@alap3.anarazel.de> <CAA4eK1+j00Pc+p=AfJLXh-a+OJ-a0UD4adMr2tRkQvKG_LggHA@mail.gmail.com>
 <20220221074806.nyhdberpuwbcbudp@alap3.anarazel.de> <CAA4eK1J3dhfMRNQ6Q585nFr1vmXaTXVW9qv4hmUxLwDwJp-9RQ@mail.gmail.com>
 <CAD21AoAtrgBEUU08VvfzRnGS+=9rS=MEq1cTLaZ1OT51xX34dQ@mail.gmail.com> <TYCPR01MB8373D12532FEECDAFDD5BA2BED3B9@TYCPR01MB8373.jpnprd01.prod.outlook.com>
In-Reply-To: <TYCPR01MB8373D12532FEECDAFDD5BA2BED3B9@TYCPR01MB8373.jpnprd01.prod.outlook.com>
From: Masahiko Sawada <sawada.mshk@gmail.com>
Date: Tue, 22 Feb 2022 23:46:31 +0900
Message-ID: <CAD21AoBRt=cyKsZP83rcMkHnT498gHH0TEP34fZBrGCxT-Ahwg@mail.gmail.com>
Subject: Re: Design of pg_stat_subscription_workers vs pgstats
To: "osumi.takamichi@fujitsu.com" <osumi.takamichi@fujitsu.com>
Cc: Amit Kapila <amit.kapila16@gmail.com>, Andres Freund <andres@anarazel.de>, 
	"David G. Johnston" <david.g.johnston@gmail.com>, pgsql-hackers <pgsql-hackers@postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Archived-At: <https://www.postgresql.org/message-id/CAD21AoBRt%3DcyKsZP83rcMkHnT498gHH0TEP34fZBrGCxT-Ahwg%40mail.gmail.com>
Precedence: bulk

On Tue, Feb 22, 2022 at 9:22 PM osumi.takamichi@fujitsu.com
<osumi.takamichi@fujitsu.com> wrote:
>
> On Tuesday, February 22, 2022 2:45 PM Masahiko Sawada <sawada.mshk@gmail.com> 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/