Re: 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