Thread: Handle TEMP_CONFIG for pg_regress style tests in pg_regress.c
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
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.
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
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
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