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

On 2021/03/03 14:33, Masahiro Ikeda wrote:
> 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(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"
>>>
>>> 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.

Thanks for updating the patches! I'm now reading 001 patch.

+ /* Check whether the WAL file was synced to disk right now */
+ if (enableFsync &&
+ (sync_method == SYNC_METHOD_FSYNC ||
+ sync_method == SYNC_METHOD_FSYNC_WRITETHROUGH ||
+ sync_method == SYNC_METHOD_FDATASYNC))
+ {

Isn't it better to make issue_xlog_fsync() return immediately
if enableFsync is off, sync_method is open_sync or open_data_sync,
to simplify the code more?

+ /*
+ * Send WAL statistics only if WalWriterDelay has elapsed to minimize
+ * the overhead in WAL-writing.
+ */
+ if (rc & WL_TIMEOUT)
+ pgstat_send_wal();

On second thought, this change means that it always takes wal_writer_delay
before walwriter's WAL stats is sent after XLogBackgroundFlush() is called.
For example, if wal_writer_delay is set to several seconds, some values in
pg_stat_wal would be not up-to-date meaninglessly for those seconds.
So I'm thinking to withdraw my previous comment and it's ok to send
the stats every after XLogBackgroundFlush() is called. Thought?

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION