Subject: Re: Refactor calculations to use instr_time

From: Kyotaro Horiguchi <[email-masked]>
Date: Mon, 20 Feb 2023 12:01:14 +0900 (JST)

At Fri, 17 Feb 2023 13:53:36 +0300, Nazir Bilal Yavuz <[email-masked]> wrote in 
> Thanks for the review. I updated the patch.

 	WalUsageAccumDiff(&diff, &pgWalUsage, &prevWalUsage);
-	PendingWalStats.wal_records = diff.wal_records;
-	PendingWalStats.wal_fpi = diff.wal_fpi;
-	PendingWalStats.wal_bytes = diff.wal_bytes;
...
+	WALSTAT_ACC(wal_records, diff);
+	WALSTAT_ACC(wal_fpi, diff);
+	WALSTAT_ACC(wal_bytes, diff);
+	WALSTAT_ACC(wal_buffers_full, PendingWalStats);

The lifetime of the variable "diff" seems to be longer now. Wouldn't
it be clearer if we renamed it to something more meaningful, like
wal_usage_diff, WalUsageDiff or PendingWalUsage?  Along those same
lines, it occurs to me that the new struct should be named
PgStat_PendingWalStats, instead of ..Usage. That change makes the name
of the type and the variable consistent.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

---

From: Nazir Bilal Yavuz <[email-masked]>
Date: Tue, 21 Feb 2023 16:11:19 +0300

Hi,

Thanks for the review.

> of the type and the variable consistent.

I agree. The patch is updated.

Regards,
Nazir Bilal Yavuz
Microsoft

---

From: Kyotaro Horiguchi <[email-masked]>
Date: Wed, 22 Feb 2023 11:50:19 +0900 (JST)

At Tue, 21 Feb 2023 16:11:19 +0300, Nazir Bilal Yavuz <[email-masked]> wrote in 
> I agree. The patch is updated.

Thanks, that part looks good to me. I'd like to provide some
additional comments.

PgStat_PendingStats should be included in typedefs.list.

+ * Created for accumulating wal_write_time and wal_sync_time as a instr_time
+ * but instr_time can't be used as a type where it ends up on-disk
+ * because its units may change. PgStat_WalStats type is used for
+ * in-memory/on-disk data. So, PgStat_PendingWalStats is created for
+ * accumulating intervals as a instr_time.
+ */
+typedef struct PgStat_PendingWalStats

IMHO, this comment looks somewhat off. Maybe we could try something
like the following instead?

> This struct stores wal-related durations as instr_time, which makes it
> easier to accumulate them without requiring type conversions. Then,
> during stats flush, they will be moved into shared stats with type
> conversions.

The aim of this patch is to keep using instr_time for accumulation.
So it seems like we could do the same refactoring for
pgStatBlockReadTime, pgStatBlockWriteTime, pgStatActiveTime and
pgStatTransactionIdleTime. What do you think - should we include this
additional refactoring in the same patch or make a separate one for
it?

regards.

---

From: Nazir Bilal Yavuz <[email-masked]>
Date: Wed, 22 Feb 2023 13:13:03 +0300

Hi,

Thanks for the review.

> PgStat_PendingStats should be included in typedefs.list.

Done.

> > during stats flush, they will be moved into shared stats with type

Done. And I think we should write why we didn't change
PgStat_WalStats's variable types and instead created a new struct.
Maybe we can explain it in the commit description?

> additional refactoring in the same patch or make a separate one for

I tried a bit but it seems the required changes for additional
refactoring aren't small. So, I think we can create a separate patch
for these changes.

Regards,
Nazir Bilal Yavuz
Microsoft

---

From: Nazir Bilal Yavuz <[email-masked]>
Date: Thu, 9 Mar 2023 16:02:44 +0300

Hi,

There was a warning while applying the patch, v5 is attached.

Regards,
Nazir Bilal Yavuz
Microsoft
From dcd49e48a0784a95b8731df1c6ee7c3a612a8529 Mon Sep 17 00:00:00 2001
From: Nazir Bilal Yavuz <[email-masked]>
Date: Thu, 9 Mar 2023 15:35:38 +0300
Subject: [PATCH v5] Refactor instr_time calculations

In instr_time.h it is stated that:

* When summing multiple measurements, it's recommended to leave the
* running sum in instr_time form (ie, use INSTR_TIME_ADD or
* INSTR_TIME_ACCUM_DIFF) and convert to a result format only at the end.

So, this patch aims to refactor 'PendingWalStats' to use 'instr_time' instead
of 'PgStat_Counter' while accumulating 'wal_write_time' and 'wal_sync_time'.

