Re: About to add WAL write/fsync statistics to pg_stat_wal view

On 2020-12-08 16:45, Li Japin wrote:
> Hi,
> 
>> On Dec 8, 2020, at 1:06 PM, Masahiro Ikeda <ikedamsh@oss.nttdata.com> 
>> wrote:
>> 
>> Hi,
>> 
>> I propose to add wal write/fsync statistics to pg_stat_wal view.
>> It's useful not only for developing/improving source code related to 
>> WAL
>> but also for users to detect workload changes, HW failure, and so on.
>> 
>> I introduce "track_wal_io_timing" parameter and provide the following 
>> information on pg_stat_wal view.
>> I separate the parameter from "track_io_timing" to 
>> "track_wal_io_timing"
>> because IIUC, WAL I/O activity may have a greater impact on query 
>> performance than database I/O activity.
>> 
>> ```
>> postgres=# SELECT wal_write, wal_write_time, wal_sync, wal_sync_time 
>> FROM pg_stat_wal;
>> -[ RECORD 1 ]--+----
>> wal_write      | 650  # Total number of times WAL data was written to 
>> the disk
>> 
>> wal_write_time | 43   # Total amount of time that has been spent in 
>> the portion of WAL data was written to disk
>>                      # if track-wal-io-timing is enabled, otherwise 
>> zero
>> 
>> wal_sync       | 78   # Total number of times WAL data was synced to 
>> the disk
>> 
>> wal_sync_time  | 104  # Total amount of time that has been spent in 
>> the portion of WAL data was synced to disk
>>                      # if track-wal-io-timing is enabled, otherwise 
>> zero
>> ```
>> 
>> What do you think?
>> Please let me know your comments.
>> 
>> Regards
>> --
>> Masahiro Ikeda
>> NTT DATA 
>> CORPORATION<0001_add_wal_io_activity_to_the_pg_stat_wal.patch>
> 
> There is a no previous prototype warning for ‘fsyncMethodCalled’, and
> it now only used in xlog.c,
> should we declare with static? And this function wants a boolean as a
> return, should we use
> true/false other than 0/1?
> 
> +/*
> + * Check if fsync mothod is called.
> + */
> +bool
> +fsyncMethodCalled()
> +{
> +       if (!enableFsync)
> +               return 0;
> +
> +       switch (sync_method)
> +       {
> +               case SYNC_METHOD_FSYNC:
> +               case SYNC_METHOD_FSYNC_WRITETHROUGH:
> +               case SYNC_METHOD_FDATASYNC:
> +                       return 1;
> +               default:
> +                       /* others don't have a specific fsync method */
> +                       return 0;
> +       }
> +}
> +

Thanks for your review.
I agree with your comments. I fixed them.

Regards
-- 
Masahiro Ikeda
NTT DATA CORPORATION