From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
To: 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: Sat, 29 Feb 2020 13:24:41 +0100
Message-ID: <20200229122441.ow7gnnqgesmojyc3@development> (raw)
In-Reply-To: <20200228231918.GA30292@alvherre.pgsql>
References: <20200121135614.vt4cpuzwiyarzfca@development>
<20200228231918.GA30292@alvherre.pgsql>
On Fri, Feb 28, 2020 at 08:19:18PM -0300, Alvaro Herrera wrote:
>On 2020-Jan-21, Tomas Vondra wrote:
>
>> On Tue, Jan 21, 2020 at 05:09:33PM +0900, Masahiko Sawada wrote:
>
>> > I've not tested the performance impact but perhaps we might want to
>> > disable these counter by default and controlled by a GUC. And similar
>> > to buffer statistics it might be better to inline
>> > pgstat_count_slru_page_xxx function for better performance.
>>
>> Hmmm, yeah. Inlining seems like a good idea, and maybe we should have
>> something like track_slru GUC.
>
>I disagree with adding a GUC. If a performance impact can be measured
>let's turn the functions to static inline, as already proposed. My
>guess is that pgstat_count_slru_page_hit() is the only candidate for
>that; all the other paths involve I/O or lock acquisition or even WAL
>generation, so the impact won't be measurable anyhow. We removed
>track-enabling GUCs years ago.
>
Did we actually remove track-enabling GUCs? I think we still have
- track_activities
- track_counts
- track_io_timing
- track_functions
But maybe I'm missing something?
That being said, I'm not sure we need to add a GUC. I'll do some
measurements and we'll see. Maybe the statis inline will me enough.
>BTW, this comment:
> /* update the stats counter of pages found in shared buffers */
>
>is not strictly true, because we don't use what we normally call "shared
>buffers" for SLRUs.
>
Oh, right. Will fix.
>Patch applies cleanly. I suggest to move the page_miss() call until
>after SlruRecentlyUsed(), for consistency with the other case.
>
OK.
>I find SlruType pretty odd, and the accompanying "if" list in
>pg_stat_get_slru() correspondingly so. Would it be possible to have
>each SLRU enumerate itself somehow? Maybe add the name in SlruCtlData
>and query that, somehow. (I don't think we have an array of SlruCtlData
>anywhere though, so this might be a useless idea.)
>
Well, maybe. We could have a system to register SLRUs dynamically, but
the trick here is that by having a fixed predefined number of SLRUs
simplifies serialization in pgstat.c and so on. I don't think the "if"
branches in pg_stat_get_slru() are particularly ugly, but maybe we could
replace the enum with a registry of structs, something like rmgrlist.h.
It seems like an overkill to me, though.
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, alvherre@2ndquadrant.com, masahiko.sawada@2ndquadrant.com, tsunakawa.takay@fujitsu.com
Subject: Re: SLRU statistics
In-Reply-To: <20200229122441.ow7gnnqgesmojyc3@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