Re: wal stats questions - Mailing list pgsql-hackers

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,