Refactor calculations to use instr_time

Hi,

On 2023-02-16 16:19:02 +0300, Nazir Bilal Yavuz wrote:

What do you think?

Here's a small review:

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

+ * 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;
+

I wonder if we should try to put pgWalUsage in here. But that's probably
better done as a separate patch.

Greetings,

Andres Freund

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

At Fri, 17 Feb 2023 13:53:36 +0300, Nazir Bilal Yavuz <byavuz81@gmail.com> 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

Hi,

Thanks for the review.

On Mon, 20 Feb 2023 at 06:01, Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:

At Fri, 17 Feb 2023 13:53:36 +0300, Nazir Bilal Yavuz <byavuz81@gmail.com> 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.

I agree. The patch is updated.

Regards,
Nazir Bilal Yavuz
Microsoft

At Tue, 21 Feb 2023 16:11:19 +0300, Nazir Bilal Yavuz <byavuz81@gmail.com> 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.

--
Kyotaro Horiguchi
NTT Open Source Software Center

Hi,

Thanks for the review.

On Wed, 22 Feb 2023 at 05:50, Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:

PgStat_PendingStats should be included in typedefs.list.

Done.

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

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?

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?

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

Hi,

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

Regards,
Nazir Bilal Yavuz
Microsoft

On Thu, Mar 09, 2023 at 04:02:44PM +0300, Nazir Bilal Yavuz wrote:

From dcd49e48a0784a95b8731df1c6ee7c3a612a8529 Mon Sep 17 00:00:00 2001
From: Nazir Bilal Yavuz <byavuz81@gmail.com>
Date: Thu, 9 Mar 2023 15:35:38 +0300
Subject: [PATCH v5] Refactor instr_time calculations

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/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
@@ -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);

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.

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

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

Hi,

Thanks for the review.

On Fri, 17 Mar 2023 at 02:02, Melanie Plageman
<melanieplageman@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

Hi,

I pushed this version! Thanks all, for the contribution and reviews.

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.

I invite you to write a patch for that for 17...

Greetings,

Andres Freund