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