Re: wal stats questions

On 2021/03/25 16:37, Kyotaro Horiguchi wrote:
> 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.

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