Re: pg_regress inputdir

Hi,

with regards to --inputdir, --srcdir, --outputdir and --schedule,
pg_regress does something like this:

convert files in <srcdir>/input, and place them in ./sql
convert files in <srcdir>/output, and place them in ./expected
read schedule from <schedule>
for each test in schedule:
read test from <inputdir>/sql/<testname>.sql
read expected result from <inputdir>/expected/<testname>.out
write results to <outputdir>/results/<testname>.out

My problem when running pg_regress standalone, is that converted files
are written to e.g. ./sql, but read from e.g. <inputdir>/sql, which
makes the --inputdir parameter pretty unusable if it is set to something
different from some path leading to cwd.

Illustrated with code:

Writing converted source file (pg_regress.s:493):
snprintf(destfile, MAXPGPATH, "%s/%s.%s", dest, prefix, suffix);

(Where dest is "sql" or "expected".)

Reading files (pg_regress_main.c:37+38):
snprintf(infile, sizeof(infile), "%s/sql/%s.sql", inputdir, testname);

The attached patch makes pg_regress write converted files to
<inputdir>/sql and <inputdir>/expected, which is one way to make it read
and write to the same directory. Tested on Solaris x86 with pgsql "make
check" and standalone.

-J
--

Jørgen Austvik, Software Engineering - QA
Sun Microsystems Database Group

http://blogs.sun.com/austvik/
http://www.austvik.net/

Jorgen Austvik - Sun Norway wrote:

> The attached patch makes pg_regress write converted files to
> <inputdir>/sql and <inputdir>/expected, which is one way to make it read
> and write to the same directory. Tested on Solaris x86 with pgsql "make
> check" and standalone.

I think this breaks VPATH builds in both letter and spirit. Why do you
need this anyway?

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Alvaro Herrera wrote:
> Jorgen Austvik - Sun Norway wrote:
>
>> The attached patch makes pg_regress write converted files to
>> <inputdir>/sql and <inputdir>/expected, which is one way to make it read
>> and write to the same directory. Tested on Solaris x86 with pgsql "make
>> check" and standalone.
>
> I think this breaks VPATH builds in both letter and spirit.

Letter:

--------8<---------------8<---------------8<---------------8<---------------8<---------------8<-------
bash-3.2$ ggrep -R "\-\-inputdir" *
src/test/regress/pg_regress.c: printf(_(" --inputdir=DIR
take input files from DIR (default \".\")\n"));
Binary file src/test/regress/pg_regress.o matches
Binary file src/test/regress/pg_regress matches
Binary file
src/test/regress/tmp_check/install/usr/local/pgsql/lib/pgxs/src/test/regress/pg_regress
matches
--------8<---------------8<---------------8<---------------8<---------------8<---------------8<-------

Since it is not used in PostgreSQL build (only default value - "."), I
have problems seeing how writing to e.g. "./sql/file" instead of writing
to "sql/file" could break anything. Please explain.

Spirit:

Nobody has ever accuse me of being spiritual ;-), but if you have a URI
that explains the VPATH spirit, I'd be interested to read about it.

> Why do you need this anyway?

I tried to explain that in the first mail, but let me try again.

Use case:
Running pg_regress outside of PostgreSQL build system. pg_regress is
installed in e.g. /usr/postgres/8.3/bin/, "input", "output", "sql" and
"expected" are installed in some other path, e.g.
/usr/postgres/8.3/share/test. User is in ~ and tries to run the
PostgreSQL regression tests. It doesn't work, in fact the only way to
make it work is to cd to the parent directory of "sql" and "expected".

Today, using --inputdir in pg_regress does not work for any other value
than something that resolves to cwd, since it will write a file to
"./sql", but try to read the same file from "<inputdir>/sql".

-J
--

Jørgen Austvik, Software Engineering - QA
Sun Microsystems Database Group

http://blogs.sun.com/austvik/
http://www.austvik.net/

Jorgen Austvik - Sun Norway wrote:
> Alvaro Herrera wrote:

>> I think this breaks VPATH builds in both letter and spirit.
>
> Letter:
>
> --------8<---------------8<---------------8<---------------8<---------------8<---------------8<-------
> bash-3.2$ ggrep -R "\-\-inputdir" *
> src/test/regress/pg_regress.c: printf(_(" --inputdir=DIR take input
> files from DIR (default \".\")\n"));
> Binary file src/test/regress/pg_regress.o matches
> Binary file src/test/regress/pg_regress matches
> Binary file
> src/test/regress/tmp_check/install/usr/local/pgsql/lib/pgxs/src/test/regress/pg_regress
> matches
> --------8<---------------8<---------------8<---------------8<---------------8<---------------8<-------
>
> Since it is not used in PostgreSQL build (only default value - "."), I
> have problems seeing how writing to e.g. "./sql/file" instead of writing
> to "sql/file" could break anything. Please explain.

Well, that's exactly my point -- because in a normal build, it is only
passed as . but in a VPATH dir it is passed as the source dir (relative
or absolute path as when you invoked configure).

> Spirit:
>
> Nobody has ever accuse me of being spiritual ;-), but if you have a URI
> that explains the VPATH spirit, I'd be interested to read about it.

The VPATH spirit is that generated files should reside in the build
directory, not in the source directory. Try creating an empty directory
somewhere, cd'ing to that, and calling /path/to/pgsqlsource/configure.
In the resulting dir try "make installcheck" and see what is passed as
--inputdir.

Hmm ... maybe I spoke too soon; I don't see --inputdir used anywhere.

> > Why do you need this anyway?
>
> I tried to explain that in the first mail, but let me try again.
>
> Use case:
> Running pg_regress outside of PostgreSQL build system. pg_regress is
> installed in e.g. /usr/postgres/8.3/bin/, "input", "output", "sql" and
> "expected" are installed in some other path, e.g.
> /usr/postgres/8.3/share/test. User is in ~ and tries to run the
> PostgreSQL regression tests. It doesn't work, in fact the only way to
> make it work is to cd to the parent directory of "sql" and "expected".
>
> Today, using --inputdir in pg_regress does not work for any other value
> than something that resolves to cwd, since it will write a file to
> "./sql", but try to read the same file from "<inputdir>/sql".

Maybe I'm missing something, but I don't see any reason why this is a
scenario worth supporting. What's the problem with cd'ing into the
directory containing the tests?

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera wrote:
>> Today, using --inputdir in pg_regress does not work for any other value
>> than something that resolves to cwd, since it will write a file to
>> "./sql", but try to read the same file from "<inputdir>/sql".
>
> Maybe I'm missing something, but I don't see any reason why this is a
> scenario worth supporting. What's the problem with cd'ing into the
> directory containing the tests?

I agree - cd or some symlinks to cover up that the --inputdir parameter
does not work if you set it to anything except the default is a workaround.

Do we also agree that if you set --inputdir to anything other than the
default, pg_regress will not work (will write a file to one folder, and
try to read the same file from another)?

