Обсуждение: BUG #16484: pg_regress fails with --outputdir parameter

The following bug has been logged on the website:

Bug reference:      16484
Logged by:          Roman Zharkov
Email address:      r.zharkov@postgrespro.ru
PostgreSQL version: 13beta1
Operating system:   linux, windows
Description:

Hello,
pg_regress fails when i try to change the output directory with
"--outputdir" parameter.
This happens bacause pg_regress creates the output directory, but doesn't
create sql and expected subdirectories.

Here is an example:

$ ./pg_regress int8 --dlpath=.
(using postmaster on Unix socket, default port)
============== dropping database "regression"         ==============
DROP DATABASE
============== creating database "regression"         ==============
CREATE DATABASE
ALTER DATABASE
============== running regression test queries        ==============
test int8                         ... ok          122 ms

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

$ ./pg_regress int8 --dlpath=. --outputdir=~regress_output
(using postmaster on Unix socket, default port)
pg_regress: could not open file "~regress_output/sql/largeobject.sql" for
writing: Нет такого файла или каталога
$ mkdir ~regress_output/sql
$ ./pg_regress int8 --dlpath=. --outputdir=~regress_output
(using postmaster on Unix socket, default port)
pg_regress: could not open file
"~regress_output/expected/create_function_2.out" for writing: Нет такого
файла или каталога
$ mkdir ~regress_output/expected
$ ./pg_regress int8 --dlpath=. --outputdir=~regress_output
(using postmaster on Unix socket, default port)
============== dropping database "regression"         ==============
DROP DATABASE
============== creating database "regression"         ==============
CREATE DATABASE
ALTER DATABASE
============== running regression test queries        ==============
test int8                         ... ok          174 ms

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

This patch can fix the issue:

diff --git a/src/test/regress/pg_regress.c b/src/test/regress/pg_regress.c
index 38b2b1e8e1b..1eb1122d237 100644
--- a/src/test/regress/pg_regress.c
+++ b/src/test/regress/pg_regress.c
@@ -465,6 +465,7 @@ convert_sourcefiles_in(const char *source_subdir, const
char *dest_dir, const ch
 {
     char        testtablespace[MAXPGPATH];
     char        indir[MAXPGPATH];
+    char        output_subdir[MAXPGPATH];
     struct stat st;
     int            ret;
     char      **name;
@@ -473,6 +474,11 @@ convert_sourcefiles_in(const char *source_subdir, const
char *dest_dir, const ch
 
     snprintf(indir, MAXPGPATH, "%s/%s", inputdir, source_subdir);
 
+    /* Create dest_subdir directory if it not exists */
+    snprintf(output_subdir, MAXPGPATH, "%s/%s", dest_dir, dest_subdir);
+    if (!directory_exists(output_subdir))
+        make_directory(output_subdir);
+
     /* Check that indir actually exists and is a directory */
     ret = stat(indir, &st);
     if (ret != 0 || !S_ISDIR(st.st_mode))

> On 8 Jun 2020, at 07:45, PG Bug reporting form <noreply@postgresql.org> wrote:

> Hello,
> pg_regress fails when i try to change the output directory with
> "--outputdir" parameter.
> This happens bacause pg_regress creates the output directory, but doesn't
> create sql and expected subdirectories.

Nice catch, testing the attached patch does indeed make pg_regress run without
errors if outputdir doesn't exist.

While looking at it though, I noticed that we nearby use stat() in a single
place while the rest use check_directory().  Is there a reason to not use
check_directory consistently as per the attached diff?

cheers ./daniel

On Mon, Jun 08, 2020 at 11:09:42AM +0200, Daniel Gustafsson wrote:
> Nice catch, testing the attached patch does indeed make pg_regress run without
> errors if outputdir doesn't exist.

No objections to that.  That seems more useful than having to recreate
them before running any custom script specifying --outputdir.

> While looking at it though, I noticed that we nearby use stat() in a single
> place while the rest use check_directory().  Is there a reason to not use
> check_directory consistently as per the attached diff?

Indeed.  And two extra things are that you can just simplify
src/bin/pg_upgrade/test.sh and src/tools/msvc/vcregress.pl so as they
don't create manually those repositories.  This report is not really
annoying for the average user in my opinion, so there may be little
point in backpatching this stuff.

And this leads me to the attached.
--
Michael

Michael Paquier <michael@paquier.xyz> writes:
> And this leads me to the attached.

That will create the output directory even if indir doesn't exist, which
seems likely to lead to bogus directories hanging around in places where
we don't need them --- plus it seems rather ugly to drop this into the
middle of the sequence concerned with indir.  So shouldn't the new code
be after the exit for indir not existing?

            regards, tom lane

> On 9 Jun 2020, at 07:55, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Michael Paquier <michael@paquier.xyz> writes:
>> And this leads me to the attached.

FWIW I agree with the extended cleanup you proposed but I also agree that it
doesn't seem worth to backpatch.

> That will create the output directory even if indir doesn't exist, which
> seems likely to lead to bogus directories hanging around in places where
> we don't need them --- plus it seems rather ugly to drop this into the
> middle of the sequence concerned with indir.  So shouldn't the new code
> be after the exit for indir not existing?

Makes sense, +1 for moving it after indir processing.

Looking at the diff, I noticed this hunk:

--- a/src/tools/msvc/vcregress.pl
+++ b/src/tools/msvc/vcregress.pl
@@ -571,8 +571,6 @@ sub upgradecheck
     my $outputdir          = "$tmp_root/regress";
     my @EXTRA_REGRESS_OPTS = ("--outputdir=$outputdir");
     mkdir "$outputdir"                || die $!;
-    mkdir "$outputdir/sql"            || die $!;
-    mkdir "$outputdir/expected"       || die $!;
     mkdir "$outputdir/testtablespace" || die $!;

On Windows we don't want testtablespace to exist whilst on non-Windows we do.
I wonder if this is a logical copy-paste from test.sh which could be removed as
well?

cheers ./daniel

On Tue, Jun 09, 2020 at 10:28:32AM +0200, Daniel Gustafsson wrote:
>> On 9 Jun 2020, at 07:55, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> FWIW I agree with the extended cleanup you proposed but I also agree that it
> doesn't seem worth to backpatch.

Thanks.

>> That will create the output directory even if indir doesn't exist, which
>> seems likely to lead to bogus directories hanging around in places where
>> we don't need them --- plus it seems rather ugly to drop this into the
>> middle of the sequence concerned with indir.  So shouldn't the new code
>> be after the exit for indir not existing?
>
> Makes sense, +1 for moving it after indir processing.

Makes sense.  Done this way in the updated version attached.

> Looking at the diff, I noticed this hunk:
>
> --- a/src/tools/msvc/vcregress.pl
> +++ b/src/tools/msvc/vcregress.pl
> @@ -571,8 +571,6 @@ sub upgradecheck
>      my $outputdir          = "$tmp_root/regress";
>      my @EXTRA_REGRESS_OPTS = ("--outputdir=$outputdir");
>      mkdir "$outputdir"                || die $!;
> -    mkdir "$outputdir/sql"            || die $!;
> -    mkdir "$outputdir/expected"       || die $!;
>      mkdir "$outputdir/testtablespace" || die $!;
>
> On Windows we don't want testtablespace to exist whilst on non-Windows we do.
> I wonder if this is a logical copy-paste from test.sh which could be removed as
> well?

Yes, we could remove this part in vcregress.pl.  And as you visibly
noted the cleanup of testtablespace is taken care of in
src/test/regress/GNUmakefile for the non-Windows case, while for MSVC
we do this work in pg_regress.c.  I have left this path creation
intentionally actually because there is a patch to improve this area
in the works and we have to check this area of the code so leaving it
behind makes grepping for testtablespace harder to forget (did not get
back to it yet, but I am planning to do so):
https://www.postgresql.org/message-id/20200219.142519.437573253063431435.horikyota.ntt@gmail.com
--
Michael

> On 11 Jun 2020, at 10:27, Michael Paquier <michael@paquier.xyz> wrote:

> I have left this path creation intentionally actually because there is a patch
> to improve this area in the works and we have to check this area of the code so
> leaving it behind makes grepping for testtablespace harder to forget

Aha, I hadn't seen that patch, but I agree it makes sense to leave it for now
then.  +1 on the attached.

cheers ./daniel

On Thu, Jun 11, 2020 at 11:59:20AM +0200, Daniel Gustafsson wrote:
> Aha, I hadn't seen that patch, but I agree it makes sense to leave it for now
> then.  +1 on the attached.

Thanks.  I have been through the patch once again, and applied it to
HEAD.  Worth noting that we have the same number of sql/ and expected/
repositories in the tree with check-world before and after this patch
(1 extra sql/ path is generated, 4 extra ones for expected/).
--
Michael