Re: Refactor calculations to use instr_time - Mailing list pgsql-hackers

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