Re: wal stats questions

On 2021/04/26 10:11, Masahiro Ikeda wrote:
>
>
> On 2021/04/23 16:30, Fujii Masao wrote:
>>
>>
>> On 2021/04/23 10:25, Andres Freund wrote:
>>> Hi,
>>>
>>> On 2021-04-23 09:26:17 +0900, Masahiro Ikeda wrote:
>>>> On 2021/04/23 0:36, Andres Freund wrote:
>>>>> On Thu, Apr 22, 2021, at 06:42, Fujii Masao wrote:
>>>>>> On 2021/04/21 18:31, Masahiro Ikeda wrote:
>>>>>>>> BTW, is it better to change PgStat_Counter from int64 to uint64
>>>>>>>> because> there aren't any counters with negative number?
>>>>>> On second thought, it's ok even if the counters like wal_records get
>>>>>> overflowed.
>>>>>> Because they are always used to calculate the diff between the previous and
>>>>>> current counters. Even when the current counters are overflowed and
>>>>>> the previous ones are not, WalUsageAccumDiff() seems to return the right
>>>>>> diff of them. If this understanding is right, I'd withdraw my comment and
>>>>>> it's ok to use "long" type for those counters. Thought?
>>>>>
>>>>> Why long? It's of a platform dependent size, which doesn't seem useful here?
>>>>
>>>> I think it's ok to unify uint64. Although it's better to use small size for
>>>> cache, the idea works well for only some platform which use 4bytes for "long".
>>>>
>>>>
>>>> (Although I'm not familiar with the standardization...)
>>>> It seems that we need to adopt unsinged long if use "long", which may be
>>>> 4bytes.
>>>>
>>>> I though WalUsageAccumDiff() seems to return the right value too. But, I
>>>> researched deeply and found that ISO/IEC 9899:1999 defines unsinged integer
>>>> never overflow(2.6.5 Types 9th section) although it doesn't define overflow of
>>>> signed integer type.
>>>>
>>>> If my understanding is right, the definition only guarantee
>>>> WalUsageAccumDiff() returns the right value only if the type is unsigned
>>>> integer. So, it's safe to change "long" to "unsigned long".
>>>
>>> I think this should just use 64bit counters. Emitting more than 4
>>> billion records in one transaction isn't actually particularly rare. And
>>
>> Right. I agree to change the types of the counters to int64.
>>
>> I think that it's better to make this change as a separate patch from
>> the changes for pg_stat_wal view.
>
> Thanks for your comments.
> OK, I separate two patches.

Thanks!

>
> 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?

Is there really the case where the number of sync is larger than zero when
the number of writes is zero? If not, it's enough to check only the number
of writes?

+ * wal records weren't generated. So, the counters of 'wal_fpi',
+ * 'wal_bytes', 'm_wal_buffers_full' are not updated neither.

It's better to add the assertion check that confirms
m_wal_buffers_full == 0 whenever wal_records is larger than zero?

>
> Second one has the changes for the type of the BufferUsage's and WalUsage's
> members. I change the type from long to int64. Is it better to make new thread?
> ("v6-0002-change-the-data-type-of-XXXUsage-from-long-to-int64.patch")

Will review the patch later. I'm ok to discuss that in this thread.

Regards,

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