Re: wal stats questions

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.

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

Greetings,

Andres Freund