Horton

Thread

  1. regress bug

    Andrew Dunstan <andrew.dunstan@pgexperts.com> — 2012-03-08T18:22:53Z

    I have just found, after an hour or two of frustration, that pg_regress' 
--inputdir and --outputdir options don't play nicely with the 
convert_sourcefiles routines. In effect these options are ignored as 
destinations for converted files, and the destination for converted 
files is apparently always $CWD/{sql,expected}. The upshot is that these 
options are pretty much unusable if you want to have converted files 
(which would, for example, specify the location of data files).

This seems like an outright bug. I don't recall any discussion on it. 
Maybe nobody's come across it before. ISTM the correct behaviour would 
be to put converted sql files in $inputdir/sql and converted results 
files in $outputdir/expected.

Thoughts?

cheers

andrew

  2. Re: regress bug

    David E. Wheeler <david@justatheory.com> — 2012-03-08T19:59:33Z

    On Mar 8, 2012, at 10:22 AM, Andrew Dunstan wrote:

> This seems like an outright bug. I don't recall any discussion on it. Maybe nobody's come across it before. ISTM the correct behaviour would be to put converted sql files in $inputdir/sql and converted results files in $outputdir/expected.

In my extension distributions, I have

    tests/sql
    tests/expected

And for that, --inputdir=test works just fine. I don't mess with --outputdir, which just seems to affect where the actual output is written to, which is just a directory named regression.out at the top of the project.

Best,

David

  3. Re: regress bug

    Andrew Dunstan <andrew.dunstan@pgexperts.com> — 2012-03-08T20:20:59Z

    On 03/08/2012 02:59 PM, David E. Wheeler wrote:
> On Mar 8, 2012, at 10:22 AM, Andrew Dunstan wrote:
>
>> This seems like an outright bug. I don't recall any discussion on it. Maybe nobody's come across it before. ISTM the correct behaviour would be to put converted sql files in $inputdir/sql and converted results files in $outputdir/expected.
> In my extension distributions, I have
>
>      tests/sql
>      tests/expected
>
> And for that, --inputdir=test works just fine. I don't mess with --outputdir, which just seems to affect where the actual output is written to, which is just a directory named regression.out at the top of the project.
>

It works fine if you don't need to do any file conversions (i.e. if you 
don't have "input" or "output" directories). But file_textarray_fdw does.

Here's a patch that I think fixes the problem.

cheers

andrew

  4. Re: regress bug

    David E. Wheeler <david@justatheory.com> — 2012-03-08T20:26:06Z

    On Mar 8, 2012, at 12:20 PM, Andrew Dunstan wrote:

> It works fine if you don't need to do any file conversions (i.e. if you don't have "input" or "output" directories). But file_textarray_fdw does.
> 
> Here's a patch that I think fixes the problem.

While you’re there, an issue I noticed is that the MODULE_PATHNAME substitutions do not work if you have your SQL files in a subdirectory. My extensions have the .sql files in an sql/ directory. So that means when I have something like this in sql/plproxy.sql.in:

    CREATE FUNCTION plproxy_call_handler ()
    RETURNS language_handler AS 'MODULE_PATHNAME' LANGUAGE C;

What I end up with in sql/plproxy.sql is:

    CREATE FUNCTION plproxy_call_handler ()
    RETURNS language_handler AS 'sql/plproxy' LANGUAGE C;

Which doesn’t work at all, because the file is not installed in an `sql` subdirectory, it's just that way in my repository (and the distribution tarball). So I avoid the whole MODULE_PATHNAME business for now (and the .in file) and just do this, instead:

    CREATE FUNCTION plproxy_call_handler ()
    RETURNS language_handler AS 'plproxy' LANGUAGE C;

Which is an okay workaround, but I’m not sure that MODULE_PATHNAME is actually working correctly.

Best,

David

  5. Re: regress bug

    Tom Lane <tgl@sss.pgh.pa.us> — 2012-03-08T20:59:03Z

    "David E. Wheeler" <david@justatheory.com> writes:
