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().
> 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?)
> 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.
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.
> Maybe there is the case where a backend generates no WAL records,
> but just writes them because it needs to do write-ahead-logging
> when flush the table data?
> Ugh! I was missing a very large blob.. Ok, we need additional check
> for non-pgWalUsage part. Sorry.
Regards,
--
Masahiro Ikeda
NTT DATA CORPORATION
