Date: Fri, 28 Feb 2020 20:19:18 -0300
From: Alvaro Herrera <alvherre@2ndquadrant.com>
To: Tomas Vondra <tomas.vondra@2ndquadrant.com>
Cc: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>,
	"tsunakawa.takay@fujitsu.com" <tsunakawa.takay@fujitsu.com>,
	"pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
Subject: Re: SLRU statistics
Message-ID: <20200228231918.GA30292@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20200121135614.vt4cpuzwiyarzfca@development>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk

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.

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.

Patch applies cleanly.  I suggest to move the page_miss() call until
after SlruRecentlyUsed(), for consistency with the other case.

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.)

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services