Thread: Failure with regression test largeobject if pg_regress invoked from external paths

Hi all,

Today a colleague (Kingter Wang) has reminded me about a failure with
the test case largeobject that can happen if pg_regress is run from an
external path that has not results/ available at hand:
*** /home/packages/stuff/expected/largeobject.out Tue Jan  5 07:56:58 2016
--- /home/packages/stuff/results/largeobject.out Tue Jan  5 07:57:21 2016
***************
*** 388,405 ****
  (1 row)

  \lo_import 'results/lotest.txt'
  \set newloid :LASTOID
  -- just make sure \lo_export does not barf
  \lo_export :newloid 'results/lotest2.txt'
  -- This is a hack to test that export/import are reversible
  -- This uses knowledge about the inner workings of large object mechanism
  -- which should not be used outside it.  This makes it a HACK
  SELECT pageno, data FROM pg_largeobject WHERE loid = (SELECT loid
from lotest_stash_values)
  EXCEPT
  SELECT pageno, data FROM pg_largeobject WHERE loid = :newloid;
!  pageno | data
! --------+------
! (0 rows)

  SELECT lo_unlink(loid) FROM lotest_stash_values;
   lo_unlink
--- 388,735 ----
  (1 row)

  \lo_import 'results/lotest.txt'
+ could not open file "results/lotest.txt": No such file or directory
  \set newloid :LASTOID
  -- just make sure \lo_export does not barf
  \lo_export :newloid 'results/lotest2.txt'
+ ERROR:  large object 0 does not exist

While looking at largeobject.source, it is indeed surprising to see
that we use a relative path for this test and not an absolute path.
For the sake of this test's portability, shouldn't we append
@abs_builddir@ then? Attached is a patch to address this issue.

I would imagine that I am not the only one seeing those tests run in
such ways...
Regards,
--
Michael

Michael Paquier <michael.paquier@gmail.com> writes:
> Today a colleague (Kingter Wang) has reminded me about a failure with
> the test case largeobject that can happen if pg_regress is run from an
> external path that has not results/ available at hand:

Um ... what exactly is the triggering condition here?

Your patch doesn't look unreasonable, but I don't have a lot of faith
in one-off fixes.  If this test condition is something we ought to
support, it'd be better if the buildfarm were checking it someplace.

            regards, tom lane

On Wed, Jan 6, 2016 at 11:35 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Michael Paquier <michael.paquier@gmail.com> writes:
>> Today a colleague (Kingter Wang) has reminded me about a failure with
>> the test case largeobject that can happen if pg_regress is run from an
>> external path that has not results/ available at hand:
>
> Um ... what exactly is the triggering condition here?

Perhaps my previous email was not clear enough. Attached is a small
script that emulates what my colleague infrastructure is doing on a
running instance. With this script, only largeobject fails. This could
be easily tackled by appending a cd /to/regress/path to the command
but it seems to me that this just a workaround, and all the other
tests avoid using relative paths with what they do. Even the first
command of largeobject exporting the object uses an absolute path...

> Your patch doesn't look unreasonable, but I don't have a lot of faith
> in one-off fixes.  If this test condition is something we ought to
> support, it'd be better if the buildfarm were checking it someplace.

A module would be needed in this case, the buildfarm client just
bothers about running make check. I guess I could write one and deploy
it on my own nodes, even submit it to Andrew if he thinks that's worth
it.
--
Michael

Michael Paquier <michael.paquier@gmail.com> writes:
> On Wed, Jan 6, 2016 at 11:35 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Um ... what exactly is the triggering condition here?

> Perhaps my previous email was not clear enough. Attached is a small
> script that emulates what my colleague infrastructure is doing on a
> running instance.

I see; so the point is you haven't cd'ed into src/test/regress.

I'm not sure if that's a case we ought to support or not.  In particular,
it's pretty unclear what is the benefit of this script compared to
"make -C $PG_SRC/src/test/regress check".  The script certainly will break
anytime we change the set of arguments the Makefile passes to pg_regress,
which we have done before and no doubt will do again.

