Thread: [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.
Attachment
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.
Attachment
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 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.
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 check > > The 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 check
>
> The 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.
--
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.