Hi,
Thanks for the review.
On Fri, 17 Mar 2023 at 02:02, Melanie Plageman
<melanieplage...@gmail.com> wrote:
> I think you want one less L here?
> WALLSTAT_ACC_INSTR_TIME_TYPE -> WALSTAT_ACC_INSTR_TIME_TYPE
Done.
> Also, I don't quite understand why TYPE is at the end of the name. I
> think it would still be clear without it.
Done.
> 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.
Since it is undefined together with WALSTAT_ACC, defining them
together makes sense to me.
> > + * 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;
> > +
>
> 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.
Yes, but like Andres said this could be better done as a separate patch.
v6 is attached.
Regards,
Nazir Bilal Yavuz
Microsoft
From 02f1b5426ad9394a673c28a6df53fc56ff6ffa89 Mon Sep 17 00:00:00 2001
From: Nazir Bilal Yavuz <byavuz81@gmail.com>
Date: Wed, 22 Mar 2023 15:25:44 +0300
Subject: [PATCH v6] 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..94f1a3b06e3 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 WALSTAT_ACC_INSTR_TIME(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);
+ WALSTAT_ACC_INSTR_TIME(wal_write_time);
+ WALSTAT_ACC_INSTR_TIME(wal_sync_time);
+#undef WALSTAT_ACC_INSTR_TIME
#undef WALSTAT_ACC
LWLockRelease(&stats_shmem->lock);
diff --git a/src/include/pgstat.h b/src/include/pgstat.h
index 1e418b682b5..e8e17423c47 100644
--- a/src/include/pgstat.h
+++ b/src/include/pgstat.h
@@ -433,6 +433,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
@@ -746,7 +761,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 097f42e1b34..879f748a7dc 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -2048,6 +2048,7 @@ PgStat_Kind
PgStat_KindInfo
PgStat_LocalState
PgStat_PendingDroppedStatsItem
+PgStat_PendingWalStats
PgStat_SLRUStats
PgStat_ShmemControl
PgStat_Snapshot
--
2.40.0
