Thread
Commits
-
Create by default sql/ and expected/ for output directory in pg_regress
- e78900afd217 14.0 landed
-
BUG #16484: pg_regress fails with --outputdir parameter
The Post Office <noreply@postgresql.org> — 2020-06-08T05:45:30Z
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)) -
Re: BUG #16484: pg_regress fails with --outputdir parameter
Daniel Gustafsson <daniel@yesql.se> — 2020-06-08T09:09:42Z
> 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
-
Re: BUG #16484: pg_regress fails with --outputdir parameter
Michael Paquier <michael@paquier.xyz> — 2020-06-09T05:33:56Z
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
-
Re: BUG #16484: pg_regress fails with --outputdir parameter
Tom Lane <tgl@sss.pgh.pa.us> — 2020-06-09T05:55:06Z
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
-
Re: BUG #16484: pg_regress fails with --outputdir parameter
Daniel Gustafsson <daniel@yesql.se> — 2020-06-09T08:28:32Z
> 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 -
Re: BUG #16484: pg_regress fails with --outputdir parameter
Michael Paquier <michael@paquier.xyz> — 2020-06-11T08:27:36Z
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 -
Re: BUG #16484: pg_regress fails with --outputdir parameter
Daniel Gustafsson <daniel@yesql.se> — 2020-06-11T09:59:20Z
> 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
-
Re: BUG #16484: pg_regress fails with --outputdir parameter
Michael Paquier <michael@paquier.xyz> — 2020-06-13T05:12:01Z
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