Re: wal stats questions

On 2021/05/18 9:57, Masahiro Ikeda wrote:
> 
> 
> On 2021/05/17 22:34, Fujii Masao wrote:
>>
>>
>> On 2021/05/17 16:39, Masahiro Ikeda wrote:
>>>
>>> Thanks for your comments!
>>>
>>>>> +     * is executed, wal records aren't nomally generated (although HOT makes
>>>>
>>>> nomally -> normally?
>>>
>>> Yes, fixed.
>>>
>>>>> +     * 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?
>>>
>>> No, I changed to use 'WAL' only. Although some other comments have 'wal' and
>>> 'WAL', it seems that 'WAL' is often used.
>>
>> Thanks for updating the patch!
> 
> Thanks a lot of comments!
> 
>> + * Buffer and generated WAL usage counters.
>> + *
>> + * The counters are accumulated values. There are infrastructures
>> + * to add the values and calculate the difference within a specific period.
>>
>> Is it really worth adding these comments here?
> 
> BufferUsage has almost same comments. So, I removed it.
> 
>> +     * Note: regarding to WAL statistics counters, it's not enough to check
>> +     * only whether the WAL record is generated or not. If a read transaction
>> +     * is executed, WAL records aren't normally generated (although HOT makes
>> +     * WAL records). But, just writes and syncs the WAL data may happen when
>> +     * to flush buffers.
>>
>> Aren't the following comments better?
>>
>> ------------------------------
>> To determine whether any WAL activity has occurred since last time, not only
>> the number of generated WAL records but also the numbers of WAL writes and
>> syncs need to be checked. Because even transaction that generates no WAL
>> records can write or sync WAL data when flushing the data pages.
>> ------------------------------
> 
> Thanks. Yes, I fixed it.
> 
>> -     * This function can be called even if nothing at all has happened. In
>> -     * this case, avoid sending a completely empty message to the stats
>> -     * collector.
>>
>> I think that it's better to leave this comment as it is.
> 
> OK. I leave it.
> 
>> +     * First, to check the WAL stats counters were updated.
>> +     *
>> +     * Even if the 'force' is true, we don't need to send the stats if the
>> +     * counters were not updated.
>> +     *
>> +     * We can know whether the counters were updated or not to check only
>> +     * three counters. So, for performance, we don't allocate another memory
>> +     * spaces and check the all stats like pgstat_send_slru().
>>
>> Is it really worth adding these comments here?
> 
> I removed them because the following comments are enough.
> 
> * This function can be called even if nothing at all has happened. In
> * this case, avoid sending a completely empty message to the stats
> * collector.
> 
>> +     * The current 'wal_records' is the same as the previous one means that
>> +     * WAL records weren't generated. So, the counters of 'wal_fpi',
>> +     * 'wal_bytes', 'm_wal_buffers_full' are not updated neither.
>> +     *
>> +     * 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
>> +     * generate WAL records. 'm_wal_write' and 'm_wal_sync' are zero means the
>> +     * counters of time spent are zero too.
>>
>> Aren't the following comments better?
>>
>> ------------------------
>> Check wal_records counter to determine whether any WAL activity has happened
>> since last time. Note that other WalUsage counters don't need to be checked
>> because they are incremented always together with wal_records counter.
>>
>> m_wal_buffers_full also doesn't need to be checked because it's incremented
>> only when at least one WAL record is generated (i.e., wal_records counter is
>> incremented). But for safely, we assert that m_wal_buffers_full is always zero
>> when no WAL record is generated
>>
>> This function can be called by a process like walwriter that normally
>> generates no WAL records. To determine whether any WAL activity has happened
>> at that process since the last time, the numbers of WAL writes and syncs are
>> also checked.
>> ------------------------
> 
> Yes, I modified them.
> 
>> + * The accumulated counters for buffer usage.
>> + *
>> + * The reason the counters are accumulated values is to avoid unexpected
>> + * reset because many callers may use them.
>>
>> Aren't the following comments better?
>>
>> ------------------------
>> These counters keep being incremented infinitely, i.e., must never be reset to
>> zero, so that we can calculate how much the counters are incremented in an
>> arbitrary period.
>> ------------------------
> 
> Yes, thanks.
> I fixed it.

Thanks for updating the patch! I modified some comments slightly and
pushed that version of the patch.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION