From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
To: Fujii Masao <masao.fujii@oss.nttdata.com>
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
Cc: tsunakawa.takay@fujitsu.com <tsunakawa.takay@fujitsu.com>
Cc: pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
Subject: Re: SLRU statistics
Date: Wed, 13 May 2020 10:21:59 +0200
Message-ID: <20200513082159.myflgcs5fnhnujbk@development> (raw)
In-Reply-To: <b8784fe6-1401-ab35-aa14-d57b5bb8e312@oss.nttdata.com>
References: <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>
<b8784fe6-1401-ab35-aa14-d57b5bb8e312@oss.nttdata.com>
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.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tomas.vondra@2ndquadrant.com, masao.fujii@oss.nttdata.com, alvherre@2ndquadrant.com, masahiko.sawada@2ndquadrant.com, tsunakawa.takay@fujitsu.com
Subject: Re: SLRU statistics
In-Reply-To: <20200513082159.myflgcs5fnhnujbk@development>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox