On 2021/03/25 16:37, Kyotaro Horiguchi wrote:
At Wed, 24 Mar 2021 21:07:26 -0700, Andres Freund <and...@anarazel.de> wrote in
Hi,
On 2021-03-25 10:51:56 +0900, Masahiro Ikeda wrote:
On 2021/03/25 8:22, Andres Freund wrote:
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?
Is your point that it's better to call pgWalUsage = 0; after sending the
stats?
Yes. At least there should be a comment explaining why it's done the way
it is.
pgWalUsage was used without resetting and we (I) thought that that
behavior should be preserved. On second thought, as Andres suggested,
we can just reset pgWalUsage at sending since AFAICS no one takes
difference crossing pgstat_report_stat() calls.
Yes, I agree that we can do that since there seems no such code for now.
Also if we do that, we can check, for example "pgWalUsage.wal_records > 0"
as you suggested, to easily determine whether there is pending WAL stats or not.
Anyway I agree it's better to add comments about the design more.
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.
I understood that for backends, this may leads performance degradation and
this problem is not only for the WalStats but also SLRUStats.
I wondered the degradation is so big because pgstat_report_stat() checks if at
least PGSTAT_STAT_INTERVAL msec is passed since it last sent before check the
diff. If my understanding is correct, to get timestamp is more expensive.
Getting a timestamp is expensive, yes. But I think we need to check for
the no-pending-wal-stats *before* the clock check. See the below:
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'm not confidence my understanding of your comment is right.)
You mean that we need to expend a clock check in pgstat_report_wal()?
I think it's unnecessary because pgstat_report_stat() is already checked it.
No, I mean that right now we might can erroneously return early
pgstat_report_wal() for normal backends in some workloads:
void
pgstat_report_stat(bool disconnect)
...
/* Don't expend a clock check if nothing to do */
if ((pgStatTabList == NULL || pgStatTabList->tsa_used == 0) &&
pgStatXactCommit == 0 && pgStatXactRollback == 0 &&
!have_function_stats && !disconnect)
return;
might return if there only is pending WAL activity. This needs to check
whether there are pending WAL stats. Which in turn means that the check
should be fast.
Agreed that the condition is wrong. On the other hand, the counters
are incremented in XLogInsertRecord() and I think we don't want add
instructions there.
Basically yes. We should avoid that especially while WALInsertLock is being
hold.
But it's not so harmful to do that after the lock is released?
If any wal activity has been recorded, wal_records is always positive
thus we can check for wal activity just by "pgWalUsage.wal_records >
0, which should be fast enough.
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? If yes, "pgWalUsage.wal_records > 0" is not enough.
Probably other fields also need to be checked.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
