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

On Thu, Feb 4, 2021 at 4:45 PM Masahiro Ikeda <ikedamsh(at)oss(dot)nttdata(dot)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"

wal_write_time and the sync items also need the note: "This is also
incremented by the WAL receiver during replication."

"The number of times it happened..." -> " (the tally of this event is
reported in wal_buffers_full in....) This is undesirable because ..."

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.

David J.