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

On 2021-03-03 16:30, Fujii Masao wrote:
> 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?

Thanks for the comments.
I added the above code in v12 patch.

>
> + /*
> + * 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?

Thanks, I didn't notice that.

Although PGSTAT_STAT_INTERVAL is 500msec, wal_writer_delay's
default value is 200msec and it may be set shorter time.

Why don't to make another way to check the timestamp?

+ /*
+ * Don't send a message unless it's been at least
PGSTAT_STAT_INTERVAL
+ * msec since we last sent one
+ */
+ now = GetCurrentTimestamp();
+ if (TimestampDifferenceExceeds(last_report, now,
PGSTAT_STAT_INTERVAL))
+ {
+ pgstat_send_wal();
+ last_report = now;
+ }
+

Although I worried that it's better to add the check code in
pgstat_send_wal(),
I didn't do so because to avoid to double check PGSTAT_STAT_INTERVAL.
pgstat_send_wal() is invoked pg_report_stat() and it already checks the
PGSTAT_STAT_INTERVAL.

Regards,
--
Masahiro Ikeda
NTT DATA CORPORATION