pgsql: Add TAP test to automate the equivalent of check_guc

Re: Michael Paquier

Add TAP test to automate the equivalent of check_guc

src/backend/utils/misc/check_guc is a script that cross-checks the
consistency of the GUCs with postgresql.conf.sample, making sure that

Hi,

this test is failing at Debian package compile time:

00:55:07 ok 18 - drop tablespace 2
00:55:07 ok 19 - drop tablespace 3
00:55:07 ok 20 - drop tablespace 4
00:55:07 ok
00:55:07 # Looks like your test exited with 2 before it could output anything.
00:55:09 t/003_check_guc.pl ..............
00:55:09 1..3
00:55:09 Dubious, test returned 2 (wstat 512, 0x200)
00:55:09 Failed 3/3 subtests
00:55:09
00:55:09 Test Summary Report
00:55:09 -------------------
00:55:09 t/003_check_guc.pl (Wstat: 512 Tests: 0 Failed: 0)
00:55:09 Non-zero exit status: 2
00:55:09 Parse errors: Bad plan. You planned 3 tests but ran 0.
00:55:09 Files=3, Tests=62, 6 wallclock secs ( 0.05 usr 0.01 sys + 3.31 cusr 1.35 csys = 4.72 CPU)
00:55:09 Result: FAIL
00:55:09 make[3]: *** [/<<PKGBUILDDIR>>/build/../src/makefiles/pgxs.mk:457: check] Error 1
00:55:09 make[3]: Leaving directory '/<<PKGBUILDDIR>>/build/src/test/modules/test_misc'

### Starting node "main"
# Running: pg_ctl -w -D /srv/projects/postgresql/pg/master/build/src/test/modules/test_misc/tmp_check/t_003_check_guc_main_data/pgdata -l /srv/projects/postgresql/pg/master/build/src/test/modules/test_misc/tmp_check/log/003_check_guc_main.log -o --cluster-name=main start
waiting for server to start.... done
server started
# Postmaster PID for node "main" is 1432398
Could not open /usr/share/postgresql/15/postgresql.conf.sample: No such file or directory at t/003_check_guc.pl line 47.
### Stopping node "main" using mode immediate

So it's trying to read from /usr/share/postgresql which doesn't exist
yet at build time.

The relevant part of the test is this:

# Find the location of postgresql.conf.sample, based on the information
# provided by pg_config.
my $sample_file =
$node->config_data('--sharedir') . '/postgresql.conf.sample';

It never caused any problem in the 12+ years we have been doing this,
but Debian is patching pg_config not to be relocatable since we are
installing into /usr/lib/postgresql/NN /usr/share/postgresql/NN, so
not a single prefix.

https://salsa.debian.org/postgresql/postgresql/-/blob/15/debian/patches/50-per-version-dirs.patch

Christoph

Christoph Berg <myon@debian.org> writes:

this test is failing at Debian package compile time:
Could not open /usr/share/postgresql/15/postgresql.conf.sample: No such file or directory at t/003_check_guc.pl line 47.

So it's trying to read from /usr/share/postgresql which doesn't exist
yet at build time.

The relevant part of the test is this:

# Find the location of postgresql.conf.sample, based on the information
# provided by pg_config.
my $sample_file =
$node->config_data('--sharedir') . '/postgresql.conf.sample';

This seems like a pretty bad idea even if it weren't failing outright.
We should be examining the version of the file that's in the source
tree; the one in the installation tree might have version-skew
problems, if you've not yet done "make install".

regards, tom lane

On Fri, Feb 11, 2022 at 09:59:55AM -0500, Tom Lane wrote:

Christoph Berg <myon@debian.org> writes:

this test is failing at Debian package compile time:
Could not open /usr/share/postgresql/15/postgresql.conf.sample: No such file or directory at t/003_check_guc.pl line 47.

So it's trying to read from /usr/share/postgresql which doesn't exist
yet at build time.

The relevant part of the test is this:

# Find the location of postgresql.conf.sample, based on the information
# provided by pg_config.
my $sample_file =
$node->config_data('--sharedir') . '/postgresql.conf.sample';

This seems like a pretty bad idea even if it weren't failing outright.
We should be examining the version of the file that's in the source
tree; the one in the installation tree might have version-skew
problems, if you've not yet done "make install".

My original way used the source tree, but Michael thought it would be an issue
for "installcheck" where the config may not be available.

/messages/by-id/YfTg/WHNLVVygy8v@paquier.xyz

This is what I had written:

-- test that GUCS are in postgresql.conf
SELECT lower(name) FROM tab_settings_flags WHERE NOT not_in_sample EXCEPT
SELECT regexp_replace(ln, '^#?([_[:alpha:]]+) (= .*|[^ ]*$)', '\1') AS guc
FROM (SELECT regexp_split_to_table(pg_read_file('postgresql.conf'), '\n') AS ln) conf
WHERE ln ~ '^#?[[:alpha:]]'
ORDER BY 1;
lower
-----------------------------
config_file
plpgsql.check_asserts
plpgsql.extra_errors
plpgsql.extra_warnings
plpgsql.print_strict_params
plpgsql.variable_conflict
(6 rows)

-- test that lines in postgresql.conf that look like GUCs are GUCs
SELECT regexp_replace(ln, '^#?([_[:alpha:]]+) (= .*|[^ ]*$)', '\1') AS guc
FROM (SELECT regexp_split_to_table(pg_read_file('postgresql.conf'), '\n') AS ln) conf
WHERE ln ~ '^#?[[:alpha:]]'
EXCEPT SELECT lower(name) FROM tab_settings_flags WHERE NOT not_in_sample
ORDER BY 1;
guc
-------------------
include
include_dir
include_if_exists
(3 rows)

--
Justin

Justin Pryzby <pryzby@telsasoft.com> writes:

On Fri, Feb 11, 2022 at 09:59:55AM -0500, Tom Lane wrote:

This seems like a pretty bad idea even if it weren't failing outright.
We should be examining the version of the file that's in the source
tree; the one in the installation tree might have version-skew
problems, if you've not yet done "make install".

My original way used the source tree, but Michael thought it would be an issue
for "installcheck" where the config may not be available.

Yeah, you are at risk either way, but in practice nobody is going to be
running these TAP tests without a source tree.

This is what I had written:
FROM (SELECT regexp_split_to_table(pg_read_file('postgresql.conf'), '\n') AS ln) conf

That's not using the source tree either, but the copy in the
cluster-under-test. I'd fear it to be unstable in the buildfarm, where
animals can append whatever they please to the config file being used by
tests.

regards, tom lane

On Fri, Feb 11, 2022 at 10:41:27AM -0500, Tom Lane wrote:

Justin Pryzby <pryzby@telsasoft.com> writes:

On Fri, Feb 11, 2022 at 09:59:55AM -0500, Tom Lane wrote:

This seems like a pretty bad idea even if it weren't failing outright.
We should be examining the version of the file that's in the source
tree; the one in the installation tree might have version-skew
problems, if you've not yet done "make install".

My original way used the source tree, but Michael thought it would be an issue
for "installcheck" where the config may not be available.

Yeah, you are at risk either way, but in practice nobody is going to be
running these TAP tests without a source tree.

This is what I had written:
FROM (SELECT regexp_split_to_table(pg_read_file('postgresql.conf'), '\n') AS ln) conf

That's not using the source tree either, but the copy in the
cluster-under-test. I'd fear it to be unstable in the buildfarm, where
animals can append whatever they please to the config file being used by
tests.

The important test is that "new GUCs exist in sample.conf". The converse test
is less interesting, so I think the important half would be okay.

I see the BF client appends to postgresql.conf.
https://github.com/PGBuildFarm/client-code/blob/main/run_build.pl#L1374

It could use "include", which was added in v8.2. Or it could add a marker,
like pg_regress does:
src/test/regress/pg_regress.c: fputs("\n# Configuration added by pg_regress\n\n", pg_conf);

If it were desirable to also check that "things that look like GUCs in the conf
are actually GUCs", either of those would avoid false positives.

Or, is it okay to use ABS_SRCDIR?

+\getenv abs_srcdir PG_ABS_SRCDIR
+\set filename :abs_srcdir '../../../../src/backend/utils/misc/postgresql.conf.sample'
+-- test that GUCS are in postgresql.conf
+SELECT lower(name) FROM tab_settings_flags WHERE NOT not_in_sample EXCEPT
+SELECT regexp_replace(ln, '^#?([_[:alpha:]]+) (= .*|[^ ]*$)', '\1') AS guc
+FROM (SELECT regexp_split_to_table(pg_read_file(:'filename'), '\n') AS ln) conf
+WHERE ln ~ '^#?[[:alpha:]]'
+ORDER BY 1;
+
+-- test that lines in postgresql.conf that look like GUCs are GUCs
+SELECT regexp_replace(ln, '^#?([_[:alpha:]]+) (= .*|[^ ]*$)', '\1') AS guc
+FROM (SELECT regexp_split_to_table(pg_read_file(:'filename'), '\n') AS ln) conf
+WHERE ln ~ '^#?[[:alpha:]]'
+EXCEPT SELECT lower(name) FROM tab_settings_flags WHERE NOT not_in_sample
+ORDER BY 1;

