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