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

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)

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?

Regards,
-- 
Masahiro Ikeda
NTT DATA CORPORATION