--
Justin

Justin Pryzby <pryzby@telsasoft.com> writes:

Or, is it okay to use ABS_SRCDIR?

Don't see why not --- other tests certainly do.

regards, tom lane

On Fri, Feb 11, 2022 at 10:48:11AM +0100, Christoph Berg wrote:

It never caused any problem in the 12+ years we have been doing this,
but Debian is patching pg_config not to be relocatable since we are
installing into /usr/lib/postgresql/NN /usr/share/postgresql/NN, so
not a single prefix.

https://salsa.debian.org/postgresql/postgresql/-/blob/15/debian/patches/50-per-version-dirs.patch

Wow. This is the way for Debian to bypass the fact that ./configure
is itself patched, hence you cannot rely on things like --libdir,
--bindir and the kind at build time? That's.. Err.. Fancy, I'd
say.
--
Michael

On Fri, Feb 11, 2022 at 12:34:31PM -0500, Tom Lane wrote:

Justin Pryzby <pryzby@telsasoft.com> writes:

Or, is it okay to use ABS_SRCDIR?

Don't see why not --- other tests certainly do.

Another solution would be to rely on TESTDIR, which is always set as
of the Makefile invocations and vcregress.pl, but that's doomed for
VPATH builds as this points to the build directory, not the source
directory so we would not have access to the sample file. The root
issue here is that we don't have access to top_srcdir when running the
TAP tests, if we want to keep this stuff as a TAP test.

libpq's regress.pl tweaks things by passing down a SRCDIR to take care
of the VPATH problem, but there is nothing in %ENV that points to the
source directory in the context of a TAP test by default, if we cannot
rely on pg_config to find where the sample file is located. That's
different than this thread, but should we make an effort and expand
the data available here for TAP tests?

Hmm. Relying on ABS_SRCDIR in the main test suite would be fine,
though. It also looks that there would be nothing preventing the
addition of the three tests in the TAP test, as of three SQL queries
instead. These had better be written with one or more CTEs, for
readability, at least, with the inner query reading the contents of
the sample file to grab the list of all the GUCs.

I guess that it would be better to revert the TAP test and rework all
that for the time being, then.
--
Michael

On Sat, Feb 12, 2022 at 09:49:47AM +0900, Michael Paquier wrote:

I guess that it would be better to revert the TAP test and rework all
that for the time being, then.

And this part is done.
--
Michael

Re: Michael Paquier

On Fri, Feb 11, 2022 at 10:48:11AM +0100, Christoph Berg wrote:

It never caused any problem in the 12+ years we have been doing this,
but Debian is patching pg_config not to be relocatable since we are
installing into /usr/lib/postgresql/NN /usr/share/postgresql/NN, so
not a single prefix.

https://salsa.debian.org/postgresql/postgresql/-/blob/15/debian/patches/50-per-version-dirs.patch

Wow. This is the way for Debian to bypass the fact that ./configure
is itself patched, hence you cannot rely on things like --libdir,
--bindir and the kind at build time? That's.. Err.. Fancy, I'd
say.

--libdir and --bindir will point at the final install locations.

I think the "bug" here is that vanilla PG doesn't support installing
in FHS locations with /usr/lib and /usr/share split, hence the Debian
patch.

Christoph

Christoph Berg <myon@debian.org> writes:

I think the "bug" here is that vanilla PG doesn't support installing
in FHS locations with /usr/lib and /usr/share split, hence the Debian
patch.

I'm confused by this statement. An out-of-the-box installation
produces trees under $PREFIX/lib and $PREFIX/share. What about
that is not FHS compliant? And couldn't you fix it using the
installation fine-tuning switches that configure already provides?

regards, tom lane

Re: Tom Lane

Christoph Berg <myon@debian.org> writes:

I think the "bug" here is that vanilla PG doesn't support installing
in FHS locations with /usr/lib and /usr/share split, hence the Debian
patch.

I'm confused by this statement. An out-of-the-box installation
produces trees under $PREFIX/lib and $PREFIX/share. What about
that is not FHS compliant? And couldn't you fix it using the
installation fine-tuning switches that configure already provides?

To support multiple major versions in parallel, we need
/usr/lib/postgresql/NN/lib and /usr/share/postgresql/NN, so it's not a
single $PREFIX. But you are correct, and ./configure supports that.

I was confusing that with this: The problem that led to the pg_config
patch years ago was that we have a /usr/bin/pg_config in
(non-major-version-dependant) libpq-dev, and
/usr/lib/postgresql/NN/bin/pg_config in the individual
postgresql-server-dev-NN packages, and iirc the /usr/bin version
didn't particularly like the other binaries being in
/usr/lib/postgresql/NN/bin/.

