[PATCH] Runtime control of CLOBBER_CACHE_ALWAYS
Hi all
While working on extensions I've often wanted to enable cache clobbering
for a targeted piece of code, without paying the price of running it for
all backends during postgres startup and any initial setup tasks that are
required.
So here's a patch that, when CLOBBER_CACHE_ALWAYS or
CLOBBER_CACHE_RECURSIVE are defined, adds a GUC named clobber_cache_depth .
It defaults to 1 for CLOBBER_CACHE_ALWAYS or 3 for CLOBBER_CACHE_RECURSIVE
to match the existing compiled-in behaviour. But with this change it's now
possible to run Pg with clobber_cache_depth=0 then set it to 1 only for
targeted tests.
clobber_cache_depth is treated as an unknown GUC if Pg was not built with
CLOBBER_CACHE_ALWAYS or CLOBBER_CACHE_RECURSIVE defined.
-----
On a side note, to make things like this easier to use, I personally patch
all pg_regress tests to include the following at the start of each sql
input file:
\set ECHO none
-- Put per-test settings or overrides here
\set ECHO queries
then patch the expected files accordingly. That way it's easy for me to
make per-test adjustments while still running the whole suite. It's not
always practical to run just one targeted test with TESTS=foo.
--
Craig Ringer http://www.2ndQuadrant.com/
2ndQuadrant - PostgreSQL Solutions for the Enterprise
Attachments:
v1-0001-Add-runtime-control-over-CLOBBER_CACHE_ALWAYS.patchtext/x-patch; charset=US-ASCII; name=v1-0001-Add-runtime-control-over-CLOBBER_CACHE_ALWAYS.patchDownload
From 372defde443d178fb4a9c8cf4092dea7debf72ea Mon Sep 17 00:00:00 2001
From: Craig Ringer <craig.ringer@2ndquadrant.com>
Date: Tue, 22 Sep 2020 09:51:00 +0800
Subject: [PATCH v1] Add runtime control over CLOBBER_CACHE_ALWAYS
When CLOBBER_CACHE_ALWAYS or CLOBBER_CACHE_RECURSIVE are defined,
a new GUC "clobber_cache_depth" becomes available. This allows runtime
control over the behaviour of cache clobber builds in order to allow
more targeted testing of new or changed features using aggressive
cache clobbering.
clobber_cache_depth defaults to 1 if CLOBBER_CACHE_ALWAYS is defined,
and to 3 if CLOBBER_CACHE_RECURSIVE is defined. That makes it match
the previous hardcoded behaviour of these macros, ensuring buildfarm
members won't get upset.
While we're at it, expose the macros in pg_config_manual.h
---
src/backend/utils/cache/inval.c | 51 +++++++++++++++++++--------------
src/backend/utils/misc/guc.c | 15 ++++++++++
src/include/pg_config_manual.h | 28 ++++++++++++++++--
src/include/utils/inval.h | 8 ++++++
4 files changed, 78 insertions(+), 24 deletions(-)
diff --git a/src/backend/utils/cache/inval.c b/src/backend/utils/cache/inval.c
index 628d6f5d0c..620c9558ac 100644
--- a/src/backend/utils/cache/inval.c
+++ b/src/backend/utils/cache/inval.c
@@ -109,6 +109,7 @@
#include "storage/sinval.h"
#include "storage/smgr.h"
#include "utils/catcache.h"
+#include "utils/guc.h"
#include "utils/inval.h"
#include "utils/memdebug.h"
#include "utils/memutils.h"
@@ -179,6 +180,9 @@ static SharedInvalidationMessage *SharedInvalidMessagesArray;
static int numSharedInvalidMessagesArray;
static int maxSharedInvalidMessagesArray;
+#if CACHE_CLOBBER_ALWAYS
+int clobber_cache_depth = 0;
+#endif
/*
* Dynamically-registered callback functions. Current implementation
@@ -689,35 +693,38 @@ AcceptInvalidationMessages(void)
/*
* Test code to force cache flushes anytime a flush could happen.
*
- * If used with CLOBBER_FREED_MEMORY, CLOBBER_CACHE_ALWAYS provides a
+ * CLOBBER_CACHE_ALWAYS (clobber_cache_depth = 1) provides a
* fairly thorough test that the system contains no cache-flush hazards.
* However, it also makes the system unbelievably slow --- the regression
- * tests take about 100 times longer than normal.
- *
- * If you're a glutton for punishment, try CLOBBER_CACHE_RECURSIVELY. This
- * slows things by at least a factor of 10000, so I wouldn't suggest
- * trying to run the entire regression tests that way. It's useful to try
- * a few simple tests, to make sure that cache reload isn't subject to
- * internal cache-flush hazards, but after you've done a few thousand
- * recursive reloads it's unlikely you'll learn more.
+ * tests take about 100 times longer than normal. To mitigate the
+ * slowdown it can be turned on and off per-backend or globally using
+ * the clobber_cache_depth GUC to allow targeted testing of specific
+ * code paths.
+ *
+ * If you're a glutton for punishment, try CLOBBER_CACHE_RECURSIVELY
+ * (set clobber_cache_depth to > 1). This slows things by at least a
+ * factor of 10000, so I wouldn't suggest trying to run the entire
+ * regression tests that way. It's useful to try a few simple tests,
+ * to make sure that cache reload isn't subject to internal cache-flush
+ * hazards, but after you've done a few thousand recursive reloads it's
+ * unlikely you'll learn more.
+ *
+ * You can use postgresql.conf or any other convenient means to disable
+ * clobbering by default then re-enable for targeted sections of tests,
+ * e.g you can edit a specific pg_regress test to SET
+ * clobber_cache_depth=1, then run the suite with:
+ *
+ * PGOPTIONS="-c clobber_cache_depth=0" make -C src/test/regress check
+ *
+ * Remember to use ALTER SYSTEM SET and reload the config if you want
+ * the change to apply to bgworkers, autovacuum, etc too.
*/
#if defined(CLOBBER_CACHE_ALWAYS)
- {
- static bool in_recursion = false;
-
- if (!in_recursion)
- {
- in_recursion = true;
- InvalidateSystemCaches();
- in_recursion = false;
- }
- }
-#elif defined(CLOBBER_CACHE_RECURSIVELY)
+ if (clobber_cache_depth > 0)
{
static int recursion_depth = 0;
- /* Maximum depth is arbitrary depending on your threshold of pain */
- if (recursion_depth < 3)
+ if (recursion_depth < clobber_cache_depth)
{
recursion_depth++;
InvalidateSystemCaches();
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 596bcb7b84..ae7d46cff0 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -99,6 +99,7 @@
#include "utils/rls.h"
#include "utils/snapmgr.h"
#include "utils/tzparser.h"
+#include "utils/inval.h"
#include "utils/varlena.h"
#include "utils/xml.h"
@@ -3399,7 +3400,21 @@ static struct config_int ConfigureNamesInt[] =
check_huge_page_size, NULL, NULL
},
+#if defined(CLOBBER_CACHE_ALWAYS) || defined(CLOBBER_CACHE_RECURSIVELY)
+ /* Only available in special debug builds, see pg_config_manual.h */
+ {
+ {"clobber_cache_depth", PGC_SUSET, DEVELOPER_OPTIONS,
+ gettext_noop("Runtime control over CLOBBER_CACHE_ALWAYS and CLOBBER_CACHE_RECURSIVELY",
+ NULL,
+ GUC_NOT_IN_SAMPLE
+ },
+ &clobber_cache_depth,
+ DEFAULT_CLOBBER_CACHE_DEPTH, 0, 5,
+ NULL, NULL, NULL
+ },
+
/* End-of-list marker */
+#endif
{
{NULL, 0, 0, NULL, NULL}, NULL, 0, 0, 0, NULL, NULL, NULL
}
diff --git a/src/include/pg_config_manual.h b/src/include/pg_config_manual.h
index 705dc69c06..615e0833f1 100644
--- a/src/include/pg_config_manual.h
+++ b/src/include/pg_config_manual.h
@@ -6,8 +6,15 @@
* for developers. If you edit any of these, be sure to do a *full*
* rebuild (and an initdb if noted).
*
+ * You can define these by editing pg_config_manual.h but it's usually
+ * sufficient to pass CFLAGS to configure, e.g.
+ *
+ * ./configure --enable-debug --enable-cassert CFLAGS="-DUSE_VALGRIND"
+ *
+ * The flags are persisted in Makefile.global so they will be correctly
+ * applied to extensions, including those built by PGXS.
+ *
* Portions Copyright (c) 1996-2020, PostgreSQL Global Development Group
- * Portions Copyright (c) 1994, Regents of the University of California
*
* src/include/pg_config_manual.h
*------------------------------------------------------------------------
@@ -280,7 +287,8 @@
* percentage points even when not run under Valgrind.
*
* You should normally use MEMORY_CONTEXT_CHECKING with USE_VALGRIND;
- * instrumentation of repalloc() is inferior without it.
+ * instrumentation of repalloc() is inferior without it. So it's defined
+ * automatically for USE_VALGRIND builds.
*/
/* #define USE_VALGRIND */
@@ -309,6 +317,22 @@
*/
/* #define RANDOMIZE_ALLOCATED_MEMORY */
+/*
+ * For cache invalidation debugging, clobber caches whenever an invalidation is
+ * possible. For CLOBBER_CACHE_RECURSIVELY, do so even when already processing
+ * invalidations. The GUC clobber_cache_depth is only available when these
+ * options are defined. See inval.c for details.
+ *
+ * It's strongly recommended that you use --enable-cassert and the
+ * implied CLOBBER_FREED_MEMORY if you enable either of these.
+ */
+/* #define CLOBBER_CACHE_ALWAYS */
+/* #define CLOBBER_CACHE_RECURSIVELY */
+
+#if defined(CLOBBER_CACHE_RECURSIVELY) && !defined(CLOBBER_CACHE_ALWAYS)
+#define CLOBBER_CACHE_ALWAYS
+#endif
+
/*
* Define this to force all parse and plan trees to be passed through
* copyObject(), to facilitate catching errors and omissions in
diff --git a/src/include/utils/inval.h b/src/include/utils/inval.h
index 463888c389..ddfae7a9d6 100644
--- a/src/include/utils/inval.h
+++ b/src/include/utils/inval.h
@@ -18,6 +18,14 @@
#include "storage/relfilenode.h"
#include "utils/relcache.h"
+#ifdef CLOBBER_CACHE_ALWAYS
+extern PGDLLIMPORT int clobber_cache_depth;
+#ifdef CLOBBER_CACHE_RECURSIVELY
+#define DEFAULT_CLOBBER_CACHE_DEPTH 3
+#else
+#define DEFAULT_CLOBBER_CACHE_DEPTH 1
+#endif
+#endif
typedef void (*SyscacheCallbackFunction) (Datum arg, int cacheid, uint32 hashvalue);
typedef void (*RelcacheCallbackFunction) (Datum arg, Oid relid);
--
2.26.2
On 2020-09-25 09:40, Craig Ringer wrote:
While working on extensions I've often wanted to enable cache clobbering
for a targeted piece of code, without paying the price of running it for
all backends during postgres startup and any initial setup tasks that
are required.So here's a patch that, when CLOBBER_CACHE_ALWAYS or
CLOBBER_CACHE_RECURSIVE are defined, adds a GUC named
clobber_cache_depth . It defaults to 1 for CLOBBER_CACHE_ALWAYS or 3 for
CLOBBER_CACHE_RECURSIVE to match the existing compiled-in behaviour. But
with this change it's now possible to run Pg with clobber_cache_depth=0
then set it to 1 only for targeted tests.clobber_cache_depth is treated as an unknown GUC if Pg was not built
with CLOBBER_CACHE_ALWAYS or CLOBBER_CACHE_RECURSIVE defined.
I think it would be great if the cache clobber code is always compiled
in (but turned off) when building with assertions. The would reduce the
number of hoops to jump through to actually use this locally and reduce
the number of surprises from the build farm.
For backward compatibility, you can arrange it so that the built-in
default of clobber_cache_depth is 1 or 3 if CLOBBER_CACHE_ALWAYS or
CLOBBER_CACHE_RECURSIVE are defined.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 27.10.2020 11:34, Peter Eisentraut wrote:
On 2020-09-25 09:40, Craig Ringer wrote:
While working on extensions I've often wanted to enable cache
clobbering for a targeted piece of code, without paying the price of
running it for all backends during postgres startup and any initial
setup tasks that are required.So here's a patch that, when CLOBBER_CACHE_ALWAYS or
CLOBBER_CACHE_RECURSIVE are defined, adds a GUC named
clobber_cache_depth . It defaults to 1 for CLOBBER_CACHE_ALWAYS or 3
for CLOBBER_CACHE_RECURSIVE to match the existing compiled-in
behaviour. But with this change it's now possible to run Pg with
clobber_cache_depth=0 then set it to 1 only for targeted tests.clobber_cache_depth is treated as an unknown GUC if Pg was not built
with CLOBBER_CACHE_ALWAYS or CLOBBER_CACHE_RECURSIVE defined.I think it would be great if the cache clobber code is always compiled
in (but turned off) when building with assertions. The would reduce
the number of hoops to jump through to actually use this locally and
reduce the number of surprises from the build farm.For backward compatibility, you can arrange it so that the built-in
default of clobber_cache_depth is 1 or 3 if CLOBBER_CACHE_ALWAYS or
CLOBBER_CACHE_RECURSIVE are defined.
Status update for a commitfest entry.
This thread was inactive during the commitfest, so I am going to mark it
as "Returned with Feedback".
Craig, are you planning to continue working on it? The proposed idea is
great and it looks like the patch needs only a minor improvement.
--
Anastasia Lubennikova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
On Mon, 30 Nov 2020, 20:38 Anastasia Lubennikova, <
a.lubennikova@postgrespro.ru> wrote:
On 27.10.2020 11:34, Peter Eisentraut wrote:
On 2020-09-25 09:40, Craig Ringer wrote:
While working on extensions I've often wanted to enable cache
clobbering for a targeted piece of code, without paying the price of
running it for all backends during postgres startup and any initial
setup tasks that are required.So here's a patch that, when CLOBBER_CACHE_ALWAYS or
CLOBBER_CACHE_RECURSIVE are defined, adds a GUC named
clobber_cache_depth . It defaults to 1 for CLOBBER_CACHE_ALWAYS or 3
for CLOBBER_CACHE_RECURSIVE to match the existing compiled-in
behaviour. But with this change it's now possible to run Pg with
clobber_cache_depth=0 then set it to 1 only for targeted tests.clobber_cache_depth is treated as an unknown GUC if Pg was not built
with CLOBBER_CACHE_ALWAYS or CLOBBER_CACHE_RECURSIVE defined.I think it would be great if the cache clobber code is always compiled
in (but turned off) when building with assertions. The would reduce
the number of hoops to jump through to actually use this locally and
reduce the number of surprises from the build farm.For backward compatibility, you can arrange it so that the built-in
default of clobber_cache_depth is 1 or 3 if CLOBBER_CACHE_ALWAYS or
CLOBBER_CACHE_RECURSIVE are defined.Status update for a commitfest entry.
This thread was inactive during the commitfest, so I am going to mark it
as "Returned with Feedback".
Craig, are you planning to continue working on it? The proposed idea is
great and it looks like the patch needs only a minor improvement.
Thanks.
I'll update it soon.
On Tue, 27 Oct 2020 at 16:34, Peter Eisentraut <
peter.eisentraut@2ndquadrant.com> wrote:
On 2020-09-25 09:40, Craig Ringer wrote:
While working on extensions I've often wanted to enable cache clobbering
for a targeted piece of code, without paying the price of running it for
all backends during postgres startup and any initial setup tasks that
are required.So here's a patch that, when CLOBBER_CACHE_ALWAYS or
CLOBBER_CACHE_RECURSIVE are defined, adds a GUC named
clobber_cache_depth . It defaults to 1 for CLOBBER_CACHE_ALWAYS or 3 for
CLOBBER_CACHE_RECURSIVE to match the existing compiled-in behaviour. But
with this change it's now possible to run Pg with clobber_cache_depth=0
then set it to 1 only for targeted tests.I think it would be great if the cache clobber code is always compiled
in (but turned off) when building with assertions. The would reduce the
number of hoops to jump through to actually use this locally and reduce
the number of surprises from the build farm.
I implemented something a bit like that for a patched postgres where it
became an additional configure option. It's easy enough to enable it
automatically for USING_CASSERT instead, and I think that's sensible, so
I've adapted the patch to do so.
As initially written the patch defined the clobber control GUC only if
cache clobber control is compiled in. But we don't do that for anything
else, so I've changed it to always define the GUC, which will be ignored
without effect if no cache clobber control is compiled in. I also renamed
the GUC to debug_clobber_cache_depth to match other debug GUCs.
For backward compatibility, you can arrange it so that the built-in
default of clobber_cache_depth is 1 or 3 if CLOBBER_CACHE_ALWAYS or
CLOBBER_CACHE_RECURSIVE are defined.
It already does that - see diff hunk for src/include/utils/inval.h in prior
patch.
I've just changed it to default to 0 if neither are defined and moved it to
pg_config_manual.h .
I noticed that I missed handling of RECOVER_RELATION_BUILD_MEMORY in the
earlier patch - the relcache was eagerly freeing relation descriptor memory
without regard for the current clobber_cache_depth value. I've changed that
in the updated patch so that RelationBuildDesc only frees memory eagerly if
clobber_cache_depth is actually > 0 or if RECOVER_RELATION_BUILD_MEMORY has
been explicitly defined to 1. It also preserves the original behaviour of
not eagerly freeing memory even when cache clobber is enabled if
RECOVER_RELATION_BUILD_MEMORY is explicitly defined to 0.
Both CLOBBER_CACHE_ENABLED and RECOVER_RELATION_BUILD_MEMORY are now shown
in pg_config_manual.h .
A small entry has been added to the docs too, under developer options.
The changes add a little more noise than I'd prefer, but I didn't want to
vanish support for the compile-time constants or introduce surprising
behaviour.
To try it out, apply the patch (git am), build with --enable-cassert, then
compare:
make -C src/test/regress check
and
PGOPTIONS="-c debug_clobber_cache_depth=1" \
make -C src/test/regress check
The speed difference will be obvious if nothing else!
It's much more practical using make check-tests and a specific TESTS= list.
--
Craig Ringer http://www.2ndQuadrant.com/
2ndQuadrant - PostgreSQL Solutions for the Enterprise
Attachments:
v2-0001-Replace-CLOBBER_CACHE_ALWAYS-with-debug_clobber_c.patchtext/x-patch; charset=US-ASCII; name=v2-0001-Replace-CLOBBER_CACHE_ALWAYS-with-debug_clobber_c.patchDownload
From fed26086ca2d5840ac288846b7b81b0d74af6949 Mon Sep 17 00:00:00 2001
From: Craig Ringer <craig.ringer@2ndquadrant.com>
Date: Tue, 22 Sep 2020 09:51:00 +0800
Subject: [PATCH v2] Replace CLOBBER_CACHE_ALWAYS with
debug_clobber_cache_depth GUC
Forced cache invalidation (CLOBBER_CACHE_ALWAYS) has been impractical to use
for testing in PostgreSQL because it's so slow and because it's toggled on/off
only at build time. It is helpful when hunting bugs in any code that uses the
sycache/relcache because causes cache invalidations to be injected whenever it
would be possible for an invalidation to occur, whether or not one was really
pending.
Address this by providing runtime control over cache clobber behaviour using
the new debug_clobber_cache_depth GUC. Support is not compiled in at all
unless assertions are enabled or CLOBBER_CACHE_ENABLED is explicitly defined at
compile time. It defaults to 0 if compiled in, so it has negligible effect
on assert build performance by default.
When support is compiled in, test code can now set debug_clobber_cache_depth=1
locally to a backend to test specific queries, functions, extensions, etc. Or
tests can toggle it globally for a specific test case while retaining normal
performance during test setup and teardown.
For backwards compatibility with existing test harnesses and scripts,
clobber_cache_depth defaults to 1 if CLOBBER_CACHE_ALWAYS is defined, and to 3
if CLOBBER_CACHE_RECURSIVE is defined.
CLOBBER_CACHE_ENABLED is now visible in pg_config_manual.h, as is the related
RECOVER_RELATION_BUILD_MEMORY setting for the relcache.
---
doc/src/sgml/config.sgml | 34 ++++++++++++++
doc/src/sgml/regress.sgml | 19 ++++++++
src/backend/utils/adt/lockfuncs.c | 2 +-
src/backend/utils/cache/inval.c | 57 +++++++++++++-----------
src/backend/utils/cache/plancache.c | 5 ++-
src/backend/utils/cache/relcache.c | 69 ++++++++++++++++++-----------
src/backend/utils/misc/guc.c | 27 +++++++++++
src/include/pg_config_manual.h | 68 +++++++++++++++++++++++++++-
src/include/utils/inval.h | 1 +
9 files changed, 225 insertions(+), 57 deletions(-)
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index f810789ea8..93f8d73c44 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -10463,6 +10463,40 @@ LOG: CleanUpLock: deleting: lock(0xb7acd844) id(24688,24696,0,0,0,1)
</listitem>
</varlistentry>
+ <varlistentry id="guc-debug-clobber-cache-depth" xreflabel="debug_clobber_cache_depth">
+ <term><varname>debug_clobber_cache_depth</varname> (<type>integer</type>)
+ <indexterm>
+ <primary><varname>debug_cobber_cache_depth</varname> configuration parameter</primary>
+ </indexterm>
+ </term>
+ <listitem>
+ <para>
+ This advanced developer setting for cache invalidation debugging is not
+ supported in production PostgreSQL builds. Its value will always be
+ <literal>0</literal> on production builds and attempts to set it will
+ raise an error.
+ </para>
+ <para>
+ It is supported but off by default (0) on assert-enabled builds
+ (<xref linkend="guc-debug-assertions"/>) or when <symbol>CLOBBER_CACHE_ENABLED</symbol>
+ was defined at compile time.
+ </para>
+ <para>
+ When set to 1, each cache lookup for a PostgreSQL system catalog entry
+ is invalidated at the first possible opportunity, irrespective of
+ whether anything that would render it invalid really occurred. Caching
+ of PostgreSQL system catalogs is effectively disabled as a result, so
+ the server will run extremely slowly. This option can be very helful when
+ trying to trigger hard-to-reproduce bugs involving concurrency and catalog
+ changes but is otherwise rarely needed.
+ </para>
+ <para>
+ See the source code files <filename>inval.c</filename> and
+ <filename>pg_config_manual.h</filename> for details.
+ </para>
+ </listitem>
+ </varlistentry>
+
</variablelist>
</sect1>
<sect1 id="runtime-config-short">
diff --git a/doc/src/sgml/regress.sgml b/doc/src/sgml/regress.sgml
index 083d0bf46b..1603cf664f 100644
--- a/doc/src/sgml/regress.sgml
+++ b/doc/src/sgml/regress.sgml
@@ -91,6 +91,25 @@ make MAX_CONNECTIONS=10 check
</screen>
runs no more than ten tests concurrently.
</para>
+
+ <para>
+ Custom server settings to use when running a regression test suite may
+ be set by supplying a pre-written <filename>postgresql.conf</filename>:
+<screen>
+# Using a config file:
+echo 'log_checkpoints = on' > test_postgresql.conf
+echo 'work_mem = 50MB' >> test_postgresql.conf
+make EXTRA_REGRESS_OPTS="--temp-config=test_postgresql.conf" check
+</screen>
+ or by setting them in the <varname>PGOPTIONS</varname>
+ environment variable:
+<screen>
+PGOPTIONS="-c log_checkpoints=on -c work_mem=50MB" make check
+</screen>
+ This can be useful to enable additional logging, adjust resource limits,
+ or enable extra runtime checks like <xref linkend="guc-debug-clobber-cache-depth"/>.
+ </para>
+
</sect2>
<sect2>
diff --git a/src/backend/utils/adt/lockfuncs.c b/src/backend/utils/adt/lockfuncs.c
index f592292d06..b674c766de 100644
--- a/src/backend/utils/adt/lockfuncs.c
+++ b/src/backend/utils/adt/lockfuncs.c
@@ -629,7 +629,7 @@ pg_isolation_test_session_is_blocked(PG_FUNCTION_ARGS)
* Check if any of these are in the list of interesting PIDs, that being
* the sessions that the isolation tester is running. We don't use
* "arrayoverlaps" here, because it would lead to cache lookups and one of
- * our goals is to run quickly under CLOBBER_CACHE_ALWAYS. We expect
+ * our goals is to run quickly with debug_clobber_cache_depth > 0. We expect
* blocking_pids to be usually empty and otherwise a very small number in
* isolation tester cases, so make that the outer loop of a naive search
* for a match.
diff --git a/src/backend/utils/cache/inval.c b/src/backend/utils/cache/inval.c
index 628d6f5d0c..f424d99889 100644
--- a/src/backend/utils/cache/inval.c
+++ b/src/backend/utils/cache/inval.c
@@ -109,6 +109,7 @@
#include "storage/sinval.h"
#include "storage/smgr.h"
#include "utils/catcache.h"
+#include "utils/guc.h"
#include "utils/inval.h"
#include "utils/memdebug.h"
#include "utils/memutils.h"
@@ -179,6 +180,8 @@ static SharedInvalidationMessage *SharedInvalidMessagesArray;
static int numSharedInvalidMessagesArray;
static int maxSharedInvalidMessagesArray;
+/* GUC storage for debug_clobber_cache_depth */
+int debug_clobber_cache_depth = 0;
/*
* Dynamically-registered callback functions. Current implementation
@@ -689,35 +692,39 @@ AcceptInvalidationMessages(void)
/*
* Test code to force cache flushes anytime a flush could happen.
*
- * If used with CLOBBER_FREED_MEMORY, CLOBBER_CACHE_ALWAYS provides a
- * fairly thorough test that the system contains no cache-flush hazards.
- * However, it also makes the system unbelievably slow --- the regression
- * tests take about 100 times longer than normal.
- *
- * If you're a glutton for punishment, try CLOBBER_CACHE_RECURSIVELY. This
- * slows things by at least a factor of 10000, so I wouldn't suggest
- * trying to run the entire regression tests that way. It's useful to try
- * a few simple tests, to make sure that cache reload isn't subject to
- * internal cache-flush hazards, but after you've done a few thousand
- * recursive reloads it's unlikely you'll learn more.
+ * This helps detect intermittent faults caused by code that reads a
+ * cache entry then performs an action that could invalidate the entry,
+ * but rarely actually does so. This can spot issues that would otherwise
+ * only arise with badly timed concurrent DDL, for example.
+ *
+ * The default debug_clobber_cache_depth = 0 does no forced cache flushes.
+ *
+ * debug_clobber_cache_depth = 1 (CLOBBER_CACHE_ALWAYS) provides a fairly
+ * thorough test that the system contains no cache-flush hazards.
+ * However, it also makes the system unbelievably slow --- the
+ * regression tests take about 100 times longer than normal.
+ *
+ * If you're a glutton for punishment, try debug_clobber_cache_depth = 3
+ * (CLOBBER_CACHE_RECURSIVELY). This slows things by at least a factor
+ * of 10000, so I wouldn't suggest trying to run the entire regression
+ * tests that way. It's useful to try a few simple tests, to make sure
+ * that cache reload isn't subject to internal cache-flush hazards, but
+ * after you've done a few thousand recursive reloads it's unlikely
+ * you'll learn more.
+ *
+ * To make it practical to target specific suspect or changed
+ * code-paths the debug_clobber_cache_depth can be adjusted per-backend or
+ * globally like any other GUC. Just keep in mind that global GUC
+ * changes only propagate to existing backends next time they check
+ * ConfigReloadPending() so the effect of global changes won't be
+ * instant.
*/
-#if defined(CLOBBER_CACHE_ALWAYS)
- {
- static bool in_recursion = false;
-
- if (!in_recursion)
- {
- in_recursion = true;
- InvalidateSystemCaches();
- in_recursion = false;
- }
- }
-#elif defined(CLOBBER_CACHE_RECURSIVELY)
+#if defined(CLOBBER_CACHE_ENABLED)
+ if (debug_clobber_cache_depth > 0)
{
static int recursion_depth = 0;
- /* Maximum depth is arbitrary depending on your threshold of pain */
- if (recursion_depth < 3)
+ if (recursion_depth < debug_clobber_cache_depth)
{
recursion_depth++;
InvalidateSystemCaches();
diff --git a/src/backend/utils/cache/plancache.c b/src/backend/utils/cache/plancache.c
index 50d6ad28b4..5c217097c4 100644
--- a/src/backend/utils/cache/plancache.c
+++ b/src/backend/utils/cache/plancache.c
@@ -897,8 +897,9 @@ BuildCachedPlan(CachedPlanSource *plansource, List *qlist,
* rejected a generic plan, it's possible to reach here with is_valid
* false due to an invalidation while making the generic plan. In theory
* the invalidation must be a false positive, perhaps a consequence of an
- * sinval reset event or the CLOBBER_CACHE_ALWAYS debug code. But for
- * safety, let's treat it as real and redo the RevalidateCachedQuery call.
+ * sinval reset event or the debug_clobber_cache_depth > 0
+ * (CLOBBER_CACHE_ALWAYS) debug code. But for safety, let's treat it
+ * as real and redo the RevalidateCachedQuery call.
*/
if (!plansource->is_valid)
qlist = RevalidateCachedQuery(plansource, queryEnv);
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index 66393becfb..50de139fb3 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -91,15 +91,17 @@
#define RELCACHE_INIT_FILEMAGIC 0x573266 /* version ID value */
/*
- * Default policy for whether to apply RECOVER_RELATION_BUILD_MEMORY:
- * do so in clobber-cache builds but not otherwise. This choice can be
- * overridden at compile time with -DRECOVER_RELATION_BUILD_MEMORY=1 or =0.
+ * Whether to bother checking if relation cache memory needs to be freed
+ * eagerly. See RelationBuildDesc and pg_config_manual.h.
*/
-#ifndef RECOVER_RELATION_BUILD_MEMORY
-#if defined(CLOBBER_CACHE_ALWAYS) || defined(CLOBBER_CACHE_RECURSIVELY)
-#define RECOVER_RELATION_BUILD_MEMORY 1
+#if defined(RECOVER_RELATION_BUILD_MEMORY)
+#define CAN_RECOVER_RELATION_BUILD_MEMORY RECOVER_RELATION_BUILD_MEMORY
#else
#define RECOVER_RELATION_BUILD_MEMORY 0
+#if defined(CLOBBER_CACHE_ENABLED)
+#define CAN_RECOVER_RELATION_BUILD_MEMORY 1
+#else
+#define CAN_RECOVER_RELATION_BUILD_MEMORY 0
#endif
#endif
@@ -1040,19 +1042,25 @@ RelationBuildDesc(Oid targetRelId, bool insertIt)
* scope, and relcache loads shouldn't happen so often that it's essential
* to recover transient data before end of statement/transaction. However
* that's definitely not true in clobber-cache test builds, and perhaps
- * it's not true in other cases. If RECOVER_RELATION_BUILD_MEMORY is not
- * zero, arrange to allocate the junk in a temporary context that we'll
- * free before returning. Make it a child of caller's context so that it
- * will get cleaned up appropriately if we error out partway through.
+ * it's not true in other cases.
+ *
+ * When cache clobbering is enabled or when forced to by
+ * RECOVER_RELATION_BUILD_MEMORY=1, arrange to allocate the junk in a
+ * temporary context that we'll free before returning. Make it a child
+ * of caller's context so that it will get cleaned up appropriately if
+ * we error out partway through.
*/
-#if RECOVER_RELATION_BUILD_MEMORY
- MemoryContext tmpcxt;
- MemoryContext oldcxt;
+#if CAN_RECOVER_RELATION_BUILD_MEMORY
+ MemoryContext tmpcxt = NULL;
+ MemoryContext oldcxt = NULL;
- tmpcxt = AllocSetContextCreate(CurrentMemoryContext,
- "RelationBuildDesc workspace",
- ALLOCSET_DEFAULT_SIZES);
- oldcxt = MemoryContextSwitchTo(tmpcxt);
+ if (RECOVER_RELATION_BUILD_MEMORY || debug_clobber_cache_depth > 0)
+ {
+ tmpcxt = AllocSetContextCreate(CurrentMemoryContext,
+ "RelationBuildDesc workspace",
+ ALLOCSET_DEFAULT_SIZES);
+ oldcxt = MemoryContextSwitchTo(tmpcxt);
+ }
#endif
/*
@@ -1065,10 +1073,13 @@ RelationBuildDesc(Oid targetRelId, bool insertIt)
*/
if (!HeapTupleIsValid(pg_class_tuple))
{
-#if RECOVER_RELATION_BUILD_MEMORY
- /* Return to caller's context, and blow away the temporary context */
- MemoryContextSwitchTo(oldcxt);
- MemoryContextDelete(tmpcxt);
+#if CAN_RECOVER_RELATION_BUILD_MEMORY
+ if (tmpcxt != NULL)
+ {
+ /* Return to caller's context, and blow away the temporary context */
+ MemoryContextSwitchTo(oldcxt);
+ MemoryContextDelete(tmpcxt);
+ }
#endif
return NULL;
}
@@ -1247,10 +1258,13 @@ RelationBuildDesc(Oid targetRelId, bool insertIt)
/* It's fully valid */
relation->rd_isvalid = true;
-#if RECOVER_RELATION_BUILD_MEMORY
- /* Return to caller's context, and blow away the temporary context */
- MemoryContextSwitchTo(oldcxt);
- MemoryContextDelete(tmpcxt);
+#if CAN_RECOVER_RELATION_BUILD_MEMORY
+ if (tmpcxt != NULL)
+ {
+ /* Return to caller's context, and blow away the temporary context */
+ MemoryContextSwitchTo(oldcxt);
+ MemoryContextDelete(tmpcxt);
+ }
#endif
return relation;
@@ -1647,8 +1661,9 @@ LookupOpclassInfo(Oid operatorClassOid,
* while we are loading the info, and it's very hard to provoke that if
* this happens only once per opclass per backend.
*/
-#if defined(CLOBBER_CACHE_ALWAYS)
- opcentry->valid = false;
+#if defined(CLOBBER_CACHE_ENABLED)
+ if (debug_clobber_cache_depth > 0)
+ opcentry->valid = false;
#endif
if (opcentry->valid)
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 245a3472bc..19caebc6cc 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -99,6 +99,7 @@
#include "utils/rls.h"
#include "utils/snapmgr.h"
#include "utils/tzparser.h"
+#include "utils/inval.h"
#include "utils/varlena.h"
#include "utils/xml.h"
@@ -225,6 +226,7 @@ static bool check_recovery_target_lsn(char **newval, void **extra, GucSource sou
static void assign_recovery_target_lsn(const char *newval, void *extra);
static bool check_primary_slot_name(char **newval, void **extra, GucSource source);
static bool check_default_with_oids(bool *newval, void **extra, GucSource source);
+static bool check_debug_clobber_cache_depth(int *newval, void **extra, GucSource source);
/* Private functions in guc-file.l that need to be called from guc.c */
static ConfigVariable *ProcessConfigFileInternal(GucContext context,
@@ -3399,6 +3401,17 @@ static struct config_int ConfigureNamesInt[] =
check_huge_page_size, NULL, NULL
},
+ {
+ {"debug_clobber_cache_depth", PGC_SUSET, DEVELOPER_OPTIONS,
+ gettext_noop("Auto-flush catalog caches for debugging purposes."),
+ NULL,
+ GUC_NOT_IN_SAMPLE
+ },
+ &debug_clobber_cache_depth,
+ DEFAULT_CLOBBER_CACHE_DEPTH, 0, 5,
+ check_debug_clobber_cache_depth, NULL, NULL
+ },
+
/* End-of-list marker */
{
{NULL, 0, 0, NULL, NULL}, NULL, 0, 0, 0, NULL, NULL, NULL
@@ -12135,4 +12148,18 @@ check_default_with_oids(bool *newval, void **extra, GucSource source)
return true;
}
+static bool
+check_debug_clobber_cache_depth(int *newval, void **extra, GucSource source)
+{
+#ifndef CLOBBER_CACHE_ENABLED
+ if (*newval)
+ {
+ GUC_check_errcode(ERRCODE_FEATURE_NOT_SUPPORTED);
+ GUC_check_errmsg("debug_clobber_cache_depth support was not enabled at compile time");
+ return false;
+ }
+#endif
+ return true;
+}
+
#include "guc-file.c"
diff --git a/src/include/pg_config_manual.h b/src/include/pg_config_manual.h
index 705dc69c06..3b3d43519b 100644
--- a/src/include/pg_config_manual.h
+++ b/src/include/pg_config_manual.h
@@ -6,8 +6,15 @@
* for developers. If you edit any of these, be sure to do a *full*
* rebuild (and an initdb if noted).
*
+ * You can define these by editing pg_config_manual.h but it's usually
+ * sufficient to pass CFLAGS to configure, e.g.
+ *
+ * ./configure --enable-debug --enable-cassert CFLAGS="-DUSE_VALGRIND"
+ *
+ * The flags are persisted in Makefile.global so they will be correctly
+ * applied to extensions, including those built by PGXS.
+ *
* Portions Copyright (c) 1996-2020, PostgreSQL Global Development Group
- * Portions Copyright (c) 1994, Regents of the University of California
*
* src/include/pg_config_manual.h
*------------------------------------------------------------------------
@@ -280,7 +287,8 @@
* percentage points even when not run under Valgrind.
*
* You should normally use MEMORY_CONTEXT_CHECKING with USE_VALGRIND;
- * instrumentation of repalloc() is inferior without it.
+ * instrumentation of repalloc() is inferior without it. So it's defined
+ * automatically for USE_VALGRIND builds.
*/
/* #define USE_VALGRIND */
@@ -309,6 +317,62 @@
*/
/* #define RANDOMIZE_ALLOCATED_MEMORY */
+/*
+ * For cache invalidation debugging purposes define CLOBBER_CACHE_ENABLED to
+ * enable use of the debug_clobber_cache_depth GUC to aggressively flush
+ * syscache/relcache entries whenever it's possible to deliver invalidations.
+ * See AcceptInvalidationMessages() in src/backend/utils/cache/inval.c for
+ * details.
+ *
+ * USE_ASSERT_CHECKING builds default to enabling the use of
+ * debug_clobber_cache_depth but set it to 0 by default unless overridden by
+ * DEFAULT_CLOBBER_CACHE_DEPTH or the backwards-compatibility macros defined
+ * below.
+ *
+ * It's possible to use CLOBBER_CACHE_ENABLED without a cassert build and the
+ * implied CLOBBER_FREED_MEMORY and MEMORY_CONTEXT_CHECKING options but it's
+ * unlikely to be as effective at identifying problems. Just use an assert
+ * build.
+ */
+/* #define CLOBBER_CACHE_ENABLED */
+
+#if defined(USE_ASSERT_CHECKING) && !defined(CLOBBER_CACHE_ENABLED)
+#define CLOBBER_CACHE_ENABLED
+#endif
+
+/*
+ * Backwards compatibility for the older compile-time-only
+ * cache clobber macros.
+ */
+#if defined(CLOBBER_CACHE_RECURSIVELY) && !defined(CLOBBER_CACHE_ALWAYS)
+#define CLOBBER_CACHE_ALWAYS
+#endif
+#if defined(CLOBBER_CACHE_ALWAYS) && !defined(CLOBBER_CACHE_ENABLED)
+#define CLOBBER_CACHE_ENABLED
+#endif
+#if defined(CLOBBER_CACHE_RECURSIVELY) && !defined(DEFAULT_CLOBBER_CACHE_DEPTH)
+#define DEFAULT_CLOBBER_CACHE_DEPTH 3
+#endif
+#if defined(CLOBBER_CACHE_ALWAYS) && !defined(DEFAULT_CLOBBER_CACHE_DEPTH)
+#define DEFAULT_CLOBBER_CACHE_DEPTH 1
+#endif
+#ifndef DEFAULT_CLOBBER_CACHE_DEPTH
+#define DEFAULT_CLOBBER_CACHE_DEPTH 0
+#endif
+
+/*
+ * Recover memory used for relcache entries when invalidated. See
+ * RelationBuildDescr in src/backend/utils/cache/relcache.c .
+ *
+ * This is active automatically for clobber cache builds when clobbering is
+ * active, but can be overridden here by explicitly defining
+ * RECOVER_RELATION_BUILD_MEMORY. Define to 1 to always free relation cache
+ * memory even when clobber is off, or to 0 to never free relation cache memory
+ * even when clobbering is on.
+ */
+/* #define RECOVER_RELATION_BUILD_MEMORY 0 */ /* Force disable */
+/* #define RECOVER_RELATION_BUILD_MEMORY 1 */ /* Force enable */
+
/*
* Define this to force all parse and plan trees to be passed through
* copyObject(), to facilitate catching errors and omissions in
diff --git a/src/include/utils/inval.h b/src/include/utils/inval.h
index 463888c389..689f24ad8f 100644
--- a/src/include/utils/inval.h
+++ b/src/include/utils/inval.h
@@ -18,6 +18,7 @@
#include "storage/relfilenode.h"
#include "utils/relcache.h"
+extern PGDLLIMPORT int debug_clobber_cache_depth;
typedef void (*SyscacheCallbackFunction) (Datum arg, int cacheid, uint32 hashvalue);
typedef void (*RelcacheCallbackFunction) (Datum arg, Oid relid);
--
2.26.2
On Thu, 3 Dec 2020 at 15:53, Craig Ringer <craig@2ndquadrant.com> wrote:
On Tue, 27 Oct 2020 at 16:34, Peter Eisentraut <
peter.eisentraut@2ndquadrant.com> wrote:On 2020-09-25 09:40, Craig Ringer wrote:
While working on extensions I've often wanted to enable cache
clobbering
for a targeted piece of code, without paying the price of running it
for
all backends during postgres startup and any initial setup tasks that
are required.So here's a patch that, when CLOBBER_CACHE_ALWAYS or
CLOBBER_CACHE_RECURSIVE are defined, adds a GUC named
clobber_cache_depth . It defaults to 1 for CLOBBER_CACHE_ALWAYS or 3for
CLOBBER_CACHE_RECURSIVE to match the existing compiled-in behaviour.
But
with this change it's now possible to run Pg with clobber_cache_depth=0
then set it to 1 only for targeted tests.I think it would be great if the cache clobber code is always compiled
in (but turned off) when building with assertions. The would reduce the
number of hoops to jump through to actually use this locally and reduce
the number of surprises from the build farm.
Updated per your comments Peter, see mail immediately up-thread.
Moved to this CF as https://commitfest.postgresql.org/31/2749/ .
On 2020-12-03 07:01, Craig Ringer wrote:
To try it out, apply the patch (git am), build with --enable-cassert,
then compare:make -C src/test/regress check
and
PGOPTIONS="-c debug_clobber_cache_depth=1" \
make -C src/test/regress checkThe speed difference will be obvious if nothing else!
This is a really useful feature change. I have a version that I'm happy
to commit, but I wanted to check on the name of the setting. The
proposed name arose during the discussion when it was just to set the
recursion depth but not enable the feature altogether, so I think that
name is a bit misleading now. We could reuse the old macro name, as in
clobber_cache_always=N, which is very recognizable. But the feature
itself doesn't clobber anything (that's done by CLOBBER_FREED_MEMORY),
so most accurate would be something like
invalidate_system_caches_always=N. Thoughts?
--
Peter Eisentraut
2ndQuadrant, an EDB company
https://www.2ndquadrant.com/
On Tue, 5 Jan 2021, 22:41 Peter Eisentraut, <
peter.eisentraut@2ndquadrant.com> wrote:
On 2020-12-03 07:01, Craig Ringer wrote:
To try it out, apply the patch (git am), build with --enable-cassert,
then compare:make -C src/test/regress check
and
PGOPTIONS="-c debug_clobber_cache_depth=1" \
make -C src/test/regress checkThe speed difference will be obvious if nothing else!
This is a really useful feature change. I have a version that I'm happy
to commit, but I wanted to check on the name of the setting. The
proposed name arose during the discussion when it was just to set the
recursion depth but not enable the feature altogether, so I think that
name is a bit misleading now. We could reuse the old macro name, as in
clobber_cache_always=N, which is very recognizable. But the feature
itself doesn't clobber anything (that's done by CLOBBER_FREED_MEMORY),
so most accurate would be something like
invalidate_system_caches_always=N. Thoughts?
Modulo typo, I think that's a better name.
Perhaps debug_invalidate_system_caches_always ? It's a bit long but we use
the debug prefix elsewhere too.
Show quoted text
--
Peter Eisentraut
2ndQuadrant, an EDB company
https://www.2ndquadrant.com/
On 2021-01-06 00:30, Craig Ringer wrote:
Perhaps debug_invalidate_system_caches_always ? It's a bit long but we
use the debug prefix elsewhere too.
Committed with that name.
In your original patch, this hunk in pg_config_manual.h:
+ * You can define these by editing pg_config_manual.h but it's usually
+ * sufficient to pass CFLAGS to configure, e.g.
+ *
+ * ./configure --enable-debug --enable-cassert CFLAGS="-DUSE_VALGRIND"
+ *
+ * The flags are persisted in Makefile.global so they will be correctly
+ * applied to extensions, including those built by PGXS.I don't think that necessarily works for all settings. Maybe we should
make a pass through it and ensure it all works sensibly, but that seems
like a separate project.
--
Peter Eisentraut
2ndQuadrant, an EDB company
https://www.2ndquadrant.com/
On Wed, 6 Jan 2021 at 18:23, Peter Eisentraut <
peter.eisentraut@2ndquadrant.com> wrote:
On 2021-01-06 00:30, Craig Ringer wrote:
Perhaps debug_invalidate_system_caches_always ? It's a bit long but we
use the debug prefix elsewhere too.Committed with that name.
Thanks very much.
In your original patch, this hunk in pg_config_manual.h:
+ * You can define these by editing pg_config_manual.h but it's usually + * sufficient to pass CFLAGS to configure, e.g. + * + * ./configure --enable-debug --enable-cassert CFLAGS="-DUSE_VALGRIND" + * + * The flags are persisted in Makefile.global so they will be correctly + * applied to extensions, including those built by PGXS.I don't think that necessarily works for all settings. Maybe we should
make a pass through it and ensure it all works sensibly, but that seems
like a separate project.
It should work for everything, since we persist the CFLAGS string, rather
than individual settings.
But I didn't mean to leave that in the same patch anyway, it's separate.