Subject: Re: SLRU statistics
From: Fujii Masao <masao.fujii@oss.nttdata.com>
To: Tomas Vondra <tomas.vondra@2ndquadrant.com>
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>,
 Masahiko Sawada <masahiko.sawada@2ndquadrant.com>,
 "tsunakawa.takay@fujitsu.com" <tsunakawa.takay@fujitsu.com>,
 "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
References: <20200229205518.xrxo535zgd7cl6no@development>
 <20200327004111.md4suoi6u5suv2qe@development>
 <20200328002630.2nz5y3dvymqnvxtv@development>
 <20200402004136.wodwftan45mcwggm@development>
 <f02fd0ec-a240-b5bf-9992-5faf582738f1@oss.nttdata.com>
 <20200430181901.24tlriahmafi4sfn@development>
 <0e1c8904-13be-ab70-0096-54f0b30e43ba@oss.nttdata.com>
 <20200502000818.nn4kpewiput42bnb@development>
 <49b8ad17-f6d9-a086-a952-ff1fec3ac1b3@oss.nttdata.com>
 <20200502105500.hwhba3d6s3hlrqf6@development>
 <20200502165905.brfxv6dk5vjdbgti@development>
 <976bbb73-a112-de3c-c488-b34b64609793@oss.nttdata.com>
Message-ID: <b8784fe6-1401-ab35-aa14-d57b5bb8e312@oss.nttdata.com>
Date: Wed, 13 May 2020 16:10:30 +0900
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:68.0)
 Gecko/20100101 Thunderbird/68.8.0
MIME-Version: 1.0
In-Reply-To: <976bbb73-a112-de3c-c488-b34b64609793@oss.nttdata.com>
Content-Type: multipart/mixed;
 boundary="------------B2BB23E0BDD78D6EE3666BB2"
Content-Language: en-US
Precedence: bulk

This is a multi-part message in MIME format.
--------------B2BB23E0BDD78D6EE3666BB2
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit


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?

Regards,


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

--------------B2BB23E0BDD78D6EE3666BB2
Content-Type: text/plain; charset=UTF-8; x-mac-type="0"; x-mac-creator="0";
 name="pg_stat_slru_reset_timestamp.patch"
Content-Transfer-Encoding: base64
Content-Disposition: attachment;
 filename="pg_stat_slru_reset_timestamp.patch"

ZGlmZiAtLWdpdCBhL3NyYy9iYWNrZW5kL3V0aWxzL2FkdC9wZ3N0YXRmdW5jcy5jIGIvc3Jj
L2JhY2tlbmQvdXRpbHMvYWR0L3Bnc3RhdGZ1bmNzLmMKaW5kZXggNDQ2MDQ0NjA5ZS4uNmI0
NzYxNzMyOCAxMDA2NDQKLS0tIGEvc3JjL2JhY2tlbmQvdXRpbHMvYWR0L3Bnc3RhdGZ1bmNz
LmMKKysrIGIvc3JjL2JhY2tlbmQvdXRpbHMvYWR0L3Bnc3RhdGZ1bmNzLmMKQEAgLTE3NTcs
NyArMTc1Nyw3IEBAIHBnX3N0YXRfZ2V0X3NscnUoUEdfRlVOQ1RJT05fQVJHUykKIAkJdmFs
dWVzWzVdID0gSW50NjRHZXREYXR1bShzdGF0LmJsb2Nrc19leGlzdHMpOwogCQl2YWx1ZXNb
Nl0gPSBJbnQ2NEdldERhdHVtKHN0YXQuZmx1c2gpOwogCQl2YWx1ZXNbN10gPSBJbnQ2NEdl
dERhdHVtKHN0YXQudHJ1bmNhdGUpOwotCQl2YWx1ZXNbOF0gPSBJbnQ2NEdldERhdHVtKHN0
YXQuc3RhdF9yZXNldF90aW1lc3RhbXApOworCQl2YWx1ZXNbOF0gPSBUaW1lc3RhbXBUekdl
dERhdHVtKHN0YXQuc3RhdF9yZXNldF90aW1lc3RhbXApOwogCiAJCXR1cGxlc3RvcmVfcHV0
dmFsdWVzKHR1cHN0b3JlLCB0dXBkZXNjLCB2YWx1ZXMsIG51bGxzKTsKIAl9Cg==
--------------B2BB23E0BDD78D6EE3666BB2--