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:
Thank you for your 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?
This is because if we have hundreds or thousands of tables for table sync,
we need to create many entries to cover them and store the entries for all tables.
> 2.
> + PGSTAT_MTYPE_SUBWORKERXACTEND,
> } StatMsgType;
>
> I don't think we comma with the last message type.
> 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.
Okay, I'll fix both points in the next version.
> 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?
Absolutely, this was a mistake when I took the decision to merge both stats
of table sync and apply worker.
> I think if you choose
> to show separate stats for table sync and apply worker then probably it will be
> used.
Yeah, I'll fix this. Of course, after I could confirm that the idea for merging the
two types of workers stats was acceptable for you and others.
Best Regards,
Takamichi Osumi