And if we agree above - should we make setting --inputdir work (read and
write from/to same directory), remove the --inputdir parameter (since
setting it to anything different from default value doesn't work), or
keep it there (to confuse people)?

-J
--

Jørgen Austvik, Software Engineering - QA
Sun Microsystems Database Group

Jorgen Austvik - Sun Norway wrote:

> Do we also agree that if you set --inputdir to anything other than the
> default, pg_regress will not work (will write a file to one folder, and
> try to read the same file from another)?
>
> And if we agree above - should we make setting --inputdir work (read and
> write from/to same directory), remove the --inputdir parameter (since
> setting it to anything different from default value doesn't work), or
> keep it there (to confuse people)?

I think the problem here is that you have to set --outputdir too.

$ LC_ALL=C /pgsql/build/00head/src/test/regress/pg_regress --inputdir=/pgsql/source/00head/src/test/regress --outputdir=/pgsql/build/00head/src/test/regress timetz
(using postmaster on Unix socket, port 55432)
============== dropping database "regression" ==============
DROP DATABASE
============== creating database "regression" ==============
CREATE DATABASE
ALTER DATABASE
============== running regression test queries ==============
test timetz ... ok

=====================
All 1 tests passed.
=====================

Note that this is a VPATH build, so the input and output dirs are
different.

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera wrote:
> Jorgen Austvik - Sun Norway wrote:
>
> > Do we also agree that if you set --inputdir to anything other than the
> > default, pg_regress will not work (will write a file to one folder, and
> > try to read the same file from another)?
> >
> > And if we agree above - should we make setting --inputdir work (read and
> > write from/to same directory), remove the --inputdir parameter (since
> > setting it to anything different from default value doesn't work), or
> > keep it there (to confuse people)?
>
> I think the problem here is that you have to set --outputdir too.

Huh, scratch that, I chose a bad test. create_function_2 obviously fails as
you say:

$ LC_ALL=C /pgsql/build/00head/src/test/regress/pg_regress --inputdir=/pgsql/source/00head/src/test/regress --srcdir=/pgsql/source/00head/src/test/regress/ create_function_2
(using postmaster on Unix socket, port 55432)
============== dropping database "regression" ==============
DROP DATABASE
============== creating database "regression" ==============
CREATE DATABASE
ALTER DATABASE
============== running regression test queries ==============
test create_function_2 ... /bin/sh: /pgsql/source/00head/src/test/regress/sql/create_function_2.sql: No such file or directory
diff: /pgsql/source/00head/src/test/regress/expected/create_function_2.out: No such file or directory
diff: ./results/create_function_2.out: No such file or directory
diff command failed with status 512: diff -w "/pgsql/source/00head/src/test/regress/expected/create_function_2.out" "./results/create_function_2.out" > "./results/create_function_2.out.diff"

I'm not sure if the problem here is --inputdir or --srcdir, or the fact
that we fail to provide a --builddir switch. In my opinion, the need
for running tests outside the test dir is not very strong (or we would
have heard complaints before), and thus the solution is to remove
--inputdir and --outputdir.

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Alvaro Herrera wrote:
> In my opinion, the need
> for running tests outside the test dir is not very strong (or we would
> have heard complaints before), and thus the solution is to remove
> --inputdir and --outputdir.

Attached is a patch that removes --inputdir and --outputdir. I still
prefere the first patch (that fixed my problem), but removing them is
probably better than having them when they don't work.

Tested with psql make check on solaris x86.

-J
--

Jørgen Austvik, Software Engineering - QA
Sun Microsystems Database Group

Jorgen Austvik - Sun Norway wrote:
> Alvaro Herrera wrote:
>> In my opinion, the need
>> for running tests outside the test dir is not very strong (or we would
>> have heard complaints before), and thus the solution is to remove
>> --inputdir and --outputdir.
>
> Attached is a patch that removes --inputdir and --outputdir. I still
> prefere the first patch (that fixed my problem), but removing them is
> probably better than having them when they don't work.

There is interest among packagers to run the regression tests or other
tests after the build. The Red Hat RPMs have shipped a postgresql-test
package for years with a hacked-up makefile that will probably overwrite
random files that it shouldn't in /usr/lib. So I would rather be in
favor of coming up with a solution that would make this work rather than
removing the options. The solution would probably be adding another
option to place the generated files, but the exact behavior would need
to be worked out.

Peter Eisentraut wrote:

> There is interest among packagers to run the regression tests or other
> tests after the build. The Red Hat RPMs have shipped a postgresql-test
> package for years with a hacked-up makefile that will probably overwrite
> random files that it shouldn't in /usr/lib. So I would rather be in
> favor of coming up with a solution that would make this work rather than
> removing the options. The solution would probably be adding another
> option to place the generated files, but the exact behavior would need
> to be worked out.

Hmm. I took a look at the RPM makefiles and patches, and it doesn't
seem like changing this part of pg_regress would solve anything. The
RPM changes are about shared libraries, whereas this is just about
moving the generated files (equivalent to those in the "sql" directory).

For an example of the hacked-up makefiles and stuff, see here:
https://projects.commandprompt.com/public/pgcore/browser/rpm/redhat/8.3/postgresql/F-9/

The relevant files are Makefile.regress, postgresql-test.patch, and
postgresql-8.3.spec (lines 440ff).

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> Peter Eisentraut wrote:
>> There is interest among packagers to run the regression tests or other
>> tests after the build. The Red Hat RPMs have shipped a postgresql-test
>> package for years with a hacked-up makefile that will probably overwrite
>> random files that it shouldn't in /usr/lib. So I would rather be in
>> favor of coming up with a solution that would make this work rather than
>> removing the options. The solution would probably be adding another
>> option to place the generated files, but the exact behavior would need
>> to be worked out.

> Hmm. I took a look at the RPM makefiles and patches, and it doesn't
> seem like changing this part of pg_regress would solve anything.

Well, it would be interesting if it were possible for an unprivileged
user to run postgresql-test. That would mean arranging for the tests to
not write anything in the regression source directory, but only in some
user-private directory; ie, keeping the modifiable and non-modifiable
files separate. Which I think is what Peter is getting at above.

However, at least for Red Hat I don't think I could use such a feature
if I had it :-(. You'll note that Makefile.regress has to fool around
with SELinux labeling, which I think isn't possible for any old random
user. That's not something that could be avoided if we had a pg_regress
that was careful about modifiable vs non-modifiable files, because the
restriction is actually enforced against the installed postgresql
binaries.

regards, tom lane

Jorgen Austvik - Sun Norway wrote:

> The attached patch makes pg_regress write converted files to
> <inputdir>/sql and <inputdir>/expected, which is one way to make it read
> and write to the same directory. Tested on Solaris x86 with pgsql "make
> check" and standalone.

Okay, so this patch does change it in a way that it still works, but
what else do you need to be able to run the test from another directory?
I tried to run the test from another directory with this patch
installed, and found that it didn't work because it's replacing
@abs_builddir@ in the input files improperly (to the current path; it
should be using the output dir path, I think)

So maybe this is a step in the right direction, but ISTM you need a
slightly larger patch for it to be actually useful.

If I am not making sense, then maybe I am not understanding what you
mean by running it standalone. In that case, please explain.

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera wrote:
> Jorgen Austvik - Sun Norway wrote:
>
>> The attached patch makes pg_regress write converted files to
>> <inputdir>/sql and <inputdir>/expected, which is one way to make it read
>> and write to the same directory. Tested on Solaris x86 with pgsql "make
>> check" and standalone.
>
> Okay, so this patch does change it in a way that it still works, but
> what else do you need to be able to run the test from another directory?
> I tried to run the test from another directory with this patch
> installed, and found that it didn't work because it's replacing
> @abs_builddir@ in the input files improperly (to the current path; it
> should be using the output dir path, I think)
>
> So maybe this is a step in the right direction, but ISTM you need a
> slightly larger patch for it to be actually useful.
>
> If I am not making sense, then maybe I am not understanding what you
> mean by running it standalone. In that case, please explain.
>

I think the appropriate interface would be adding another option to
pg_regress called --workdir or --tempdir, which defaults to PWD, and
write the converted sql files there, and then look for the sql files to
execute in workdir/sql and in inputdir/sql. In some way, this copies
the vpath search mechanism.

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> Alvaro Herrera wrote:
>> I tried to run the test from another directory with this patch
>> installed, and found that it didn't work because it's replacing
>> @abs_builddir@ in the input files improperly (to the current path; it
>> should be using the output dir path, I think)

> I think the appropriate interface would be adding another option to
> pg_regress called --workdir or --tempdir, which defaults to PWD, and
> write the converted sql files there, and then look for the sql files to
> execute in workdir/sql and in inputdir/sql. In some way, this copies
> the vpath search mechanism.

That would be required to make pg_regress run as far as its own
facilities are concerned. But I think Alvaro is worried about something
at a higher level: the regression test process as a whole has some
directory layout assumptions built into it, particularly in regards
to where to find .so's. If we don't have a workable solution for that
it's not really going to help to change pg_regress like this.

regards, tom lane

Tom Lane wrote:
> Peter Eisentraut <peter_e(at)gmx(dot)net> writes:

> > I think the appropriate interface would be adding another option to
> > pg_regress called --workdir or --tempdir, which defaults to PWD, and
> > write the converted sql files there, and then look for the sql files to
> > execute in workdir/sql and in inputdir/sql. In some way, this copies
> > the vpath search mechanism.
>
> That would be required to make pg_regress run as far as its own
> facilities are concerned. But I think Alvaro is worried about something
> at a higher level: the regression test process as a whole has some
> directory layout assumptions built into it, particularly in regards
> to where to find .so's. If we don't have a workable solution for that
> it's not really going to help to change pg_regress like this.

Maybe the same work dir can be used as a place to store the shared
objects. I think all it'd require is to change @abs_builddir@ to point
to workdir.

That should work fine as long as nobody attempts to put the workdir in
some mount point that's marked noexec (which is somewhat common with
/tmp)

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Tom Lane wrote:
> But I think Alvaro is worried about something
> at a higher level: the regression test process as a whole has some
> directory layout assumptions built into it, particularly in regards
> to where to find .so's.

The only information about the location of the .so's is in the test
files themselves, which seems reasonable, because they are created and
installed at the same time as the .so's that they are presumably
supposed to test. So I see no problem here.

Peter Eisentraut wrote:
> Tom Lane wrote:
>> But I think Alvaro is worried about something
>> at a higher level: the regression test process as a whole has some
>> directory layout assumptions built into it, particularly in regards
>> to where to find .so's.
>
> The only information about the location of the .so's is in the test
> files themselves, which seems reasonable, because they are created and
> installed at the same time as the .so's that they are presumably
> supposed to test. So I see no problem here.

Here is a more involved patch that fixes all these issues. The major
simplication is that the input files are looked for in both the build
tree and the source tree (like a vpath search), which allowed me to
remove a lot of redundant makefile code. I could also remove the
--srcdir option but added --dlpath to address the above mentioned issue
and changed some option defaults. Now you can run pg_regress inside and
outside of the build tree. It isn't quite ready for the general public,
but a packager that wants to adopt this can use it. Currently, you need
to create the directories sql, expected, and testtablespace yourself,
when running outside the build tree. We can attempt to sort that out
later, but SELinux might make it difficult.