I guess it's time to revisit that problem now and see if it can be
solved more pretty today on our side.

Sorry for the rumbling,
Christoph

Christoph Berg <myon@debian.org> writes:

I was confusing that with this: The problem that led to the pg_config
patch years ago was that we have a /usr/bin/pg_config in
(non-major-version-dependant) libpq-dev, and
/usr/lib/postgresql/NN/bin/pg_config in the individual
postgresql-server-dev-NN packages, and iirc the /usr/bin version
didn't particularly like the other binaries being in
/usr/lib/postgresql/NN/bin/.

Ah. That seems a bit problematic anyway ... if the version-dependent
pg_configs don't all return identical results, what's the
non-version-dependent one supposed to do?

regards, tom lane

Re: Tom Lane

Ah. That seems a bit problematic anyway ... if the version-dependent
pg_configs don't all return identical results, what's the
non-version-dependent one supposed to do?

It still returns a correct --includedir for client apps that need it.

(Unfortunately many need --includedir-server since that's where the
type OIDs live.)

Christoph

Hi,

On 2022-02-12 13:29:54 +0900, Michael Paquier wrote:

On Sat, Feb 12, 2022 at 09:49:47AM +0900, Michael Paquier wrote:

I guess that it would be better to revert the TAP test and rework all
that for the time being, then.

And this part is done.

You wrote in the commit message that:

"However, this is also an issue as a TAP test only offers the build directory
as of TESTDIR in the environment context, so this would fail with VPATH
builds."

Tap tests currently are always executed in the source directory above t/, as
far as I can tell. So I don't think VPATH/non-VPATH or windows / non-windows
would break using a relative reference from the test dir.

Greetings,

Andres Freund

On Sat, Feb 12, 2022 at 05:31:55PM +0100, Christoph Berg wrote:

To support multiple major versions in parallel, we need
/usr/lib/postgresql/NN/lib and /usr/share/postgresql/NN, so it's not a
single $PREFIX. But you are correct, and ./configure supports that.

Yep, I don't quite see the problem, but I don't have the years of
experience in terms of packaging you have, either. (NB: I use various
--libdir and --bindir combinations that for some of packaging of PG I
maintain for pg_upgrade, but that's not as complex as Debian, I
guess).

I was confusing that with this: The problem that led to the pg_config
patch years ago was that we have a /usr/bin/pg_config in
(non-major-version-dependant) libpq-dev, and
/usr/lib/postgresql/NN/bin/pg_config in the individual
postgresql-server-dev-NN packages, and iirc the /usr/bin version
didn't particularly like the other binaries being in
/usr/lib/postgresql/NN/bin/.

I guess it's time to revisit that problem now and see if it can be
solved more pretty today on our side.

If you can solve that, my take is that it could make things nicer in
the long-run for some of the TAP test facilities. Still, I'll try to
look at a solution for this thread that does not interact badly with
what you are doing, and I am going to grab your patch when testing
things.
--
Michael

On Sat, Feb 12, 2022 at 12:10:46PM -0800, Andres Freund wrote:

Tap tests currently are always executed in the source directory above t/, as
far as I can tell. So I don't think VPATH/non-VPATH or windows / non-windows
would break using a relative reference from the test dir.

That would mean to rely on $ENV{PWD} for this job, as of something
like that:
-# Find the location of postgresql.conf.sample, based on the information
-# provided by pg_config.
-my $sample_file =
-  $node->config_data('--sharedir') . '/postgresql.conf.sample';
+my $rootdir = $ENV{PWD} . "/../../../..";
+my $sample_file = "$rootdir/src/backend/utils/misc/postgresql.conf.sample";

A run through the CI shows that this is working, and it also works
with Debian's patch for the config data. I still tend to prefer the
readability of a TAP test over the main regression test suite for this
facility. Even if we could use the same trick with PG_ABS_SRCDIR, I'd
rather not add this kind of dependency with a file in the source
tree in src/test/regress/. Do others have any opinions on the matter?
--
Michael

On Mon, Feb 14, 2022 at 02:11:37PM +0900, Michael Paquier wrote:

A run through the CI shows that this is working, and it also works
with Debian's patch for the config data. I still tend to prefer the
readability of a TAP test over the main regression test suite for this
facility.

Hearing nothing, I have looked once again at this stuff and tested it
with what Debian was doing. And that looks to work fine for
everything we care about, so applied with the use of a relative path
to find postgresql.conf.sample in the source tree.
--
Michael