Also, some calculations are refactored to use 'INSTR_TIME_ACCUM_DIFF' instead
of using 'INSTR_TIME_SUBTRACT' and 'INSTR_TIME_ADD'.
---
 src/backend/access/transam/xlog.c       |  6 ++---
 src/backend/storage/file/buffile.c      |  6 ++---
 src/backend/utils/activity/pgstat_wal.c | 31 +++++++++++++------------
 src/include/pgstat.h                    | 17 +++++++++++++-
 src/tools/pgindent/typedefs.list        |  1 +
 5 files changed, 37 insertions(+), 24 deletions(-)

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 543d4d897ae..46821ad6056 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -2206,8 +2206,7 @@ XLogWrite(XLogwrtRqst WriteRqst, TimeLineID tli, bool flexible)
 					instr_time	duration;
 
 					INSTR_TIME_SET_CURRENT(duration);
-					INSTR_TIME_SUBTRACT(duration, start);
-					PendingWalStats.wal_write_time += INSTR_TIME_GET_MICROSEC(duration);
+					INSTR_TIME_ACCUM_DIFF(PendingWalStats.wal_write_time, duration, start);
 				}
 
 				PendingWalStats.wal_write++;
@@ -8204,8 +8203,7 @@ issue_xlog_fsync(int fd, XLogSegNo segno, TimeLineID tli)
 		instr_time	duration;
 
 		INSTR_TIME_SET_CURRENT(duration);
-		INSTR_TIME_SUBTRACT(duration, start);
-		PendingWalStats.wal_sync_time += INSTR_TIME_GET_MICROSEC(duration);
+		INSTR_TIME_ACCUM_DIFF(PendingWalStats.wal_sync_time, duration, start);
 	}
 
 	PendingWalStats.wal_sync++;
diff --git a/src/backend/storage/file/buffile.c b/src/backend/storage/file/buffile.c
index 0a51624df3b..e55f86b675e 100644
--- a/src/backend/storage/file/buffile.c
+++ b/src/backend/storage/file/buffile.c
@@ -469,8 +469,7 @@ BufFileLoadBuffer(BufFile *file)
 	if (track_io_timing)
 	{
 		INSTR_TIME_SET_CURRENT(io_time);
-		INSTR_TIME_SUBTRACT(io_time, io_start);
-		INSTR_TIME_ADD(pgBufferUsage.temp_blk_read_time, io_time);
+		INSTR_TIME_ACCUM_DIFF(pgBufferUsage.temp_blk_read_time, io_time, io_start);
 	}
 
 	/* we choose not to advance curOffset here */
@@ -544,8 +543,7 @@ BufFileDumpBuffer(BufFile *file)
 		if (track_io_timing)
 		{
 			INSTR_TIME_SET_CURRENT(io_time);
-			INSTR_TIME_SUBTRACT(io_time, io_start);
-			INSTR_TIME_ADD(pgBufferUsage.temp_blk_write_time, io_time);
+			INSTR_TIME_ACCUM_DIFF(pgBufferUsage.temp_blk_write_time, io_time, io_start);
 		}
 
 		file->curOffset += bytestowrite;
diff --git a/src/backend/utils/activity/pgstat_wal.c b/src/backend/utils/activity/pgstat_wal.c
index e8598b2f4e0..58daae3fbd6 100644
--- a/src/backend/utils/activity/pgstat_wal.c
+++ b/src/backend/utils/activity/pgstat_wal.c
@@ -21,7 +21,7 @@
 #include "executor/instrument.h"

-PgStat_WalStats PendingWalStats = {0};
+PgStat_PendingWalStats PendingWalStats = {0};
 
 /*
  * WAL usage counters saved from pgWALUsage at the previous call to
@@ -70,7 +70,7 @@ bool
 pgstat_flush_wal(bool nowait)
 {
 	PgStatShared_Wal *stats_shmem = &pgStatLocal.shmem->wal;
-	WalUsage	diff = {0};
+	WalUsage	wal_usage_diff = {0};
 
 	Assert(IsUnderPostmaster || !IsPostmasterEnvironment);
 	Assert(pgStatLocal.shmem != NULL &&
@@ -88,25 +88,26 @@ pgstat_flush_wal(bool nowait)
 	 * Calculate how much WAL usage counters were increased by subtracting the
 	 * previous counters from the current ones.
 	 */
-	WalUsageAccumDiff(&diff, &pgWalUsage, &prevWalUsage);
-	PendingWalStats.wal_records = diff.wal_records;
-	PendingWalStats.wal_fpi = diff.wal_fpi;
-	PendingWalStats.wal_bytes = diff.wal_bytes;
+	WalUsageAccumDiff(&wal_usage_diff, &pgWalUsage, &prevWalUsage);
 
 	if (!nowait)
 		LWLockAcquire(&stats_shmem->lock, LW_EXCLUSIVE);
 	else if (!LWLockConditionalAcquire(&stats_shmem->lock, LW_EXCLUSIVE))
 		return true;
 
