RE: Failed transaction statistics to measure the logical replication progress - Mailing list pgsql-hackers

From osumi.takamichi@fujitsu.com
Subject RE: Failed transaction statistics to measure the logical replication progress
Date
Msg-id TYCPR01MB837345FB72E3CE9C827AF42CED049@TYCPR01MB8373.jpnprd01.prod.outlook.com
Whole thread Raw
In response to Re: Failed transaction statistics to measure the logical replication progress  (Masahiko Sawada <sawada.mshk@gmail.com>)
Responses Re: Failed transaction statistics to measure the logical replication progress
List pgsql-hackers
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


Attachment

pgsql-hackers by date:

Previous
From: "David G. Johnston"
Date:
Subject: Re: Doc about how to set max_wal_senders when setting minimal wal_level
Next
From: "osumi.takamichi@fujitsu.com"
Date:
Subject: RE: Failed transaction statistics to measure the logical replication progress