Re: wal stats questions

At Wed, 24 Mar 2021 21:07:26 -0700, Andres Freund <andres(at)anarazel(dot)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.

> > > 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.

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.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center