-#define WALSTAT_ACC(fld) stats_shmem->stats.fld += PendingWalStats.fld
-	WALSTAT_ACC(wal_records);
-	WALSTAT_ACC(wal_fpi);
-	WALSTAT_ACC(wal_bytes);
-	WALSTAT_ACC(wal_buffers_full);
-	WALSTAT_ACC(wal_write);
-	WALSTAT_ACC(wal_sync);
-	WALSTAT_ACC(wal_write_time);
-	WALSTAT_ACC(wal_sync_time);
+#define WALSTAT_ACC(fld, var_to_add) \
+	(stats_shmem->stats.fld += var_to_add.fld)
+#define WALLSTAT_ACC_INSTR_TIME_TYPE(fld) \
+	(stats_shmem->stats.fld += INSTR_TIME_GET_MICROSEC(PendingWalStats.fld))
+	WALSTAT_ACC(wal_records, wal_usage_diff);
+	WALSTAT_ACC(wal_fpi, wal_usage_diff);
+	WALSTAT_ACC(wal_bytes, wal_usage_diff);
+	WALSTAT_ACC(wal_buffers_full, PendingWalStats);
+	WALSTAT_ACC(wal_write, PendingWalStats);
+	WALSTAT_ACC(wal_sync, PendingWalStats);
+	WALLSTAT_ACC_INSTR_TIME_TYPE(wal_write_time);
+	WALLSTAT_ACC_INSTR_TIME_TYPE(wal_sync_time);
+#undef WALLSTAT_ACC_INSTR_TIME_TYPE
 #undef WALSTAT_ACC
 
 	LWLockRelease(&stats_shmem->lock);
diff --git a/src/include/pgstat.h b/src/include/pgstat.h
index f43fac09ede..5bbc55bb341 100644
--- a/src/include/pgstat.h
+++ b/src/include/pgstat.h
@@ -442,6 +442,21 @@ typedef struct PgStat_WalStats
 	TimestampTz stat_reset_timestamp;
 } PgStat_WalStats;
 
+/*
+ * This struct stores wal-related durations as instr_time, which makes it
+ * easier to accumulate them without requiring type conversions. Then,
+ * during stats flush, they will be moved into shared stats with type
+ * conversions.
+ */
+typedef struct PgStat_PendingWalStats
+{
+	PgStat_Counter wal_buffers_full;
+	PgStat_Counter wal_write;
+	PgStat_Counter wal_sync;
+	instr_time wal_write_time;
+	instr_time wal_sync_time;
+} PgStat_PendingWalStats;
+
 
 /*
  * Functions in pgstat.c
@@ -755,7 +770,7 @@ extern PGDLLIMPORT SessionEndType pgStatSessionEndCause;
  */
 
 /* updated directly by backends and background processes */
-extern PGDLLIMPORT PgStat_WalStats PendingWalStats;
+extern PGDLLIMPORT PgStat_PendingWalStats PendingWalStats;

 #endif							/* PGSTAT_H */
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 86a9303bf56..68eb52886fa 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -2049,6 +2049,7 @@ PgStat_Kind
 PgStat_KindInfo
 PgStat_LocalState
 PgStat_PendingDroppedStatsItem
+PgStat_PendingWalStats
 PgStat_SLRUStats
 PgStat_ShmemControl
 PgStat_Snapshot
-- 
2.39.2

---

From: Melanie Plageman <[email-masked]>
Date: Thu, 16 Mar 2023 19:02:43 -0400

> +	WALLSTAT_ACC_INSTR_TIME_TYPE(wal_write_time);

I think you want one less L here?
WALLSTAT_ACC_INSTR_TIME_TYPE -> WALSTAT_ACC_INSTR_TIME_TYPE

Also, I don't quite understand why TYPE is at the end of the name. I
think it would still be clear without it.

I might find it clearer if the WALSTAT_ACC_INSTR_TIME_TYPE macro was
defined before using it for those fields instead of defining it right
after defining WALSTAT_ACC.

> +} PgStat_PendingWalStats;

So, I am not a fan of having this second struct (PgStat_PendingWalStats)
which only has a subset of the members of PgStat_WalStats. It is pretty
confusing.

It is okay to have two structs -- one that is basically "in-memory" and
one that is a format that can be on disk, but these two structs with
different members are confusing and don't convey why we have the two
structs.

I would either put WalUsage into PgStat_PendingWalStats (so that it has
all the same members as PgStat_WalStats), or figure out a way to
maintain WalUsage separately from PgStat_WalStats or something else.
Worst case, add more comments to the struct definitions to explain why
they have the members they have and how WalUsage relates to them.

- Melanie

---

From: Nazir Bilal Yavuz <[email-masked]>
Date: Thu, 23 Mar 2023 17:38:14 +0300

Hi,

Thanks for the review.

Done.

Done.

> after defining WALSTAT_ACC.

Since it is undefined together with WALSTAT_ACC, defining them
together makes sense to me.

> they have the members they have and how WalUsage relates to them.

Yes, but like Andres said this could be better done as a separate patch.

v6 is attached.

Regards,
Nazir Bilal Yavuz
Microsoft