Thread: Refactor calculations to use instr_time
Hi, 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, I refactored 'PendingWalStats' to use 'instr_time' instead of 'PgStat_Counter' while accumulating 'wal_write_time' and 'wal_sync_time'. Also, I refactored some calculations to use 'INSTR_TIME_ACCUM_DIFF' instead of using 'INSTR_TIME_SUBTRACT' and 'INSTR_TIME_ADD'. What do you think? Regards, Nazir Bilal Yavuz Microsoft
Attachment
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_timeMinor 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
Attachment
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
Attachment
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
Attachment
Hi, There was a warning while applying the patch, v5 is attached. Regards, Nazir Bilal Yavuz Microsoft
Attachment
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
Attachment
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