Subject: Re: Refactor calculations to use instr_time

From: Nazir Bilal Yavuz <[email-masked]>
Date: Fri, 17 Feb 2023 13:53:36 +0300

Hi,

On 2/16/23 19:13, Andres Freund wrote:
>> +#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, diff);
>> +	WALSTAT_ACC(wal_fpi, diff);
>> +	WALSTAT_ACC(wal_bytes, 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 WALSTAT_ACC
>> -
>>   	LWLockRelease(&stats_shmem->lock);
> WALSTAT_ACC is undefined, but WALLSTAT_ACC_INSTR_TIME_TYPE isn't.
>
> I'd not remove the newline before LWLockRelease().
>
>
>>   	/*
>> diff --git a/src/include/pgstat.h b/src/include/pgstat.h
>> index db9675884f3..295c5eabf38 100644
>> --- a/src/include/pgstat.h
>> +++ b/src/include/pgstat.h
>> @@ -445,6 +445,21 @@ typedef struct PgStat_WalStats
>>   	TimestampTz stat_reset_timestamp;
>>   } PgStat_WalStats;
>>   
>> +/* Created for accumulating wal_write_time and wal_sync_time as a
>> instr_time
> Minor code-formatting point: In postgres we don't put code in the same line as
> a multi-line comment starting with the /*. So either
>
> /* single line comment */
> or
> /*
>   * multi line
>   * comment
>   */

Thanks for the review. I updated the patch.

Regards,
Nazir Bilal Yavuz
Microsoft

From e3723aca8b79b07190834b0cfd1440dbbf706862 Mon Sep 17 00:00:00 2001
From: Nazir Bilal Yavuz <[email-masked]>
Date: Thu, 2 Feb 2023 15:06:48 +0300
Subject: [PATCH v2] Refactor instr_time calculations

---
 src/backend/access/transam/xlog.c       |  6 ++----
 src/backend/storage/file/buffile.c      |  6 ++----
 src/backend/utils/activity/pgstat_wal.c | 27 +++++++++++++------------
 src/include/pgstat.h                    | 18 ++++++++++++++++-
 4 files changed, 35 insertions(+), 22 deletions(-)

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index f9f0f6db8d1..3c35dc1ca23 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++;
@@ -8201,8 +8200,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..ab2869a0e24 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_PendingWalUsage PendingWalStats = {0};
 
 /*
  * WAL usage counters saved from pgWALUsage at the previous call to
@@ -89,24 +89,25 @@ pgstat_flush_wal(bool nowait)
 	 * 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;
 
 	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, diff);
+	WALSTAT_ACC(wal_fpi, diff);
+	WALSTAT_ACC(wal_bytes, 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 db9675884f3..cc103df7d2e 100644
--- a/src/include/pgstat.h
+++ b/src/include/pgstat.h
@@ -445,6 +445,22 @@ typedef struct PgStat_WalStats
 	TimestampTz stat_reset_timestamp;
 } PgStat_WalStats;
 
+/*
+ * 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_PendingWalUsage is created for
+ * accumulating intervals as a instr_time.
+ */
+typedef struct PgStat_PendingWalUsage
+{
+	PgStat_Counter wal_buffers_full;
+	PgStat_Counter wal_write;
+	PgStat_Counter wal_sync;
+	instr_time wal_write_time;
+	instr_time wal_sync_time;
+} PgStat_PendingWalUsage;
+
 
 /*
  * Functions in pgstat.c
@@ -758,7 +774,7 @@ extern PGDLLIMPORT SessionEndType pgStatSessionEndCause;
  */
 
 /* updated directly by backends and background processes */
-extern PGDLLIMPORT PgStat_WalStats PendingWalStats;
+extern PGDLLIMPORT PgStat_PendingWalUsage PendingWalStats;

 #endif							/* PGSTAT_H */
-- 
2.39.1