Re: SLRU statistics

On 2020/05/13 17:21, Tomas Vondra wrote:
> On Wed, May 13, 2020 at 04:10:30PM +0900, Fujii Masao wrote:
>>
>>
>> On 2020/05/07 13:47, Fujii Masao wrote:
>>>
>>>
>>> On 2020/05/03 1:59, Tomas Vondra wrote:
>>>> On Sat, May 02, 2020 at 12:55:00PM +0200, Tomas Vondra wrote:
>>>>> On Sat, May 02, 2020 at 03:56:07PM +0900, Fujii Masao wrote:
>>>>>>
>>>>>> ...
>>>>>
>>>>>>>> Another thing I found is; pgstat_send_slru() should be called also by
>>>>>>>> other processes than backend? For example, since clog data is flushed
>>>>>>>> basically by checkpointer, checkpointer seems to need to send slru stats.
>>>>>>>> Otherwise, pg_stat_slru.flushes would not be updated.
>>>>>>>>
>>>>>>>
>>>>>>> Hmmm, that's a good point. If I understand the issue correctly, the
>>>>>>> checkpointer accumulates the stats but never really sends them because
>>>>>>> it never calls pgstat_report_stat/pgstat_send_slru. That's only called
>>>>>>> from PostgresMain, but not from CheckpointerMain.
>>>>>>
>>>>>> Yes.
>>>>>>
>>>>>>> I think we could simply add pgstat_send_slru() right after the existing
>>>>>>> call in CheckpointerMain, right?
>>>>>>
>>>>>> Checkpointer sends off activity statistics to the stats collector in
>>>>>> two places, by calling pgstat_send_bgwriter(). What about calling
>>>>>> pgstat_send_slru() just after pgstat_send_bgwriter()?
>>>>>>
>>>>>
>>>>> Yep, that's what I proposed.
>>>>>
>>>>>> In previous email, I mentioned checkpointer just as an example.
>>>>>> So probably we need to investigate what process should send slru stats,
>>>>>> other than checkpointer. I guess that at least autovacuum worker,
>>>>>> logical replication walsender and parallel query worker (maybe this has
>>>>>> been already covered by parallel query some mechanisms. Sorry I've
>>>>>> not checked that) would need to send its slru stats.
>>>>>>
>>>>>
>>>>> Probably. Do you have any other process type in mind?
>>>
>>> No. For now what I'm in mind are just checkpointer, autovacuum worker,
>>> logical replication walsender and parallel query worker. Seems logical
>>> replication worker and syncer have sent slru stats via pgstat_report_stat().
>>>
>>>> I've looked at places calling pgstat_send_* functions, and I found
>>>> thsese places:
>>>>
>>>> src/backend/postmaster/bgwriter.c
>>>>
>>>> - AFAIK it merely writes out dirty shared buffers, so likely irrelevant.
>>>>
>>>> src/backend/postmaster/checkpointer.c
>>>>
>>>> - This is what we're already discussing here.
>>>>
>>>> src/backend/postmaster/pgarch.c
>>>>
>>>> - Seems irrelevant.
>>>>
>>>>
>>>> I'm a bit puzzled why we're not sending any stats from walsender, which
>>>> I suppose could do various stuff during logical decoding.
>>>
>>> Not sure why, but that seems an oversight...
>>>
>>>
>>> Also I found another minor issue; SLRUStats has not been initialized to 0
>>> and which could update the counters unexpectedly. Attached patch fixes
>>> this issue.
>>
>> This is minor issue, but basically it's better to fix that before
>> v13 beta1 release. So barring any objection, I will commit the patch.
>>
>> +        values[8] = Int64GetDatum(stat.stat_reset_timestamp);
>>
>> Also I found another small issue: pg_stat_get_slru() returns the timestamp
>> when pg_stat_slru was reset by using Int64GetDatum(). This works maybe
>> because the timestamp is also int64. But TimestampTzGetDatum() should
>> be used here, instead. Patch attached. Thought?
>>
>
> I agree with both fixes.

Pushed both. Thanks!

Regards,

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