On Wednesday, March 2, 2022 2:18 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> On Wed, Mar 2, 2022 at 10:21 AM osumi.takamichi@fujitsu.com
> <osumi.takamichi@fujitsu.com> wrote:
> > Also, I quickly checked other similar views(pg_stat_slru,
> > pg_stat_wal_receiver) commit logs, especially when they introduce columns.
> > But, I couldn't find column name validations.
> > So, I feel this is aligned.
> >
>
> I've looked at v26 patch and here are some random comments:
Hi, thank you for reviewing !
> + /* determine the subscription entry */
> + Oid m_subid;
> +
> + PgStat_Counter apply_commit_count;
> + PgStat_Counter apply_rollback_count;
>
> I think it's better to add the prefix "m_" to apply_commit/rollback_count for
> consistency.
Fixed.
> ---
> +/*
> + * Increment the counter of commit for subscription statistics.
> + */
> +static void
> +subscription_stats_incr_commit(void)
> +{
> + Assert(OidIsValid(subStats.subid));
> +
> + subStats.apply_commit_count++;
> +}
> +
>
> I think we don't need the Assert() here since it should not be a problem even if
> subStats.subid is InvalidOid at least in this function.
>
> If we remove it, we can remove both subscription_stats_incr_commit() and
> +subscription_stats_incr_rollback() as well.
Removed the Assert() from both functions.
> ---
> +void
> +pgstat_report_subscription_xact(bool force) {
> + static TimestampTz last_report = 0;
> + PgStat_MsgSubscriptionXact msg;
> +
> + /* Bailout early if nothing to do */
> + if (!OidIsValid(subStats.subid) ||
> + (subStats.apply_commit_count == 0 &&
> subStats.apply_rollback_count == 0))
> + return;
> +
>
> +LogicalRepSubscriptionStats subStats =
> +{
> + .subid = InvalidOid,
> + .apply_commit_count = 0,
> + .apply_rollback_count = 0,
> +};
>
> Do we need subStats.subid? I think we can pass MySubscription->oid (or
> MyLogicalRepWorker->subid) to pgstat_report_subscription_xact() along
> with the pointer of the statistics (subStats). That way, we don't need to expose
> subStats.
Removed the subStats.subid. Also, now I pass the oid to the
pgstat_report_subscription_xact with the pointer of the statistics.
> Also, I think it's better to add "Xact" or something to the struct name. For
> example, SubscriptionXactStats.
Renamed.
> ---
> +
> +typedef struct LogicalRepSubscriptionStats {
> + Oid subid;
> +
> + int64 apply_commit_count;
> + int64 apply_rollback_count;
> +} LogicalRepSubscriptionStats;
>
> We need a description for this struct.
>
> Probably it is better to declare it in logicalworker.h instead so that pgstat.c
> includes it instead of worker_internal.h? worker_internal.h is the header file
> shared by logical replication workers such as apply worker, tablesync worker,
> and launcher. So it might not be advisable to include it in pgstat.c.
Changed the definition place to logicalworker.h
and added some explanations for it.
Attached the updated v27.
Best Regards,
Takamichi Osumi
