Re: Failed transaction statistics to measure the logical replication progress

On Wed, Mar 2, 2022 at 10:21 AM osumi(dot)takamichi(at)fujitsu(dot)com
<osumi(dot)takamichi(at)fujitsu(dot)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:

+ /* 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.

---
+/*
+ * 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.

---
+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.

Also, I think it's better to add "Xact" or something to the struct
name. For example, SubscriptionXactStats.

---
+
+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.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/