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

On 2021-02-24 16:14, Fujii Masao wrote:
> On 2021/02/15 11:59, Masahiro Ikeda wrote:
>> On 2021-02-10 00:51, David G. Johnston wrote:
>>> On Thu, Feb 4, 2021 at 4:45 PM Masahiro Ikeda
>>> <ikedamsh@oss.nttdata.com> wrote:
>>> 
>>>> I pgindented the patches.
>>> 
>>> ... <function>XLogWrite</function>, which is invoked during an
>>> <function>XLogFlush</function> request (see ...).  This is also
>>> incremented by the WAL receiver during replication.
>>> 
>>> ("which normally called" should be "which is normally called" or
>>> "which normally is called" if you want to keep true to the original)
>>> You missed the adding the space before an opening parenthesis here 
>>> and
>>> elsewhere (probably copy-paste)
>>> 
>>> is ether -> is either
>>> "This parameter is off by default as it will repeatedly query the
>>> operating system..."
>>> ", because" -> "as"
>> 
>> Thanks, I fixed them.
>> 
>>> wal_write_time and the sync items also need the note: "This is also
>>> incremented by the WAL receiver during replication."
>> 
>> I skipped changing it since I separated the stats for the WAL receiver
>> in pg_stat_wal_receiver.
>> 
>>> "The number of times it happened..." -> " (the tally of this event is
>>> reported in wal_buffers_full in....) This is undesirable because ..."
>> 
>> Thanks, I fixed it.
>> 
>>> I notice that the patch for WAL receiver doesn't require explicitly
>>> computing the sync statistics but does require computing the write
>>> statistics.  This is because of the presence of issue_xlog_fsync but
>>> absence of an equivalent pg_xlog_pwrite.  Additionally, I observe 
>>> that
>>> the XLogWrite code path calls pgstat_report_wait_*() while the WAL
>>> receiver path does not.  It seems technically straight-forward to
>>> refactor here to avoid the almost-duplicated logic in the two places,
>>> though I suspect there may be a trade-off for not adding another
>>> function call to the stack given the importance of WAL processing
>>> (though that seems marginalized compared to the cost of actually
>>> writing the WAL).  Or, as Fujii noted, go the other way and don't 
>>> have
>>> any shared code between the two but instead implement the WAL 
>>> receiver
>>> one to use pg_stat_wal_receiver instead.  In either case, this
>>> half-and-half implementation seems undesirable.
>> 
>> OK, as Fujii-san mentioned, I separated the WAL receiver stats.
>> (v10-0002-Makes-the-wal-receiver-report-WAL-statistics.patch)
> 
> Thanks for updating the patches!
> 
> 
>> I added the infrastructure code to communicate the WAL receiver stats 
>> messages between the WAL receiver and the stats collector, and
>> the stats for WAL receiver is counted in pg_stat_wal_receiver.
>> What do you think?
> 
> On second thought, this idea seems not good. Because those stats are
> collected between multiple walreceivers, but other values in
> pg_stat_wal_receiver is only related to the walreceiver process running
> at that moment. IOW, it seems strange that some values show dynamic
> stats and the others show collected stats, even though they are in
> the same view pg_stat_wal_receiver. Thought?

OK, I fixed it.
The stats collected in the WAL receiver is exposed in pg_stat_wal view 
in v11 patch.

> I notice that the patch for WAL receiver doesn't require explicitly 
> computing the sync statistics but does require computing the write 
> statistics.  This is because of the presence of issue_xlog_fsync but 
> absence of an equivalent pg_xlog_pwrite.  Additionally, I observe that 
> the XLogWrite code path calls pgstat_report_wait_*() while the WAL 
> receiver path does not.  It seems technically straight-forward to 
> refactor here to avoid the almost-duplicated logic in the two places, 
> though I suspect there may be a trade-off for not adding another 
> function call to the stack given the importance of WAL processing 
> (though that seems marginalized compared to the cost of actually 
> writing the WAL).  Or, as Fujii noted, go the other way and don't have 
> any shared code between the two but instead implement the WAL receiver 
> one to use pg_stat_wal_receiver instead.  In either case, this 
> half-and-half implementation seems undesirable.

I refactored the logic to write xlog file to unify collecting the write 
stats.
As David said, although pgstat_report_wait_start(WAIT_EVENT_WAL_WRITE) 
is not called in the WAL receiver's path,
I agreed that the cost to write the WAL is much bigger.

Regards,
-- 
Masahiro Ikeda
NTT DATA CORPORATION