On Monday, December 13, 2021 6:19 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> 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?
Now, the added stats show only the statistics of apply worker
as we agreed.
> 2.
> + PGSTAT_MTYPE_SUBWORKERXACTEND,
> } StatMsgType;
>
> I don't think we comma with the last message type.
Fixed.
> 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.
Removed.
> 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.
Fixed.
The new patch is shared in [1].
[1] -
https://www.postgresql.org/message-id/TYCPR01MB83734A7A0596AC7ADB0DCB51ED779%40TYCPR01MB8373.jpnprd01.prod.outlook.com
Best Regards,
Takamichi Osumi