Re: wal stats questions

On 2021/04/23 9:26, Masahiro Ikeda wrote:
> 
>
> On 2021/04/23 0:36, Andres Freund wrote:
>> Hi
>>
>> 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'm not sure why "long" was chosen for the counters in BufferUsage.
And then I guess that maybe we didn't change that because using "long"
for them caused no actual issue in practice.

> 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".

Yes, we can change the counters so they use uint64. But if we do that,
ISTM that we need to change the code more than your patch does.
For example, even with the patch, pg_stat_statements uses Int64GetDatumFast()
to report the counter like shared_blks_hit, but this should be changed?
For example, "%ld" should be changed to "%llu" at the following code in
vacuumlazy.c? I think that we should check all codes that use the counters
whose types are changed to uint64.

appendStringInfo(&buf,
_("WAL usage: %ld records, %ld full page images, %llu bytes"),
walusage.wal_records,
walusage.wal_fpi,
(unsigned long long) walusage.wal_bytes);

Regards,

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