Re: wal stats questions

On 2021-05-13 09:05, Masahiro Ikeda wrote:
> 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,

Thanks for updating the patch!

> + * is executed, wal records aren't nomally generated (although HOT
> makes

nomally -> normally?

> + * It's not enough to check the number of generated wal records, for
> + * example the walwriter may write/sync the WAL although it doesn't

You use both 'wal' and 'WAL' in the comments, but are there any
intension?

Regards,