> While youre there, an issue I noticed is that the MODULE_PATHNAME
> substitutions do not work if you have your SQL files in a
> subdirectory.

Huh?  MODULE_PATHNAME is not substituted by pg_regress at all (anymore
anyway).  There's still some vestigial support for it in pgxs.mk, but
the future of that code is to vanish, not get improved.  You should
not be needing it to get substituted at build time either.

			regards, tom lane

  6. Re: regress bug

    David E. Wheeler <david@justatheory.com> — 2012-03-08T21:33:24Z

    On Mar 8, 2012, at 12:59 PM, Tom Lane wrote:

> Huh?  MODULE_PATHNAME is not substituted by pg_regress at all (anymore
> anyway).

Yeah, sorry, I meant `make`.

> There's still some vestigial support for it in pgxs.mk, but
> the future of that code is to vanish, not get improved.  You should
> not be needing it to get substituted at build time either.

I still see this pattern a *lot*; I removed it from PL/Proxy last week. The attached tarball demonstrates it:

    > make
    sed 's,MODULE_PATHNAME,$libdir/sql/exttest,g' sql/exttest.sql.in >sql/exttest.sql
    make: *** No rule to make target `exttest.so', needed by `all'.  Stop.

So MODULE_PATHNAME is replaced with $libdir/sql/exttest rather than $libdir/exttest. Maybe that should not be fixed, but there are a *lot* of extensions out there using this approach (copied from contrib, which used it for years, albeit without the .sql.in files in a subdirectory).

So perhaps DATA_built is to be removed from pgxs.mk? And if so, is the idea then that one should just put the module name in the .sql file, rather than MODULE_PATHNAME in a .sql.in file?

Best,

David

  7. Re: regress bug

    Andrew Dunstan <andrew@dunslane.net> — 2012-03-08T21:45:15Z

    On 03/08/2012 04:33 PM, David E. Wheeler wrote:
> On Mar 8, 2012, at 12:59 PM, Tom Lane wrote:
>
>> Huh?  MODULE_PATHNAME is not substituted by pg_regress at all (anymore
>> anyway).
> Yeah, sorry, I meant `make`.
>
>> There's still some vestigial support for it in pgxs.mk, but
>> the future of that code is to vanish, not get improved.  You should
>> not be needing it to get substituted at build time either.
> I still see this pattern a *lot*; I removed it from PL/Proxy last week. The attached tarball demonstrates it:
>
>      >  make
>      sed 's,MODULE_PATHNAME,$libdir/sql/exttest,g' sql/exttest.sql.in>sql/exttest.sql
>      make: *** No rule to make target `exttest.so', needed by `all'.  Stop.
>
> So MODULE_PATHNAME is replaced with $libdir/sql/exttest rather than $libdir/exttest. Maybe that should not be fixed, but there are a *lot* of extensions out there using this approach (copied from contrib, which used it for years, albeit without the .sql.in files in a subdirectory).
>
> So perhaps DATA_built is to be removed from pgxs.mk? And if so, is the idea then that one should just put the module name in the .sql file, rather than MODULE_PATHNAME in a .sql.in file?
>



Extensions (unlike non-extension modules) should not monkey with 
MODULE_PATHNAME at all.

Change the Makefile def from DATA_built to DATA and rename the file to 
exttest.sql

cheers

andrew

  8. Re: regress bug

    David E. Wheeler <david@justatheory.com> — 2012-03-08T21:49:23Z

    On Mar 8, 2012, at 1:45 PM, Andrew Dunstan wrote:

>> So perhaps DATA_built is to be removed from pgxs.mk? And if so, is the idea then that one should just put the module name in the .sql file, rather than MODULE_PATHNAME in a .sql.in file?
> 
> Extensions (unlike non-extension modules) should not monkey with MODULE_PATHNAME at all.
> 
> Change the Makefile def from DATA_built to DATA and rename the file to exttest.sql

I did (and I do). But this is not documented or recommended anywhere. Something should be promulgated to encourage existing authors to do that.

David