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
