Re: pg_regress cleans up tablespace twice.

Thank you for looking this!

At Fri, 15 May 2020 11:58:55 +0900, Michael Paquier <michael@paquier.xyz> wrote in 
> On Mon, May 11, 2020 at 05:13:54PM +0900, Kyotaro Horiguchi wrote:
> > Tablespace directory cleanup is not done for all testing
> > targets. Actually it is not done for the tools under bin/ except
> > pg_upgrade.
> 
> Let's first take one problem at a time, as I can see that your patch
> 0002 is modifying a portion of what you added in 0001, and so let's
> try to remove this WIN32 stuff from pg_regress.c.

Yes, 0001 and 0001+0002 are alternatives.  They should be merged if we
are going to fix the pg_upgrade test.  I take this as we go on 0001+0002.

> +sub CleanupTablespaceDirectory
> +{
> +   my $tablespace = 'testtablespace';
> +
> +   rmtree($tablespace) if (-e $tablespace);
> +   mkdir($tablespace);
> +}
> This check should use "-d" and not "-e" as it would be true for a file
> as well.  Also, in pg_regress.c, we remove the existing tablespace

That was intentional so that a file with the name don't stop
testing. Actually pg_regress is checking only for a directory in other
place and it's not that bad since no-one can create a file with that
name while running test.  On the other hand, is there any reason for
refraining from removing if it weren't a directory but a file?

Changed to -d in the attached.

> as well.  Also, in pg_regress.c, we remove the existing tablespace
> test directory in --outputdir, which is "." by default but it can be a
> custom one.  Shouldn't you do the same logic in this new routine?  So
> we should have an optional argument for the output directory that
> defaults to `pwd` if not defined, no?  This means passing down the
> argument only for upgradecheck() in vcregress.pl.

I thought of that but didn't in the patch.  I refrained from doing
that because the output directory is dedicatedly created at the only
place (pg_upgrade test) where the --outputdir is specified. (I think I
tend to do too-much.)

It is easy in perl scripts, but rather complex for makefiles. The
attached is using a perl one-liner to extract outputdir from
EXTRA_REGRESS_OPTS. I don't like that but I didn't come up with better
alternatives.  On the other hand ActivePerl (with default
installation) doesn't seem to know Getopt::Long::GetOptions and
friends.  In the attached vcregress.pl parses --outputdir not using
GetOpt::Long...

>  sub isolationcheck
>  {
>  	chdir "../isolation";
> +	CleanupTablespaceDirectory();
>  	copy("../../../$Config/isolationtester/isolationtester.exe",
>  		"../../../$Config/pg_isolation_regress");
>  	my @args = (
> [...]
>  	print "============================================================\n";
>  	print "Checking $module\n";
> +	CleanupTablespaceDirectory();
>  	my @args = (
>  		"$topdir/$Config/pg_regress/pg_regress",
>  		"--bindir=${topdir}/${Config}/psql",
> I would put that just before the system() calls for consistency with
> the rest.

Right. That's just an mistake. Fixed along with subdircheck.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center