I update the patch since there were my misunderstanding points.
On 2021/03/26 16:20, Masahiro Ikeda wrote:
> Thanks for many your suggestions!
> I made the patch to handle the issues.
>
>> 1) What is the motivation to have both prevWalUsage and pgWalUsage,
>> instead of just accumulating the stats since the last submission?
>> There doesn't seem to be any comment explaining it? Computing
>> differences to previous values, and copying to prev*, isn't free. I
>> assume this is out of a fear that the stats could get reset before
>> they're used for instrument.c purposes - but who knows?
>
> I removed the unnecessary code copying pgWalUsage and just reset the
> pgWalUsage after reporting the stats in pgstat_report_wal().
I didn't change this.
>> 2) Why is there both pgstat_send_wal() and pgstat_report_wal()? With the
>> former being used by wal writer, the latter by most other processes?
>> There again don't seem to be comments explaining this.
>
> I added the comments why two functions are separated.
> (But is it better to merge them?)
I updated the comments for following reasons.
>> 3) Doing if (memcmp(&WalStats, &all_zeroes, sizeof(PgStat_MsgWal)) == 0)
>> just to figure out if there's been any changes isn't all that
>> cheap. This is regularly exercised in read-only workloads too. Seems
>> adding a boolean WalStats.have_pending = true or such would be
>> better.
>> 4) For plain backends pgstat_report_wal() is called by
>> pgstat_report_stat() - but it is not checked as part of the "Don't
>> expend a clock check if nothing to do" check at the top. It's
>> probably rare to have pending wal stats without also passing one of
>> the other conditions, but ...
>
> I added the logic to check if the stats counters are updated or not in
> pgstat_report_stat() using not only generated wal record but also write/sync
> counters, and it can skip to call reporting function.
I removed the checking code whether the wal stats counters were updated or not
in pgstat_report_stat() since I couldn't understand the valid case yet.
pgstat_report_stat() is called by backends when the transaction is finished.
This means that the condition seems always pass.
I thought to implement if the WAL stats counters were not updated, skip to
send all statistics including the counters for databases and so on. But I
think it's not good because it may take more time to be reflected the
generated stats by read-only transaction.
> Although I added the condition which the write/sync counters are updated or
> not, I haven't understood the following case yet...Sorry. I checked related
> code and tested to insert large object, but I couldn't. I'll investigate more
> deeply, but if you already know the function which leads the following case,
> please let me know.
I understood the above case (Fujii-san, thanks for your advice in person).
When to flush buffers, for example, to select buffer replacement victim,
it requires a WAL flush if the buffer is dirty. So, to check the WAL stats
counters are updated or not, I check the number of generated wal record and
the counter of syncing in pgstat_report_wal().
The reason why not to check the counter of writing is that if to write is
happened, to sync is happened too in the above case. I added the comments in
the patch.
Regards,
--
Masahiro Ikeda
NTT DATA CORPORATION