Re: Michael Paquier

Hearing nothing, I have looked once again at this stuff and tested it
with what Debian was doing. And that looks to work fine for
everything we care about, so applied with the use of a relative path
to find postgresql.conf.sample in the source tree.

The build works with the "Add TAP test to automate the equivalent of
check_guc, take two" commit. Thanks!

(Tests are still failing for
/messages/by-id/YgjwrkEvNEqoz4Vm@msg.df7cb.de
though)

Christoph

On Wed, Feb 16, 2022 at 01:38:51PM +0100, Christoph Berg wrote:

The build works with the "Add TAP test to automate the equivalent of
check_guc, take two" commit. Thanks!

Thanks for double-checking, Christoph!

(Tests are still failing for
/messages/by-id/YgjwrkEvNEqoz4Vm@msg.df7cb.de
though)

This has been unanswered for four weeks now. I'll see about jumping
into it, then..
--
Michael

On Wed, Feb 16, 2022 at 7:24 PM Michael Paquier <michael@paquier.xyz> wrote:

On Wed, Feb 16, 2022 at 01:38:51PM +0100, Christoph Berg wrote:

The build works with the "Add TAP test to automate the equivalent of
check_guc, take two" commit. Thanks!

Thanks for double-checking, Christoph!

(Tests are still failing for
/messages/by-id/YgjwrkEvNEqoz4Vm@msg.df7cb.de
though)

This has been unanswered for four weeks now. I'll see about jumping
into it, then..

Sorry, I saw that and then forgot about it ... but isn't it 3 days,
not 4 weeks? In any case, I'm happy to have you take care of it, but I
can also look at it tomorrow if you prefer.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Wed, Feb 16, 2022 at 07:39:26PM -0500, Robert Haas wrote:

Sorry, I saw that and then forgot about it ... but isn't it 3 days,
not 4 weeks? In any case, I'm happy to have you take care of it, but I
can also look at it tomorrow if you prefer.

Ugh. I looked at the top of the thread and saw January the 17th :)

So that means that I need more caffeine. If you are planning to look
at it, please feel free. Thanks!
--
Michael

Re: Michael Paquier

I was confusing that with this: The problem that led to the pg_config
patch years ago was that we have a /usr/bin/pg_config in
(non-major-version-dependant) libpq-dev, and
/usr/lib/postgresql/NN/bin/pg_config in the individual
postgresql-server-dev-NN packages, and iirc the /usr/bin version
didn't particularly like the other binaries being in
/usr/lib/postgresql/NN/bin/.

I guess it's time to revisit that problem now and see if it can be
solved more pretty today on our side.

If you can solve that, my take is that it could make things nicer in
the long-run for some of the TAP test facilities. Still, I'll try to
look at a solution for this thread that does not interact badly with
what you are doing, and I am going to grab your patch when testing
things.

tl;dr: This is no longer an issue.

Since build-time testing broke again about two weeks ago due to
Debian's pg_config patch, I revisited the situation and found that the
patch is in fact no longer necessary to support pg_config in /usr/bin:

To support cross-compilation against libpq, some years ago I had
already replaced /usr/bin/pg_config (in libpq-dev) with a perl script
that collects path info at build time and outputs them statically at
run time [1]https://salsa.debian.org/postgresql/postgresql-common/-/blob/master/server/postgresql.mk#L189-194 https://salsa.debian.org/postgresql/postgresql-common/-/blob/master/server/pg_config.pl. The "real" /usr/lib/postgresql/15/bin/pg_config (in
postgresql-server-dev-15) is still the C version, now unpatched with
full relocatability support.

[1]: https://salsa.debian.org/postgresql/postgresql-common/-/blob/master/server/postgresql.mk#L189-194 https://salsa.debian.org/postgresql/postgresql-common/-/blob/master/server/pg_config.pl
https://salsa.debian.org/postgresql/postgresql-common/-/blob/master/server/pg_config.pl

Christoph

On Fri, Apr 15, 2022 at 04:49:28PM +0200, Christoph Berg wrote:

Since build-time testing broke again about two weeks ago due to
Debian's pg_config patch, I revisited the situation and found that the
patch is in fact no longer necessary to support pg_config in /usr/bin:

To support cross-compilation against libpq, some years ago I had
already replaced /usr/bin/pg_config (in libpq-dev) with a perl script
that collects path info at build time and outputs them statically at
run time [1]. The "real" /usr/lib/postgresql/15/bin/pg_config (in
postgresql-server-dev-15) is still the C version, now unpatched with
full relocatability support.

Nice! Thanks for letting us know, Christoph.
--
Michael