Thread: Handle TEMP_CONFIG for pg_regress style tests in pg_regress.c

Handle TEMP_CONFIG for pg_regress style tests in pg_regress.c

From
Andres Freund
Date:
Hi,

When building with meson, TEMP_CONFIG is supported for TAP tests, but doesn't
do anything for regress/isolation.

The reason for that is that meson's (and ninja's) architecture is to separate
"build setup" from the "build/test/whatever" stage, moving dynamism (and more
costly operations) to the "setup" phase.

In this case the implication is that the command line for the test isn't
re-computed dynamically. But pg_regress doesn't look at TEMP_CONFIG, it just
has a --temp-config=... parameter, that src/Makefile.global.in dynamically
adds if TEMP_CONFIG is set.

In contrast to that, TEMP_CONFIG support for tap tests is implemented in
Cluster.pm, and thus works transparently.

My inclination is to move TEMP_CONFIG support from the Makefile to
pg_regress.c. That way it's consistent across the build tools and isn't
duplicated. pg_regress already looks at a bunch of temporary variables
(e.g. PG_REGRESS_SOCK_DIR, PG_TEST_USE_UNIX_SOCKETS), so this isn't really
breaking new ground.

It can be implemented differently, e.g. by adding the parameter dynamically in
the wrapper around pg_regress, but I don't see an advantage in that.

Patch attached.

Greetings,

Andres Freund

Attachment

Re: Handle TEMP_CONFIG for pg_regress style tests in pg_regress.c

From
Peter Eisentraut
Date:
On 18.02.23 21:26, Andres Freund wrote:
> When building with meson, TEMP_CONFIG is supported for TAP tests, but doesn't
> do anything for regress/isolation.
> 
> The reason for that is that meson's (and ninja's) architecture is to separate
> "build setup" from the "build/test/whatever" stage, moving dynamism (and more
> costly operations) to the "setup" phase.
> 
> In this case the implication is that the command line for the test isn't
> re-computed dynamically. But pg_regress doesn't look at TEMP_CONFIG, it just
> has a --temp-config=... parameter, that src/Makefile.global.in dynamically
> adds if TEMP_CONFIG is set.
> 
> In contrast to that, TEMP_CONFIG support for tap tests is implemented in
> Cluster.pm, and thus works transparently.
> 
> My inclination is to move TEMP_CONFIG support from the Makefile to
> pg_regress.c. That way it's consistent across the build tools and isn't
> duplicated. pg_regress already looks at a bunch of temporary variables
> (e.g. PG_REGRESS_SOCK_DIR, PG_TEST_USE_UNIX_SOCKETS), so this isn't really
> breaking new ground.

I'm having a hard time understanding what TEMP_CONFIG is for.  It 
appears that the intention is to allow injecting arbitrary configuration 
into the tests?  In that case, I think your proposal makes sense.  But I 
don't see this documented, so who knows what it is actually used for.




Re: Handle TEMP_CONFIG for pg_regress style tests in pg_regress.c

From
Andrew Dunstan
Date:


On 2023-02-19 Su 02:25, Peter Eisentraut wrote:
On 18.02.23 21:26, Andres Freund wrote:
When building with meson, TEMP_CONFIG is supported for TAP tests, but doesn't
do anything for regress/isolation.

The reason for that is that meson's (and ninja's) architecture is to separate
"build setup" from the "build/test/whatever" stage, moving dynamism (and more
costly operations) to the "setup" phase.

In this case the implication is that the command line for the test isn't
re-computed dynamically. But pg_regress doesn't look at TEMP_CONFIG, it just
has a --temp-config=... parameter, that src/Makefile.global.in dynamically
adds if TEMP_CONFIG is set.

In contrast to that, TEMP_CONFIG support for tap tests is implemented in
Cluster.pm, and thus works transparently.

My inclination is to move TEMP_CONFIG support from the Makefile to
pg_regress.c. That way it's consistent across the build tools and isn't
duplicated. pg_regress already looks at a bunch of temporary variables
(e.g. PG_REGRESS_SOCK_DIR, PG_TEST_USE_UNIX_SOCKETS), so this isn't really
breaking new ground.

