Re: Refactor calculations to use instr_time

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(at)gmail(dot)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