Thread: Very confusing installcheck behavior with PGXS
The rule that gets executed if you do `make installcheck` with something using PGXS is pgxs.mk: $(pg_regress_installcheck) $(REGRESS_OPTS) $(REGRESS) where $(pg_regress_installcheck) is set in Makefile.global.in to > pg_regress_installcheck = $(top_builddir)/src/test/regress/pg_regress --inputdir=$(srcdir) --bindir='$(bindir)' $(pg_regress_locale_flags)$(EXTRA_REGRESS_OPTS) The problem here is that in a PGXS make, srcdir is set to '.'[1], and --inputdir is specified a second time in REGRESS_OPTS. Normally that works OK (for some reason ignoring what's in ./sql), but if you happen to have a file in your test/sql directory that matches a file in ./sql, pg_regress runs the first file and not the second. Presumably that's exactly what you'd want in most of the tree, but it's definitely not what you want in an extension. Is the best way to fix this to add a pg_regress_installcheck_pgxs variable in Makefile.global.in and modify pgxs.mk accordingly? [1]: > decibel@decina:[16:18]~/git/trunklet (master=)$make print-pg_regress_installcheck print-REGRESS_OPTS print-REGRESS > pg_regress_installcheck = /Users/decibel/pgsql/9.4/i/lib/pgxs/src/makefiles/../../src/test/regress/pg_regress --inputdir=./--psqldir=/Users/decibel/pgsql/9.4/i/bin > REGRESS_OPTS = --inputdir=test --load-language=plpgsql --dbname=contrib_regression > REGRESS = all build -- Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX Experts in Analytics, Data Architecture and PostgreSQL Data in Trouble? Get it in Treble! http://BlueTreble.com
On Sun, Jan 3, 2016 at 5:22 PM, Jim Nasby <Jim.Nasby@bluetreble.com> wrote: > The rule that gets executed if you do `make installcheck` with something > using PGXS is > > pgxs.mk: $(pg_regress_installcheck) $(REGRESS_OPTS) $(REGRESS) > > where $(pg_regress_installcheck) is set in Makefile.global.in to > >> pg_regress_installcheck = $(top_builddir)/src/test/regress/pg_regress >> --inputdir=$(srcdir) --bindir='$(bindir)' $(pg_regress_locale_flags) >> $(EXTRA_REGRESS_OPTS) > > The problem here is that in a PGXS make, srcdir is set to '.'[1], and > --inputdir is specified a second time in REGRESS_OPTS. Normally that works > OK (for some reason ignoring what's in ./sql), but if you happen to have a > file in your test/sql directory that matches a file in ./sql, pg_regress > runs the first file and not the second. Stupid question time: why in the world would you have that? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> On Sun, Jan 3, 2016 at 5:22 PM, Jim Nasby <Jim.Nasby@bluetreble.com> wrote:
>> The rule that gets executed if you do `make installcheck` with something
>> using PGXS is
>>
>> pgxs.mk: $(pg_regress_installcheck) $(REGRESS_OPTS) $(REGRESS)
>>
>> where $(pg_regress_installcheck) is set in Makefile.global.in to
>>
> pg_regress_installcheck = $(top_builddir)/src/test/regress/pg_regress
> --inputdir=$(srcdir) --bindir='$(bindir)' $(pg_regress_locale_flags)
> $(EXTRA_REGRESS_OPTS)
>>
>> The problem here is that in a PGXS make, srcdir is set to '.'[1], and
>> --inputdir is specified a second time in REGRESS_OPTS. Normally that works
>> OK (for some reason ignoring what's in ./sql), but if you happen to have a
>> file in your test/sql directory that matches a file in ./sql, pg_regress
>> runs the first file and not the second.
> Stupid question time: why in the world would you have that?
AFAICS, the pieces we supply (Makefile.global and pgxs.mk) would only
specify --inputdir once. If there's a second specification being added
to REGRESS_OPTS by your own Makefile, that would override the default,
which seems like entirely sensible behavior. Maybe I'm misunderstanding
something, but it sounds like you're saying "if I write --inputdir=test
in REGRESS_OPTS, it runs the tests in test/sql not those in ./sql".
Why would you not think that's expected?
regards, tom lane
On 1/6/16 11:54 AM, Tom Lane wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> On Sun, Jan 3, 2016 at 5:22 PM, Jim Nasby <Jim.Nasby@bluetreble.com> wrote: >>> The rule that gets executed if you do `make installcheck` with something >>> using PGXS is >>> >>> pgxs.mk: $(pg_regress_installcheck) $(REGRESS_OPTS) $(REGRESS) >>> >>> where $(pg_regress_installcheck) is set in Makefile.global.in to >>> >> pg_regress_installcheck = $(top_builddir)/src/test/regress/pg_regress >> --inputdir=$(srcdir) --bindir='$(bindir)' $(pg_regress_locale_flags) >> $(EXTRA_REGRESS_OPTS) >>> >>> The problem here is that in a PGXS make, srcdir is set to '.'[1], and >>> --inputdir is specified a second time in REGRESS_OPTS. Normally that works >>> OK (for some reason ignoring what's in ./sql), but if you happen to have a >>> file in your test/sql directory that matches a file in ./sql, pg_regress >>> runs the first file and not the second. > >> Stupid question time: why in the world would you have that? It doesn't seem that odd to have a test file that matches an extension file... > AFAICS, the pieces we supply (Makefile.global and pgxs.mk) would only > specify --inputdir once. If there's a second specification being added > to REGRESS_OPTS by your own Makefile, that would override the default, > which seems like entirely sensible behavior. Maybe I'm misunderstanding > something, but it sounds like you're saying "if I write --inputdir=test > in REGRESS_OPTS, it runs the tests in test/sql not those in ./sql". > Why would you not think that's expected? Actually, it's more bizarre than I thought... This is what my makefile[1] produces, which currently works: /Users/decibel/pgsql/HEAD/i/lib/pgxs/src/makefiles/../../src/test/regress/pg_regress --inputdir=./ --bindir='/Users/decibel/pgsql/HEAD/i/bin' --inputdir=test --load-language=plpgsql --dbname=contrib_regression acl_type build compat rights Removing the first --inputdir also works: /Users/decibel/pgsql/HEAD/i/lib/pgxs/src/makefiles/../../src/test/regress/pg_regress --bindir='/Users/decibel/pgsql/HEAD/i/bin' --inputdir=test --load-language=plpgsql --dbname=contrib_regression acl_type build compat rights However, if I do this: mv test/sql/acl_type.sql test/sql/acl.sql mv test/expected/acl_type.out test/expected/acl.out And change acl_type to acl in that pg_regress command: /Users/decibel/pgsql/HEAD/i/lib/pgxs/src/makefiles/../../src/test/regress/pg_regress --bindir='/Users/decibel/pgsql/HEAD/i/bin' --inputdir=test --load-language=plpgsql --dbname=contrib_regression acl build compat rights Instead of executing test/sql/acl.sql, it executes ./sql/acl.sql. I'd assumed this was due to the extra --inputdir option, but clearly something else is going on here. Time to gvim pg_regress.c... [1] https://github.com/decibel/pg_acl -- Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX Experts in Analytics, Data Architecture and PostgreSQL Data in Trouble? Get it in Treble! http://BlueTreble.com
Jim Nasby <Jim.Nasby@BlueTreble.com> writes:
> However, if I do this:
> mv test/sql/acl_type.sql test/sql/acl.sql
> mv test/expected/acl_type.out test/expected/acl.out
> And change acl_type to acl in that pg_regress command:
> /Users/decibel/pgsql/HEAD/i/lib/pgxs/src/makefiles/../../src/test/regress/pg_regress
> --bindir='/Users/decibel/pgsql/HEAD/i/bin' --inputdir=test
> --load-language=plpgsql --dbname=contrib_regression acl build compat rights
> Instead of executing test/sql/acl.sql, it executes ./sql/acl.sql.
That's pretty hard to believe. There's nothing in pg_regress that looks
in places other than the given --inputdir.
I wonder whether you have a test/input/acl.sql and/or test/output/acl.out
that's confusing matters.
regards, tom lane
On 1/7/16 8:47 AM, Tom Lane wrote: > Jim Nasby <Jim.Nasby@BlueTreble.com> writes: >> However, if I do this: >> mv test/sql/acl_type.sql test/sql/acl.sql >> mv test/expected/acl_type.out test/expected/acl.out > >> And change acl_type to acl in that pg_regress command: >> /Users/decibel/pgsql/HEAD/i/lib/pgxs/src/makefiles/../../src/test/regress/pg_regress >> --bindir='/Users/decibel/pgsql/HEAD/i/bin' --inputdir=test >> --load-language=plpgsql --dbname=contrib_regression acl build compat rights > >> Instead of executing test/sql/acl.sql, it executes ./sql/acl.sql. > > That's pretty hard to believe. There's nothing in pg_regress that looks > in places other than the given --inputdir. Actually, I think it does... from pg_regress_main.c: /* * Look for files in the output dir first, consistent with a vpath search. * This is mainly to create more reasonable errormessages if the file is * not found. It also allows local test overrides when running pg_regress * outside of the sourcetree. */snprintf(infile, sizeof(infile), "%s/sql/%s.sql", outputdir, testname);if (!file_exists(infile)) snprintf(infile, sizeof(infile), "%s/sql/%s.sql", inputdir, testname); If I add --outputdir=test, then everything works fine. Obviously I can just deal with this in my Makefile, but is this really the behavior we want? It certainly seems to violate POLA... > I wonder whether you have a test/input/acl.sql and/or test/output/acl.out > that's confusing matters. Nope... decibel@decina:[08:35]~/git/pg_acl (master *%=)$ll test total 16 drwxr-x--- 6 decibel staff 204 Jan 2 17:31 ./ drwxr-x--- 17 decibel staff 578 Jan 7 08:35 ../ -rw-r----- 1 decibel staff 65 Jan 2 17:31 deps.sql drwxr-x--- 6 decibel staff 204 Jan 7 08:32 expected/ lrwxr-x--- 1 decibel staff 25 Dec 26 13:43 pgxntool@ -> ../pgxntool/test/pgxntool drwxr-x--- 6 decibel staff 204 Jan 7 08:32 sql/ decibel@decina:[08:48]~/git/pg_acl (master *%=)$ -- Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX Experts in Analytics, Data Architecture and PostgreSQL Data in Trouble? Get it in Treble! http://BlueTreble.com
Jim Nasby <Jim.Nasby@BlueTreble.com> writes:
> On 1/7/16 8:47 AM, Tom Lane wrote:
>> That's pretty hard to believe. There's nothing in pg_regress that looks
>> in places other than the given --inputdir.
> Actually, I think it does... from pg_regress_main.c:
> /*
> * Look for files in the output dir first, consistent with a vpath search.
> * This is mainly to create more reasonable error messages if the file is
> * not found. It also allows local test overrides when running pg_regress
> * outside of the source tree.
> */
> snprintf(infile, sizeof(infile), "%s/sql/%s.sql",
> outputdir, testname);
> if (!file_exists(infile))
> snprintf(infile, sizeof(infile), "%s/sql/%s.sql",
> inputdir, testname);
Oh, I'd just been looking in pg_regress.c.
The comment's argument about "more reasonable error messages" seems pretty
dubious to me. The real reason for this behavior seems to have been to
simplify VPATH builds --- see commit feae7856, which added this code,
and was able to get rid of quite a lot of makefile cruft.
I think though that doing it exactly like this is a bit klugy. A better
implementation of VPATH would've been to introduce an optional "secondary
input directory" into which we look if we fail to find something in the
main input directory. Then we'd run it with main input directory in the
build tree and secondary input directory pointing to the source tree.
(I'm also wondering how convert_sourcefiles() works at all in a vpath
build, considering that I don't see it doing anything like this ...)
regards, tom lane
On 1/7/16 9:12 AM, Tom Lane wrote: > Jim Nasby <Jim.Nasby@BlueTreble.com> writes: >> On 1/7/16 8:47 AM, Tom Lane wrote: >>> That's pretty hard to believe. There's nothing in pg_regress that looks >>> in places other than the given --inputdir. > >> Actually, I think it does... from pg_regress_main.c: > >> /* >> * Look for files in the output dir first, consistent with a vpath search. >> * This is mainly to create more reasonable error messages if the file is >> * not found. It also allows local test overrides when running pg_regress >> * outside of the source tree. >> */ >> snprintf(infile, sizeof(infile), "%s/sql/%s.sql", >> outputdir, testname); >> if (!file_exists(infile)) >> snprintf(infile, sizeof(infile), "%s/sql/%s.sql", >> inputdir, testname); > > Oh, I'd just been looking in pg_regress.c. > > The comment's argument about "more reasonable error messages" seems pretty > dubious to me. The real reason for this behavior seems to have been to > simplify VPATH builds --- see commit feae7856, which added this code, > and was able to get rid of quite a lot of makefile cruft. > > I think though that doing it exactly like this is a bit klugy. A better > implementation of VPATH would've been to introduce an optional "secondary > input directory" into which we look if we fail to find something in the > main input directory. Then we'd run it with main input directory in the > build tree and secondary input directory pointing to the source tree. Seems reasonable. Sounds like a good beginner project to boot. :) > (I'm also wondering how convert_sourcefiles() works at all in a vpath > build, considering that I don't see it doing anything like this ...) It's only looking at outputdir, which I suspect is never ambiguous. -- Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX Experts in Analytics, Data Architecture and PostgreSQL Data in Trouble? Get it in Treble! http://BlueTreble.com
Jim Nasby <Jim.Nasby@bluetreble.com> writes:
> On 1/7/16 9:12 AM, Tom Lane wrote:
>> (I'm also wondering how convert_sourcefiles() works at all in a vpath
>> build, considering that I don't see it doing anything like this ...)
> It's only looking at outputdir, which I suspect is never ambiguous.
Eh, no, look again. What it's actually doing is reading $inputdir/input/
and converting into $outputdir/sql/, and reading $inputdir/output/ and
converting into $outputdir/expected/. I guess that works, but again it's
kind of at variance with the normal expectation of VPATH behavior. What
you'd expect is that $builddir/input files would override $srcdir/input
files; but as is, $builddir/input and $builddir/output are never examined
at all.
regards, tom lane
On 1/7/16 9:56 AM, Tom Lane wrote: > Jim Nasby <Jim.Nasby@bluetreble.com> writes: >> On 1/7/16 9:12 AM, Tom Lane wrote: >>> (I'm also wondering how convert_sourcefiles() works at all in a vpath >>> build, considering that I don't see it doing anything like this ...) > >> It's only looking at outputdir, which I suspect is never ambiguous. > > Eh, no, look again. What it's actually doing is reading $inputdir/input/ > and converting into $outputdir/sql/, and reading $inputdir/output/ and > converting into $outputdir/expected/. I guess that works, but again it's > kind of at variance with the normal expectation of VPATH behavior. What > you'd expect is that $builddir/input files would override $srcdir/input > files; but as is, $builddir/input and $builddir/output are never examined > at all. Yeah, I just discovered the whole input/ and output/ bit. That really complicates things, because ISTM it's very common to directly specify both sql/ and expected/ files directly, and you'd certainly THINK that those files are input, and not output. Which begs the question... how the hell do sql/ and expected/ ever work in a vpath build? AFAICT things are never copied from $inputdir/(sql|expected) to $outputdir... Maybe it's just me, but this whole convention seems like a giant POLA violation. If pg_regress was only used in Postgres source maybe that wouldn't matter since presumably there's always an example to copy from (and presumably tests use either input/ and output/ OR sql/ and expected/, but never both). But pg_regress is used by contrib and now extensions, so it's got a much wider audience than just -hackers. :/ input and output are used in only 3 places in the whole tree[1], so maybe the best thing to do is just rip support for that out of pg_regress and handle it some other way. Also worth noting: the only reason I'm using pg_regress is it's the easiest way to get a test cluster. If not for that, I'd just use pg_prove since I'm already using pgTap. [1] find . \( -name input -o -name output \) -type d ./contrib/dblink/input ./contrib/dblink/output ./contrib/file_fdw/input ./contrib/file_fdw/output ./src/test/regress/input ./src/test/regress/output -- Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX Experts in Analytics, Data Architecture and PostgreSQL Data in Trouble? Get it in Treble! http://BlueTreble.com
Jim Nasby <Jim.Nasby@bluetreble.com> writes:
> input and output are used in only 3 places in the whole tree[1], so
> maybe the best thing to do is just rip support for that out of
> pg_regress and handle it some other way.
[ raised eyebrow ] It's not apparent to me that moving it someplace else
would reduce the net cruft any.
The real problem is that Peter just did the minimum amount of work
to get VPATH to work at all, not to get it to work in a non-surprising
way. We really need this code to be explicitly VPATH-aware, I think,
rather than overloading the inputdir and outputdir concepts in
multiple ways.
regards, tom lane
On 1/7/16 11:49 AM, Tom Lane wrote: > Jim Nasby <Jim.Nasby@bluetreble.com> writes: >> input and output are used in only 3 places in the whole tree[1], so >> maybe the best thing to do is just rip support for that out of >> pg_regress and handle it some other way. > > [ raised eyebrow ] It's not apparent to me that moving it someplace else > would reduce the net cruft any. What it would do is de-mystify that part of pg_regress. I agree that vpath is a particular problem for input/ and output/, but it's even worse when most everyone thinks sql/ and expected/ are inputs to pg_regress, and not potential outputs. > The real problem is that Peter just did the minimum amount of work > to get VPATH to work at all, not to get it to work in a non-surprising > way. We really need this code to be explicitly VPATH-aware, I think, > rather than overloading the inputdir and outputdir concepts in > multiple ways. Agreed, especially because if it's not then if you try to use both input/ and sql/ or output/ and expected/ you're not going to get the results you'd like. If we want to keep input/ and output/ inside pg_regress then I think what needs to happen in a vpath build is to first create $vpath/sql and $vpath/expected, copy anything from $(uh... source?)/sql and /expected there, and then process /input and /output (and deal with any duplicate file references). -- Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX Experts in Analytics, Data Architecture and PostgreSQL Data in Trouble? Get it in Treble! http://BlueTreble.com
Jim Nasby <Jim.Nasby@bluetreble.com> writes:
> If we want to keep input/ and output/ inside pg_regress then I think
> what needs to happen in a vpath build is to first create $vpath/sql and
> $vpath/expected, copy anything from $(uh... source?)/sql and /expected
> there, and then process /input and /output (and deal with any duplicate
> file references).
No, I don't think we want to physically copy anything we don't have to.
(Before the commit I mentioned upthread, we were actually doing it more
or less as you suggest, and it was messy.)
I'm not really concerned about the current behavior of putting transformed
input/output files into sql/ and expected/. Only experts are likely to
be creating files requiring transformation at all (and even the experts
prefer to avoid that, because they're a PITA). So I am not very worried
about duplication of file names between eg input/ and sql/. But I don't
like handling VPATH by introducing confusion between --inputdir and
--outputdir.
It does strike me though that some of your pain is self-inflicted: why
did you think it was a good idea to create both test/sql/ and sql/
subdirectories? What's the point of test/ at all? It's certainly not
part of the standard layout for a contrib module.
regards, tom lane
On 1/7/16 12:12 PM, Tom Lane wrote: > I'm not really concerned about the current behavior of putting transformed > input/output files into sql/ and expected/. Only experts are likely to > be creating files requiring transformation at all (and even the experts > prefer to avoid that, because they're a PITA). So I am not very worried > about duplication of file names between eg input/ and sql/. But I don't > like handling VPATH by introducing confusion between --inputdir and > --outputdir. Maybe I'm just confused... what should happen then in a vpath build? Do the results of input/*.source get put in sql/ in the source tree? Oh, I guess there's magic that looks first in $source/sql and then in $vpath/sql? One thing I'm in violent agreement on is there's way too much magic happening here. That magic isn't even hinted at by --help, and since there's no documentation on pg_regress extension authors can only follow other examples here... > It does strike me though that some of your pain is self-inflicted: why > did you think it was a good idea to create both test/sql/ and sql/ > subdirectories? What's the point of test/ at all? It's certainly not > part of the standard layout for a contrib module. I blame David Wheeler, who I copied the pattern from. :) Actually, I got the pattern from pgxntool, which AFAIK got the pattern from him. Regardless of blame, the problem I see is that extensions frequently deal with nothing but sql, and mixing up your test code with your extension code is going to be confusing and messy. So instead of piling everything into /sql, David decided to put the test stuff in /test and leave the sql stuff in /sql (if there's C files those tend to go in /src). I see a lot of extensions using this pattern (or something similar). The other common pattern I see is to just pile everything into the top level extension directory. That's OK at first (and for a really, really simple extension might be all you ever want), but if you start having a few tests, a doc file, and several upgrade scripts that starts getting messy. -- Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX Experts in Analytics, Data Architecture and PostgreSQL Data in Trouble? Get it in Treble! http://BlueTreble.com
Jim Nasby wrote: > Also worth noting: the only reason I'm using pg_regress is it's the easiest > way to get a test cluster. If not for that, I'd just use pg_prove since I'm > already using pgTap. In 9.5 you might want to "use PostgresNode" which allows you to initdb and such. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 1/7/16 1:04 PM, Alvaro Herrera wrote: > Jim Nasby wrote: > >> Also worth noting: the only reason I'm using pg_regress is it's the easiest >> way to get a test cluster. If not for that, I'd just use pg_prove since I'm >> already using pgTap. > > In 9.5 you might want to "use PostgresNode" which allows you to initdb > and such. Oooh, thanks! I might well just copy that into my pgxntool utility. -- Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX Experts in Analytics, Data Architecture and PostgreSQL Data in Trouble? Get it in Treble! http://BlueTreble.com
On Jan 7, 2016, at 11:20 AM, Jim Nasby <Jim.Nasby@BlueTreble.com> wrote: >>> Also worth noting: the only reason I'm using pg_regress is it's the easiest >>> way to get a test cluster. If not for that, I'd just use pg_prove since I'm >>> already using pgTap. >> >> In 9.5 you might want to "use PostgresNode" which allows you to initdb >> and such. > > Oooh, thanks! I might well just copy that into my pgxntool utility. Is this documented somewhere? If it’s Perl, seems like it’d only be useful for those of us who compile from source, yes? David
David E. Wheeler wrote: > On Jan 7, 2016, at 11:20 AM, Jim Nasby <Jim.Nasby@BlueTreble.com> wrote: > > >>> Also worth noting: the only reason I'm using pg_regress is it's the easiest > >>> way to get a test cluster. If not for that, I'd just use pg_prove since I'm > >>> already using pgTap. > >> > >> In 9.5 you might want to "use PostgresNode" which allows you to initdb > >> and such. > > > > Oooh, thanks! I might well just copy that into my pgxntool utility. > > Is this documented somewhere? Not in detail. Patches welcome; see the "regress-tap" sect1 in doc/src/sgml/regress.sgml. > If it’s Perl, seems like it’d only be useful for those of us who compile from source, yes? Feel free to submit patches for discussion to install the modules. We already install pg_regress under lib/pgxs/src/test/regress/ so I see no reason we couldn't install the Perl test modules somewhere. If you hurry you can still make it to 9.6's last commitfest. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services