Again, I don't see much harm in the patch you propose, it's more a
question of should we contract to support this use-case.  It's not
zero cost to do so; we might for example be able to get rid of some
(in/out)put/*.source files if we didn't need to substitute @abs_builddir@
in them.  Those things are a PITA to maintain, too, so I'm not very
eager to add on more reasons why tests would need them.

            regards, tom lane

On Wed, Jan 6, 2016 at 12:29 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Michael Paquier <michael.paquier@gmail.com> writes:
>> On Wed, Jan 6, 2016 at 11:35 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> Um ... what exactly is the triggering condition here?
>
>> Perhaps my previous email was not clear enough. Attached is a small
>> script that emulates what my colleague infrastructure is doing on a
>> running instance.
>
> I see; so the point is you haven't cd'ed into src/test/regress.

Yep.

> I'm not sure if that's a case we ought to support or not.  In particular,
> it's pretty unclear what is the benefit of this script compared to
> "make -C $PG_SRC/src/test/regress check".  The script certainly will break
> anytime we change the set of arguments the Makefile passes to pg_regress,
> which we have done before and no doubt will do again.

Cross-platform testing is eased a bit in the testing infrastructure I
poke at from time to time: on Windows particularly the stuff I work on
is compiled using MSVC, and bundled within an MSI, several MSI
actually each one with a couple of different characteristics, and the
files of input/output/sql/expected are saved separately, then
redeployed for the tests. Then when testing you can use the same
single code path that kicks pg_regress. There is the burden to change
this code each time the arguments of pg_regress are changed, but this
outweighs the fact that there is no need to go through vcregress.pl
that relies on having the source code around, particularly that we do
as well automated testing of MSI upgrade or things related to Postgres
being integrated in a set of products that have different requirements
and different stability tests. So having the MSI with compiled things
and not the source code itself is a gain in my case, at least I think
so :)

> Again, I don't see much harm in the patch you propose, it's more a
> question of should we contract to support this use-case.  It's not
> zero cost to do so; we might for example be able to get rid of some
> (in/out)put/*.source files if we didn't need to substitute @abs_builddir@
> in them.  Those things are a PITA to maintain, too, so I'm not very
> eager to add on more reasons why tests would need them.

I have no clear answer to that either, it depends on where we want
things to go. Anyway, having both relative and absolute paths in this
test case is clearly a no-win. I agree that we could definitely remove
at least most the @abs_builddir@ markups which is used everywhere
except create_function if we take the direction of minimizing the
input/output files, but still it seems to me that even if this
facilitates a bit the life of maintainers, this has as cost a
reduction of the test portability, and it is not like those files are
actually modified every day.
--
Michael

On Wed, Jan 6, 2016 at 1:26 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Wed, Jan 6, 2016 at 12:29 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Again, I don't see much harm in the patch you propose, it's more a
>> question of should we contract to support this use-case.  It's not
>> zero cost to do so; we might for example be able to get rid of some
>> (in/out)put/*.source files if we didn't need to substitute @abs_builddir@
>> in them.  Those things are a PITA to maintain, too, so I'm not very
>> eager to add on more reasons why tests would need them.
>
> I have no clear answer to that either, it depends on where we want
> things to go. Anyway, having both relative and absolute paths in this
> test case is clearly a no-win. I agree that we could definitely remove
> at least most the @abs_builddir@ markups which is used everywhere
> except create_function if we take the direction of minimizing the
> input/output files, but still it seems to me that even if this
> facilitates a bit the life of maintainers, this has as cost a
> reduction of the test portability, and it is not like those files are
> actually modified every day.

Or in short... I would actually fine to just tell my users to cd into
the folder where the input/output/data/sql/expected files are. At then
end let's remove any inconsistency by either:
1) remove completely abs_builddir and abs_srcdir to reduce the number
of input and output files generated.
2) fix largeobject.source to use absolute paths.
Thoughts?
--
Michael