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

Hi,

On 2023-02-19 11:13:38 -0500, Tom Lane wrote:
> Andrew Dunstan <andrew(at)dunslane(dot)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