I'm having a hard time understanding what TEMP_CONFIG is for.  It appears that the intention is to allow injecting arbitrary configuration into the tests?  In that case, I think your proposal makes sense.  But I don't see this documented, so who knows what it is actually used for.




It started here quite a long time ago:


commit 0cb74d3cec
Author: Andrew Dunstan <andrew@dunslane.net>
Date:   Sun Sep 9 20:40:54 2007 +0000

    Provide for a file specifying non-standard config options for temp install
    for pg_regress, via --temp-config option. Pick this up in the make file
    via TEMP_CONFIG setting.

It's used by the buildfarm to add the extra config settings from its configuration file.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: Handle TEMP_CONFIG for pg_regress style tests in pg_regress.c

From
Tom Lane
Date:
Andrew Dunstan <andrew@dunslane.net> writes:
> On 2023-02-19 Su 02:25, Peter Eisentraut wrote:
>> On 18.02.23 21:26, Andres Freund wrote:
>>> My inclination is to move TEMP_CONFIG support from the Makefile to
>>> pg_regress.c. That way it's consistent across the build tools and isn't
>>> duplicated.

>> I'm having a hard time understanding what TEMP_CONFIG is for.

> It's used by the buildfarm to add the extra config settings from its 
> configuration file.

I have also used it manually to inject configuration changes into
TAP tests, for instance running them with debug_discard_caches = 1.
It's quite handy, but I agree the lack of documentation is bad.

It looks to me like pg_regress already does implement this; that
is, the Makefiles convert TEMP_CONFIG into a --temp-config switch
to pg_[isolation_]regress.  So if we made pg_regress responsible
for examining the envvar directly, very little new code would be
needed.  (Maybe net negative code if we remove the command line
switch, but I'm not sure if we should.)  What we'd lose is the
ability to write
    make TEMP_CONFIG=foo check
but I wouldn't miss that.  Having a uniform rule that TEMP_CONFIG
is an environment variable and nothing else seems good.

            regards, tom lane



Re: Handle TEMP_CONFIG for pg_regress style tests in pg_regress.c

From
Andres Freund
Date:
Hi,

On 2023-02-19 11:13:38 -0500, Tom Lane wrote:
> Andrew Dunstan <andrew@dunslane.net> writes:
> > On 2023-02-19 Su 02:25, Peter Eisentraut wrote:
> >> On 18.02.23 21:26, Andres Freund wrote:
> >>> My inclination is to move TEMP_CONFIG support from the Makefile to
> >>> pg_regress.c. That way it's consistent across the build tools and isn't
> >>> duplicated.
> 
> >> I'm having a hard time understanding what TEMP_CONFIG is for.
> 
> > It's used by the buildfarm to add the extra config settings from its 
> > configuration file.
> 
> I have also used it manually to inject configuration changes into
> TAP tests, for instance running them with debug_discard_caches = 1.

Similar. Explicitly turning on fsync, changing the log level to debug etc.


> It's quite handy, but I agree the lack of documentation is bad.

We have some minimal documentation for EXTRA_REGRESS_OPTS, but imo that's a
bit of a different use case, as it adds actual commandline options for
pg_regress (and thus doesn't work for tap tests).

Seems we'd need a section in regress.sgml documenting the various environment
variables?


> It looks to me like pg_regress already does implement this; that
> is, the Makefiles convert TEMP_CONFIG into a --temp-config switch
> to pg_[isolation_]regress.  So if we made pg_regress responsible
> for examining the envvar directly, very little new code would be
> needed.

It's very little, indeed - the patch upthread ends up with:
 4 files changed, 11 insertions(+), 16 deletions(-)


> (Maybe net negative code if we remove the command line
> switch, but I'm not sure if we should.)

I don't think we should - we use it for various regression tests, to specify a
config file they should load (shared_preload_libraries, wal_level, etc).

The way I implemented it now is that TEMP_CONFIG is added earlier in the
resulting config file, than the contents of the file explicitly specified on
the commandline.


> What we'd lose is the ability to write make TEMP_CONFIG=foo check but I
> wouldn't miss that.  Having a uniform rule that TEMP_CONFIG is an
> environment variable and nothing else seems good.

If we were concerned about it we could just add an export of TEMP_CONFIG to
src/Makefile.global.in

Greetings,

Andres Freund