Re: wal stats questions

On 2021/05/12 19:19, Fujii Masao wrote:
>
>
> On 2021/05/11 18:46, Masahiro Ikeda wrote:
>>
>>
>> On 2021/05/11 16:44, Fujii Masao wrote:
>>>
>>>
>>> On 2021/04/28 9:10, Masahiro Ikeda wrote:
>>>>
>>>>
>>>> On 2021/04/27 21:56, Fujii Masao wrote:
>>>>>
>>>>>
>>>>> On 2021/04/26 10:11, Masahiro Ikeda wrote:
>>>>>>
>>>>>> First patch has only the changes for pg_stat_wal view.
>>>>>> ("v6-0001-performance-improvements-of-reporting-wal-stats-without-introducing-a-new-variable.patch")
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>> +        pgWalUsage.wal_records == prevWalUsage.wal_records &&
>>>>> +        walStats.wal_write == 0 && walStats.wal_sync == 0 &&
>>>>>> WalStats.m_wal_write should be checked here instead of walStats.wal_write?
>>>>
>>>> Thanks! Yes, I'll fix it.
>>>
>>> Thanks!
>>
>> Thanks for your comments!
>> I fixed them.
>
> Thanks for updating the patch!
>
>      if ((pgStatTabList == NULL || pgStatTabList->tsa_used == 0) &&
>          pgStatXactCommit == 0 && pgStatXactRollback == 0 &&
> +        pgWalUsage.wal_records == prevWalUsage.wal_records &&
> +        WalStats.m_wal_write == 0 && WalStats.m_wal_sync == 0 &&
>
> I'm just wondering if the above WAL activity counters need to be checked.
> Maybe it's not necessary because "pgStatXactCommit == 0 && pgStatXactRollback
> == 0"
> is checked? IOW, is there really the case where WAL activity counters are updated
> even when both pgStatXactCommit and pgStatXactRollback are zero?

Thanks for checking.

I haven't found the case yet. But, I added the condition because there is a
discussion that it's safer.

(It seems the mail thread chain is broken, Sorry...)
I copy the discussion at the time below.

https://www.postgresql.org/message-id/20210330.172843.267174731834281075.horikyota.ntt%40gmail.com
>>>> 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.
>>
>> I removed the checking code whether the wal stats counters were updated or not
>> in pgstat_report_stat() since I couldn't understand the valid case yet.
>> pgstat_report_stat() is called by backends when the transaction is finished.
>> This means that the condition seems always pass.
>
> Doesn't the same holds for all other counters? If you are saying that
> "wal counters should be zero if all other stats counters are zero", we
> need an assertion to check that and a comment to explain that.
>
> I personally find it safer to add the WAL-stats condition to the
> fast-return check, rather than addin such assertion.

> +    if (pgWalUsage.wal_records != prevWalUsage.wal_records)
> +    {
> +        WalUsage    walusage;
> +
> +        /*
> +         * Calculate how much WAL usage counters were increased by substracting
> +         * the previous counters from the current ones. Fill the results in
> +         * WAL stats message.
> +         */
> +        MemSet(&walusage, 0, sizeof(WalUsage));
> +        WalUsageAccumDiff(&walusage, &pgWalUsage, &prevWalUsage);
>
> Isn't it better to move the code "prevWalUsage = pgWalUsage" into here?
> Because it's necessary only when pgWalUsage.wal_records !=
> prevWalUsage.wal_records.

Yes, I fixed it.

Regards,
--
Masahiro Ikeda
NTT DATA CORPORATION