Re: pgsql: Add TAP test to automate the equivalent of check_guc
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 signature.asc Description: PGP signature
Re: pgsql: Add TAP test to automate the equivalent of check_guc
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]. 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 Christoph
Re: pgsql: Add TAP test to automate the equivalent of check_guc
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 signature.asc Description: PGP signature
Re: pgsql: Add TAP test to automate the equivalent of check_guc
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 https://www.postgresql.org/message-id/YgjwrkEvNEqoz4Vm%40msg.df7cb.de though) Christoph
Re: pgsql: Add TAP test to automate the equivalent of check_guc
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 signature.asc Description: PGP signature
Re: pgsql: Add TAP test to automate the equivalent of check_guc
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
signature.asc
Description: PGP signature
Re: pgsql: Add TAP test to automate the equivalent of check_guc
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 signature.asc Description: PGP signature
Re: pgsql: Add TAP test to automate the equivalent of check_guc
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
Re: pgsql: Add TAP test to automate the equivalent of check_guc
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
Re: pgsql: Add TAP test to automate the equivalent of check_guc
Christoph Berg 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: pgsql: Add TAP test to automate the equivalent of check_guc
Re: Tom Lane > Christoph Berg 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
Re: pgsql: Add TAP test to automate the equivalent of check_guc
Christoph Berg 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: pgsql: Add TAP test to automate the equivalent of check_guc
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
Re: pgsql: Add TAP test to automate the equivalent of check_guc
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 signature.asc Description: PGP signature
Re: pgsql: Add TAP test to automate the equivalent of check_guc
On Fri, Feb 11, 2022 at 12:34:31PM -0500, Tom Lane wrote: > Justin Pryzby 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 signature.asc Description: PGP signature
Re: pgsql: Add TAP test to automate the equivalent of check_guc
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 signature.asc Description: PGP signature
Re: pgsql: Add TAP test to automate the equivalent of check_guc
Justin Pryzby writes: > Or, is it okay to use ABS_SRCDIR? Don't see why not --- other tests certainly do. regards, tom lane
Re: pgsql: Add TAP test to automate the equivalent of check_guc
On Fri, Feb 11, 2022 at 10:41:27AM -0500, Tom Lane wrote:
> Justin Pryzby 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
Re: pgsql: Add TAP test to automate the equivalent of check_guc
Justin Pryzby 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
Re: pgsql: Add TAP test to automate the equivalent of check_guc
On Fri, Feb 11, 2022 at 09:59:55AM -0500, Tom Lane wrote:
> Christoph Berg 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.
https://www.postgresql.org/message-id/YfTg/WHNLVVygy8v%40paquier.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
Re: pgsql: Add TAP test to automate the equivalent of check_guc
Christoph Berg 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
Re: 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]: *** [/<>/build/../src/makefiles/pgxs.mk:457:
check] Error 1
00:55:09 make[3]: Leaving directory
'/<>/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
