Horton

Thread

Commits

  1. Replace CLOBBER_CACHE_ALWAYS with run-time GUC

  2. doc: Document how to run regression tests with custom server settings

  1. [PATCH] Runtime control of CLOBBER_CACHE_ALWAYS

    Craig Ringer <craig@2ndquadrant.com> — 2020-09-25T07:40:17Z

    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

  2. Re: [PATCH] Runtime control of CLOBBER_CACHE_ALWAYS

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2020-10-27T08:34:48Z

    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

  3. Re: [PATCH] Runtime control of CLOBBER_CACHE_ALWAYS

    Anastasia Lubennikova <a.lubennikova@postgrespro.ru> — 2020-11-30T12:38:37Z

    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

  4. Re: [PATCH] Runtime control of CLOBBER_CACHE_ALWAYS

    Craig Ringer <craig.ringer@enterprisedb.com> — 2020-12-01T08:50:55Z

    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.

  5. Re: [PATCH] Runtime control of CLOBBER_CACHE_ALWAYS

    Craig Ringer <craig@2ndquadrant.com> — 2020-12-03T06:01:16Z

    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

  6. Re: [PATCH] Runtime control of CLOBBER_CACHE_ALWAYS

    Craig Ringer <craig.ringer@enterprisedb.com> — 2020-12-10T03:00:42Z

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

  7. Re: [PATCH] Runtime control of CLOBBER_CACHE_ALWAYS

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2021-01-05T14:40:56Z

    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/

  8. Re: [PATCH] Runtime control of CLOBBER_CACHE_ALWAYS

    Craig Ringer <craig.ringer@enterprisedb.com> — 2021-01-05T23:30:37Z

    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/
>
>
>

  9. Re: [PATCH] Runtime control of CLOBBER_CACHE_ALWAYS

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2021-01-06T10:23:45Z

    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/

  10. Re: [PATCH] Runtime control of CLOBBER_CACHE_ALWAYS

    Craig Ringer <craig.ringer@enterprisedb.com> — 2021-01-07T05:49:25Z

    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.