Re: Refactor calculations to use instr_time

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_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
> */

Thanks for the review. I updated the patch.

Regards,
Nazir Bilal Yavuz
Microsoft