MIME-Version: 1.0
References: 
 <TYCPR01MB8373DC922E549FF3F031E9DAED8E9@TYCPR01MB8373.jpnprd01.prod.outlook.com>
 <CAJcOf-cMU_1VJ1c+9fwEBgimkj+=hHcCjj_SUfbHvKTuN=1tuw@mail.gmail.com>
 <TYCPR01MB8373B1614522B90BA96883E0ED929@TYCPR01MB8373.jpnprd01.prod.outlook.com>
 <CALDaNm3-QTZnd4rxbrUfKMM4u_vgb23LO+KW3O9LX7X6b8mr8A@mail.gmail.com>
 <CALDaNm130T4jr2RvpgY19SCY87UQ75wVbjrhjGj43bHV=9-LpQ@mail.gmail.com>
 <TYCPR01MB8373FEB287F733C81C1E4D42ED989@TYCPR01MB8373.jpnprd01.prod.outlook.com>
 <TYCPR01MB8373D7AE0B6C62E5CDB03C6CED989@TYCPR01MB8373.jpnprd01.prod.outlook.com>
 <TYCPR01MB8373CFDCD60CCDB1C35CFA45ED999@TYCPR01MB8373.jpnprd01.prod.outlook.com>
 <CAD21AoBAREVDtAM+JGR3DcnwnOi1Nw7E_5m68ZwFUGkOkfDn1Q@mail.gmail.com>
 <TYCPR01MB8373271BE5D4619302C05B00ED9A9@TYCPR01MB8373.jpnprd01.prod.outlook.com>
 <CAA4eK1JCbhxuO8fEaAaw8e30=5kEDaFRXpV2+opAdMoEOH5LEg@mail.gmail.com>
 <TYWPR01MB8362FCFAAD8D90424BEA2300ED9A9@TYWPR01MB8362.jpnprd01.prod.outlook.com>
 <CAA4eK1KR41bRUuPeNBSGv2+q7ROKukS3myeAUqrZMD8MEwR0DQ@mail.gmail.com>
 <CAD21AoC_p=gQKhhGwhDLzEU9rGwCWSzTk+a89XGG1JQKn-PiKw@mail.gmail.com>
 <TYCPR01MB8373380ED159E8BD52F2D9A5ED689@TYCPR01MB8373.jpnprd01.prod.outlook.com>
 <CALDaNm32HHjdwmuoF+Nw5CU70r819Kq+Tmat3bzkkvSv_2u=gA@mail.gmail.com>
 <TYWPR01MB8362480F9148248F3D1B1213ED6B9@TYWPR01MB8362.jpnprd01.prod.outlook.com>
 <CALDaNm1T3qnUnCiTytACUFiw=vktBPi9dvy8iNXCNAyPALOP_g@mail.gmail.com>
 <TYCPR01MB8373C2D4D105B83C378511CAED6E9@TYCPR01MB8373.jpnprd01.prod.outlook.com>
In-Reply-To: 
 <TYCPR01MB8373C2D4D105B83C378511CAED6E9@TYCPR01MB8373.jpnprd01.prod.outlook.com>
From: Amit Kapila <amit.kapila16@gmail.com>
Date: Mon, 13 Dec 2021 14:49:08 +0530
Message-ID: 
 <CAA4eK1Khz1C+Y6PWJmFaK+OTigktw_+zTfGk4viXvnNobpymNw@mail.gmail.com>
Subject: Re: Failed transaction statistics to measure the logical replication
 progress
To: "osumi.takamichi@fujitsu.com" <osumi.takamichi@fujitsu.com>
Cc: vignesh C <vignesh21@gmail.com>, Masahiko Sawada <sawada.mshk@gmail.com>,
	Greg Nancarrow <gregn4422@gmail.com>,
 "houzj.fnst@fujitsu.com" <houzj.fnst@fujitsu.com>,
	"pgsql-hackers@lists.postgresql.org" <pgsql-hackers@lists.postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Archived-At: 
 <https://www.postgresql.org/message-id/CAA4eK1Khz1C%2BY6PWJmFaK%2BOTigktw_%2BzTfGk4viXvnNobpymNw%40mail.gmail.com>
Precedence: bulk

On Tue, Dec 7, 2021 at 3:12 PM osumi.takamichi@fujitsu.com
<osumi.takamichi@fujitsu.com> wrote:
>

Few questions and comments:
========================
1.
The <structname>pg_stat_subscription_workers</structname> view will contain
    one row per subscription worker on which errors have occurred, for workers
    applying logical replication changes and workers handling the initial data
-   copy of the subscribed tables.  The statistics entry is removed when the
-   corresponding subscription is dropped.
+   copy of the subscribed tables. Also, the row corresponding to the apply
+   worker shows all transaction statistics of both types of workers on the
+   subscription. The statistics entry is removed when the corresponding
+   subscription is dropped.

Why did you choose to show stats for both types of workers in one row?

2.
+ PGSTAT_MTYPE_SUBWORKERXACTEND,
 } StatMsgType;

I don't think we comma with the last message type.

3.
+ Oid m_subrelid;
+
+ /* necessary to determine column to increment */
+ LogicalRepMsgType m_command;
+
+} PgStat_MsgSubWorkerXactEnd;

Is m_subrelid used in this patch? If not, why did you keep it? I think
if you choose to show separate stats for table sync and apply worker
then probably it will be used.

4.
  /*
+ * Cumulative transaction statistics of subscription worker
+ */
+ PgStat_Counter commit_count;
+ PgStat_Counter error_count;
+ PgStat_Counter abort_count;
+

I think it is better to keep the order of columns as commit_count,
abort_count, error_count in the entire patch.

-- 
With Regards,
Amit Kapila.