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

Masahiro Ikeda <ikedamsh@oss.nttdata.com>

From: ikedamsh <ikedamsh@oss.nttdata.com>
To: Fujii Masao <masao.fujii@oss.nttdata.com>
Cc: "David G. Johnston" <david.g.johnston@gmail.com>, PostgreSQL-development <pgsql-hackers@postgresql.org>, Masahiko Sawada <sawada.mshk@gmail.com>, Li Japin <japinli@hotmail.com>, kuroda.hayato@fujitsu.com
Date: 2021-03-22T11:25:45Z
Lists: pgsql-hackers

Attachments


On 2021/03/22 16:50, Fujii Masao wrote:
> 
> 
> On 2021/03/22 9:50, ikedamsh wrote:
>> Agreed. I separated the patches.
>>
>> If only the former is committed, my trivial concern is that there may be
>> a disadvantage, but no advantage for the standby server. It may lead to
>> performance degradation to the wal receiver by calling
>> INSTR_TIME_SET_CURRENT(), but the stats can't visible for users until the
>> latter patch is committed.
> 
> Your concern is valid, so let's polish and commit also the 0003 patch to v14.
> I'm still thinking that it's better to separate wal_xxx columns into
> walreceiver's and the others. But if we count even walreceiver activity on
> the existing columns, regarding 0003 patch, we need to update the document?
> For example, "Number of times WAL buffers were written out to disk via
> XLogWrite request." should be "Number of times WAL buffers were written
> out to disk via XLogWrite request and by WAL receiver process."? Maybe
> we need to append some descriptions about this into "WAL configuration"
> section?

Agreed. Users can know whether the stats is for walreceiver or not. The
pg_stat_wal view in standby server shows for the walreceiver, and in primary
server it shows for the others. So, I updated the document.
(v20-0003-Makes-the-wal-receiver-report-WAL-statistics.patch)

>> I followed the argument of pg_pwrite().
>> But, I think "char *" is better, so fixed it.
> 
> Thanks for updating the patch!
> 
> +extern int    XLogWriteFile(int fd, char *buf,
> +                          size_t nbyte, off_t offset,
> +                          TimeLineID timelineid, XLogSegNo segno,
> +                          bool write_all);
> 
> write_all seems not to be necessary. You added this flag for walreceiver,
> I guess. But even without the argument, walreceiver seems to work expectedly.
> So, what about the attached patch? I applied some cosmetic changes to the patch.

Thanks a lot. Yes, "write_all" is unnecessary.
Your patch is looks good to me.


Regards,
-- 
Masahiro Ikeda
NTT DATA CORPORATION

Commits

  1. Send statistics collected during shutdown checkpoint to the stats collector.

  2. Force to send remaining WAL stats to the stats collector at walwriter exit.

  3. Track total amounts of times spent writing and syncing WAL data to disk.

  4. Retry short writes when flushing WAL.