SLRU statistics
Hi,
One of the stats I occasionally wanted to know are stats for the SLRU
stats (we have couple of those - clog, subtrans, ...). So here is a WIP
version of a patch adding that.
The implementation is fairly simple - the slru code updates counters in
local memory, and then sends them to the collector at the end of the
transaction (similarly to table/func stats). The collector stores it
similarly to global stats. And the collected stats are accessible
through pg_stat_slru.
The main issue is that we have multiple SLRU caches, and it seems handy
to have separate stats for each. OTOH the number of SLRU stats is not
fixed, so e.g. extensions might define their own SLRU caches. But
handing dynamic number of SLRU caches seems a bit hard (we'd need to
assign some sort of unique IDs etc.) so what I did was define a fixed
number of SLRU types
typedef enum SlruType
{
SLRU_CLOG,
SLRU_COMMIT_TS,
SLRU_MULTIXACT_OFFSET,
SLRU_MULTIXACT_MEMBER,
SLRU_SUBTRANS,
SLRU_ASYNC,
SLRU_OLDSERXID,
SLRU_OTHER
} SlruType;
with one group of counters for each group. The last type (SLRU_OTHER) is
used to store stats for all SLRUs that are not predefined. It wouldn't
be that difficult to store dynamic number of SLRUs, but I'm not sure how
to solve issues with identifying SLRUs etc. And there are probably very
few extensions adding custom SLRU anyway.
The one thing missing from the patch is a way to reset the SLRU stats,
similarly to how we can reset bgwriter stats.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
stats-slru-v1.patchtext/plain; charset=us-asciiDownload+412-9
From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
One of the stats I occasionally wanted to know are stats for the SLRU
stats (we have couple of those - clog, subtrans, ...). So here is a WIP
version of a patch adding that.
How can users take advantage of this information? I think we also need the ability to set the size of SLRU buffers. (I want to be freed from the concern about the buffer shortage by setting the buffer size to its maximum. For example, CLOG would be only 1 GB.)
Regards
Takayuki Tsunakawa
On Mon, Jan 20, 2020 at 01:04:33AM +0000, tsunakawa.takay@fujitsu.com wrote:
From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
One of the stats I occasionally wanted to know are stats for the SLRU
stats (we have couple of those - clog, subtrans, ...). So here is a WIP
version of a patch adding that.How can users take advantage of this information? I think we also need
the ability to set the size of SLRU buffers. (I want to be freed from
the concern about the buffer shortage by setting the buffer size to its
maximum. For example, CLOG would be only 1 GB.)
You're right the users can't really take advantage of this - my primary
motivation was providing a feedback for devs, benchmarking etc. That
might have been done with DEBUG messages or something, but this seems
more convenient.
I think it's unclear how desirable / necessary it is to allow users to
tweak those caches. I don't think we should have a GUC for everything,
but maybe there's some sort of heuristics to determine the size. The
assumption is we actually find practical workloads where the size of
these SLRUs is a performance issue.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2020-Jan-20, Tomas Vondra wrote:
On Mon, Jan 20, 2020 at 01:04:33AM +0000, tsunakawa.takay@fujitsu.com wrote:
From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
One of the stats I occasionally wanted to know are stats for the SLRU
stats (we have couple of those - clog, subtrans, ...). So here is a WIP
version of a patch adding that.How can users take advantage of this information? I think we also need
the ability to set the size of SLRU buffers. (I want to be freed from
the concern about the buffer shortage by setting the buffer size to its
maximum. For example, CLOG would be only 1 GB.)You're right the users can't really take advantage of this - my primary
motivation was providing a feedback for devs, benchmarking etc. That
might have been done with DEBUG messages or something, but this seems
more convenient.
I think the stats are definitely needed if we keep the current code.
I've researched some specific problems in this code, such as the need
for more subtrans SLRU buffers; IIRC it was pretty painful to figure out
what the problem was without counters, and it'd have been trivial with
them.
I think it's unclear how desirable / necessary it is to allow users to
tweak those caches. I don't think we should have a GUC for everything,
but maybe there's some sort of heuristics to determine the size. The
assumption is we actually find practical workloads where the size of
these SLRUs is a performance issue.
I expect we'll eventually realize the need for changes in this area.
Either configurability in the buffer pool sizes, or moving them to be
part of shared_buffers (IIRC Thomas Munro had a patch for this.)
Example: SLRUs like pg_commit and pg_subtrans have higher buffer
consumption as the range of open transactions increases; for many users
this is not a concern and they can live with the default values.
(I think when pg_commit (n�e pg_clog) buffers were increased, we should
have increased pg_subtrans buffers to match.)
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Mon, Jan 20, 2020 at 03:01:36PM -0300, Alvaro Herrera wrote:
On 2020-Jan-20, Tomas Vondra wrote:
On Mon, Jan 20, 2020 at 01:04:33AM +0000, tsunakawa.takay@fujitsu.com wrote:
From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
One of the stats I occasionally wanted to know are stats for the SLRU
stats (we have couple of those - clog, subtrans, ...). So here is a WIP
version of a patch adding that.How can users take advantage of this information? I think we also need
the ability to set the size of SLRU buffers. (I want to be freed from
the concern about the buffer shortage by setting the buffer size to its
maximum. For example, CLOG would be only 1 GB.)You're right the users can't really take advantage of this - my primary
motivation was providing a feedback for devs, benchmarking etc. That
might have been done with DEBUG messages or something, but this seems
more convenient.I think the stats are definitely needed if we keep the current code.
I've researched some specific problems in this code, such as the need
for more subtrans SLRU buffers; IIRC it was pretty painful to figure out
what the problem was without counters, and it'd have been trivial with
them.
Right. Improving our ability to monitor/measure things is the goal of
this patch.
I think it's unclear how desirable / necessary it is to allow users to
tweak those caches. I don't think we should have a GUC for everything,
but maybe there's some sort of heuristics to determine the size. The
assumption is we actually find practical workloads where the size of
these SLRUs is a performance issue.I expect we'll eventually realize the need for changes in this area.
Either configurability in the buffer pool sizes, or moving them to be
part of shared_buffers (IIRC Thomas Munro had a patch for this.)
Example: SLRUs like pg_commit and pg_subtrans have higher buffer
consumption as the range of open transactions increases; for many users
this is not a concern and they can live with the default values.(I think when pg_commit (n�e pg_clog) buffers were increased, we should
have increased pg_subtrans buffers to match.)
Quite possibly, yes. All I'm saying is that it's not something I intend
to address with this patch. It's quite possible the solutions will be
different for each SLRU, and that will require more research.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
You're right the users can't really take advantage of this - my primary
motivation was providing a feedback for devs, benchmarking etc. That
might have been done with DEBUG messages or something, but this seems
more convenient.
Understood. I'm in favor of adding performance information even if it doesn't make sense for users (like other DBMSs sometimes do.) One concern is that all the PostgreSQL performance statistics have been useful so far for tuning in some way, and this may become the first exception. Do we describe the SLRU stats view in the manual, or hide it only for PG devs and support staff?
I think it's unclear how desirable / necessary it is to allow users to
tweak those caches. I don't think we should have a GUC for everything,
but maybe there's some sort of heuristics to determine the size. The
assumption is we actually find practical workloads where the size of
these SLRUs is a performance issue.
I understood that the new performance statistics are expected to reveal what SLRUs need to be tunable and/or implemented with a different mechanism like shared buffers.
Regards
Takayuki Tsunakawa
On Tue, 21 Jan 2020 at 01:38, Tomas Vondra <tomas.vondra@2ndquadrant.com> wrote:
On Mon, Jan 20, 2020 at 01:04:33AM +0000, tsunakawa.takay@fujitsu.com wrote:
From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
One of the stats I occasionally wanted to know are stats for the SLRU
stats (we have couple of those - clog, subtrans, ...). So here is a WIP
version of a patch adding that.
+1
How can users take advantage of this information? I think we also need
the ability to set the size of SLRU buffers. (I want to be freed from
the concern about the buffer shortage by setting the buffer size to its
maximum. For example, CLOG would be only 1 GB.)You're right the users can't really take advantage of this - my primary
motivation was providing a feedback for devs, benchmarking etc. That
might have been done with DEBUG messages or something, but this seems
more convenient.
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.
Regards,
--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Tue, Jan 21, 2020 at 05:09:33PM +0900, Masahiko Sawada wrote:
On Tue, 21 Jan 2020 at 01:38, Tomas Vondra <tomas.vondra@2ndquadrant.com> wrote:
On Mon, Jan 20, 2020 at 01:04:33AM +0000, tsunakawa.takay@fujitsu.com wrote:
From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
One of the stats I occasionally wanted to know are stats for the SLRU
stats (we have couple of those - clog, subtrans, ...). So here is a WIP
version of a patch adding that.+1
How can users take advantage of this information? I think we also need
the ability to set the size of SLRU buffers. (I want to be freed from
the concern about the buffer shortage by setting the buffer size to its
maximum. For example, CLOG would be only 1 GB.)You're right the users can't really take advantage of this - my primary
motivation was providing a feedback for devs, benchmarking etc. That
might have been done with DEBUG messages or something, but this seems
more convenient.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.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Tue, Jan 21, 2020 at 06:24:29AM +0000, tsunakawa.takay@fujitsu.com
wrote:
From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
You're right the users can't really take advantage of this - my
primary motivation was providing a feedback for devs, benchmarking
etc. That might have been done with DEBUG messages or something, but
this seems more convenient.Understood. I'm in favor of adding performance information even if it
doesn't make sense for users (like other DBMSs sometimes do.) One
concern is that all the PostgreSQL performance statistics have been
useful so far for tuning in some way, and this may become the first
exception. Do we describe the SLRU stats view in the manual, or hide
it only for PG devs and support staff?
Yes, the pg_stat_slru view should be described in a manual. That's
missing from the patch.
I think it's unclear how desirable / necessary it is to allow users
to tweak those caches. I don't think we should have a GUC for
everything, but maybe there's some sort of heuristics to determine
the size. The assumption is we actually find practical workloads
where the size of these SLRUs is a performance issue.I understood that the new performance statistics are expected to reveal
what SLRUs need to be tunable and/or implemented with a different
mechanism like shared buffers.
Right. It's certainly meant to provide information for further tuning.
I'm just saying it's targeted more at developers, at least initially.
Maybe we'll end up with GUCs, maybe we'll choose other approaches for
some SLRUs. I don't have an opinion on that yet.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
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
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
On 2020-Feb-29, Tomas Vondra wrote:
Did we actually remove track-enabling GUCs? I think we still have
- track_activities
- track_counts
- track_io_timing
- track_functionsBut maybe I'm missing something?
Hm I remembered we removed the one for row-level stats
(track_row_stats), but what we really did is merge it with block-level
stats (track_block_stats) into track_counts -- commit 48f7e6439568.
Funnily enough, if you disable that autovacuum won't work, so I'm not
sure it's a very useful tunable. And it definitely has more overhead
than what this new GUC would have.
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.
Yeah, maybe we don't have to fix that now.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Sat, Feb 29, 2020 at 11:44:26AM -0300, Alvaro Herrera wrote:
On 2020-Feb-29, Tomas Vondra wrote:
Did we actually remove track-enabling GUCs? I think we still have
- track_activities
- track_counts
- track_io_timing
- track_functionsBut maybe I'm missing something?
Hm I remembered we removed the one for row-level stats
(track_row_stats), but what we really did is merge it with block-level
stats (track_block_stats) into track_counts -- commit 48f7e6439568.
Funnily enough, if you disable that autovacuum won't work, so I'm not
sure it's a very useful tunable. And it definitely has more overhead
than what this new GUC would have.
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.Yeah, maybe we don't have to fix that now.
IMO the current solution is sufficient for the purpose. I guess we could
just stick a name into the SlruCtlData (and remove SlruType entirely),
and use that to identify the stats entries. That might be enough, and in
fact we already have that - SimpleLruInit gets a name parameter and
copies that to the lwlock_tranche_name.
One of the main reasons why I opted to use the enum is that it makes
tracking, lookup and serialization pretty trivial - it's just an index
lookup, etc. But maybe it wouldn't be much more complex with the name,
considering the name length is limited by SLRU_MAX_NAME_LENGTH. And we
probably don't expect many entries, so we could keep them in a simple
list, or maybe a simplehash.
I'm not sure what to do with data for SLRUs that might have disappeared
after a restart (e.g. because someone removed an extension). Until now
those would be in the all in the "other" entry.
The attached v2 fixes the issues in your first message:
- I moved the page_miss() call after SlruRecentlyUsed(), but then I
realized it's entirely duplicate with the page_read() update done in
SlruPhysicalReadPage(). I removed the call from SlruPhysicalReadPage()
and renamed page_miss to page_read - that's more consistent with
shared buffers stats, which also have buffers_hit and buffer_read.
- I've also implemented the reset. I ended up adding a new option to
pg_stat_reset_shared, which always resets all SLRU entries. We track
the reset timestamp for each SLRU entry, but the value is always the
same. I admit this is a bit weird - I did it like this because (a) I'm
not sure how to identify the individual entries and (b) the SLRU is
shared, so pg_stat_reset_shared seems kinda natural.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
stats-slru-v2.patchtext/plain; charset=us-asciiDownload+434-10
Hi,
Attached is v3 of the patch with one big change and various small ones.
The main change is that it gets rid of the SlruType enum and the new
field in SlruCtlData. Instead, the patch now uses the name passed to
SimpleLruInit (which is then stored as LWLock tranche name).
The counters are still stored in a fixed-sized array, and there's a
simple name/index mapping. We don't have any registry of stable SLRU
IDs, so I can't think of anything better, and I think this is good
enough for now.
The other change is that I got rid of the io_error counter. We don't
have that for shared buffers etc. either, anyway.
I've also renamed the colunms from "pages" to "blks" to make it
consistent with other similar stats (blks_hit, blks_read). I've
renamed the fields to "blks_written" and "blks_zeroed".
And finally, I've added the view to monitoring.sgml.
Barring objections, I'll get this committed in the next few days, after
reviewing the comments a bit.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
0001-SLRU-stats-v3.patchtext/plain; charset=us-asciiDownload+500-2
Hi,
here is a bit improved version of the patch - I've been annoyed by how
the resetting works (per-entry timestamp, but resetting all entries) so
I've added a new function pg_stat_reset_slru() that allows resetting
either all entries or just one entry (identified by name). So
SELECT pg_stat_reset_slru('clog');
resets just "clog" SLRU counters, while
SELECT pg_stat_reset_slru(NULL);
resets all entries.
I've also done a bit of benchmarking, to see if this has measurable
impact (in which case it might deserve a new GUC), and I think it's not
measurable. I've used a tiny unlogged table (single row).
CREATE UNLOGGED TABLE t (a int);
INSERT INTO t VALUES (1);
and then short pgbench runs with a single client, updatint the row. I've
been unable to measure any regression, it's all well within 1% so noise.
But perhaps there's some other benchmark that I should do?
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
0001-Collect-SLRU-statistics.patchtext/plain; charset=us-asciiDownload+615-1
Hi,
I've pushed this after some minor cleanup and improvements.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Hi,
Thank you for developing great features.
The attached patch is a small fix to the committed documentation for the data type name of blks_hit column.
Best regards,
Noriyoshi Shinoda
-----Original Message-----
From: Tomas Vondra [mailto:tomas.vondra@2ndquadrant.com]
Sent: Thursday, April 2, 2020 9:42 AM
To: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>; tsunakawa.takay@fujitsu.com; pgsql-hackers@postgresql.org
Subject: Re: SLRU statistics
Hi,
I've pushed this after some minor cleanup and improvements.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
monitoring_pg_stat_slru.diffapplication/octet-stream; name=monitoring_pg_stat_slru.diffDownload+1-1
On Thu, Apr 02, 2020 at 02:04:10AM +0000, Shinoda, Noriyoshi (PN Japan A&PS Delivery) wrote:
Hi,
Thank you for developing great features.
The attached patch is a small fix to the committed documentation for the data type name of blks_hit column.
Thank you for the patch, pushed.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Hello Tomas,
On Thu, Apr 2, 2020 at 5:59 PM Tomas Vondra
<tomas.vondra@2ndquadrant.com> wrote:
Thank you for the patch, pushed.
In SimpleLruReadPage_ReadOnly, we first try to find the SLRU page in
shared buffer under shared lock, then conditionally visit
SimpleLruReadPage if reading is necessary. IMHO, we should update
hit_count if we can find the buffer in SimpleLruReadPage_ReadOnly
directly. Am I missing something?
Attached a patch for the same.
--
Thanks & Regards,
Kuntal Ghosh
EnterpriseDB: http://www.enterprisedb.com
Attachments:
v1-0001-Update-stats-in-SimpleLruReadPage_ReadOnly.patchapplication/octet-stream; name=v1-0001-Update-stats-in-SimpleLruReadPage_ReadOnly.patchDownload+4-1
On Tue, Apr 07, 2020 at 05:01:37PM +0530, Kuntal Ghosh wrote:
Hello Tomas,
On Thu, Apr 2, 2020 at 5:59 PM Tomas Vondra
<tomas.vondra@2ndquadrant.com> wrote:Thank you for the patch, pushed.
In SimpleLruReadPage_ReadOnly, we first try to find the SLRU page in
shared buffer under shared lock, then conditionally visit
SimpleLruReadPage if reading is necessary. IMHO, we should update
hit_count if we can find the buffer in SimpleLruReadPage_ReadOnly
directly. Am I missing something?Attached a patch for the same.
Yes, I think that's correct - without this we fail to account for
(possibly) a quite significant number of hits. Thanks for the report,
I'll get this pushed later today.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
