Обсуждение: regress bug

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

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
correctbehaviour 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
actualoutput is written to, which is just a directory named regression.out at the top of the project. 

Best,

David

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
correctbehaviour 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
actualoutput 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

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).
Butfile_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
ina subdirectory. My extensions have the .sql files in an sql/ directory. So that means when I have something like this
insql/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
justdo 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

"David E. Wheeler" <david@justatheory.com> writes:
> 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.

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

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,
butthere are a *lot* of extensions out there using this approach (copied from contrib, which used it for years, albeit
withoutthe .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
namein the .sql file, rather than MODULE_PATHNAME in a .sql.in file? 

Best,

David

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,
butthere are a *lot* of extensions out there using this approach (copied from contrib, which used it for years, albeit
withoutthe .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
namein 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

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
namein 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
existingauthors to do that. 

David