pgsql: Move tablespace path re-creation from the makefiles to pg_regres
Move tablespace path re-creation from the makefiles to pg_regress
Moving this logic into pg_regress fixes a potential failure with
parallel tests when pg_upgrade and the main regression test suite both
trigger the makefile rule that cleaned up testtablespace/ under
src/test/regress. Even if pg_upgrade was triggering this rule, it has
no need to do so as it uses a different tablespace path. So if
pg_upgrade triggered the makefile rule for the tablespace setup while
the main regression test suite ran the tablespace cases, it would fail.
61be85a was a similar attempt at achieving that, but that broke cases
where the regression tests require to run under an Administrator
account, like with Appveyor.
Reported-by: Andres Freund, Kyotaro Horiguchi
Reviewed-by: Peter Eisentraut
Discussion: /messages/by-id/20201209012911.uk4d6nxcnkp7ehrx@alap3.anarazel.de
Branch
------
master
Details
-------
https://git.postgresql.org/pg/commitdiff/6c788d9f6aadb41d76a72d56149268371a7895ee
Modified Files
--------------
src/bin/pg_upgrade/test.sh | 1 -
src/test/regress/GNUmakefile | 21 +++++++--------------
src/test/regress/pg_regress.c | 14 ++++++--------
src/tools/msvc/vcregress.pl | 1 -
4 files changed, 13 insertions(+), 24 deletions(-)
Re: Michael Paquier
Move tablespace path re-creation from the makefiles to pg_regress
Moving this logic into pg_regress fixes a potential failure with
parallel tests when pg_upgrade and the main regression test suite both
trigger the makefile rule that cleaned up testtablespace/ under
src/test/regress. Even if pg_upgrade was triggering this rule, it has
no need to do so as it uses a different tablespace path. So if
pg_upgrade triggered the makefile rule for the tablespace setup while
the main regression test suite ran the tablespace cases, it would fail.61be85a was a similar attempt at achieving that, but that broke cases
where the regression tests require to run under an Administrator
account, like with Appveyor.
This change broke running the testsuite on an existing PG server, if
server user and pg_regress client user are different. This is one of
the tests exercised by Debian's autopkgtest suite.
Previously I could create the tablespace directory, chown it to
postgres, and fire up pg_regress with the correct options. Now
pg_regress wipes that directory, recreates it, and then the server
can't use it because user "postgres" can't write to it.
I'm working around the problem now by running the tests as user
"postgres", but does completely break in environments where users want
to run the testsuite from a separate compilation user but don't have root.
Old code: https://salsa.debian.org/postgresql/postgresql/-/blob/8b1217fcae3e64155bc35517acbd50c6f166d997/debian/tests/installcheck
Workaround: https://salsa.debian.org/postgresql/postgresql/-/blob/cbc0240bec738b6ab3b61c498825b82c8ff21a70/debian/tests/installcheck
Christoph
On Tue, Mar 23, 2021 at 12:50:29PM +0100, Christoph Berg wrote:
I'm working around the problem now by running the tests as user
"postgres", but does completely break in environments where users want
to run the testsuite from a separate compilation user but don't have root.Old code: https://salsa.debian.org/postgresql/postgresql/-/blob/8b1217fcae3e64155bc35517acbd50c6f166d997/debian/tests/installcheck
Workaround: https://salsa.debian.org/postgresql/postgresql/-/blob/cbc0240bec738b6ab3b61c498825b82c8ff21a70/debian/tests/installcheck
So you basically mimicked the makefile rule that this commit removed
into your own test suite. Reverting the change does not really help,
because we'd be back to square one where there would be problems in
parallel runs for developers. Saying that, I would not mind adding an
option to pg_regress to control if this cleanup code is triggered or
not, say something like --no-tablespace-cleanup? Then, you could just
pass down the option by yourself before creating your tablespace path
as you wish.
--
Michael
Re: Michael Paquier
So you basically mimicked the makefile rule that this commit removed
into your own test suite. Reverting the change does not really help,
because we'd be back to square one where there would be problems in
parallel runs for developers. Saying that, I would not mind adding an
option to pg_regress to control if this cleanup code is triggered or
not, say something like --no-tablespace-cleanup? Then, you could just
pass down the option by yourself before creating your tablespace path
as you wish.
I don't think adding more snowflake code just for this use case makes
sense, so I can stick to my workaround.
I just wanted to point out that the only thing preventing the core
testsuite from being run as a true client app is this tablespace
thing, which might be a worthwhile fix on its own.
Maybe creating the tablespace directory from within the testsuite
would suffice?
CREATE TABLE foo (t text);
COPY foo FROM PROGRAM 'mkdir @testtablespace@';
CREATE TABLESPACE regress_tblspace LOCATION '@testtablespace@';
Christoph
On Wed, Mar 24, 2021 at 5:56 AM Christoph Berg <myon@debian.org> wrote:
Maybe creating the tablespace directory from within the testsuite
would suffice?CREATE TABLE foo (t text);
COPY foo FROM PROGRAM 'mkdir @testtablespace@';
CREATE TABLESPACE regress_tblspace LOCATION '@testtablespace@';
Would that work on Windows?
--
Robert Haas
EDB: http://www.enterprisedb.com
On Wed, Mar 24, 2021 at 10:50:50AM -0400, Robert Haas wrote:
On Wed, Mar 24, 2021 at 5:56 AM Christoph Berg <myon@debian.org> wrote:
Maybe creating the tablespace directory from within the testsuite
would suffice?CREATE TABLE foo (t text);
COPY foo FROM PROGRAM 'mkdir @testtablespace@';
CREATE TABLESPACE regress_tblspace LOCATION '@testtablespace@';Would that work on Windows?
I doubt that all the Windows environments would be able to get their
hands on that. And I am not sure either that this would work when it
comes to the CI case, again on Windows.
--
Michael
On Thu, Mar 25, 2021 at 07:44:02AM +0900, Michael Paquier wrote:
On Wed, Mar 24, 2021 at 10:50:50AM -0400, Robert Haas wrote:
On Wed, Mar 24, 2021 at 5:56 AM Christoph Berg <myon@debian.org> wrote:
Maybe creating the tablespace directory from within the testsuite
would suffice?CREATE TABLE foo (t text);
COPY foo FROM PROGRAM 'mkdir @testtablespace@';
CREATE TABLESPACE regress_tblspace LOCATION '@testtablespace@';Would that work on Windows?
That would entail forbidding various shell metacharacters in @testtablespace@.
One could avoid imposing such a restriction by adding a mkdir() function to
regress.c and writing it like:
CREATE FUNCTION mkdir(text)
RETURNS void AS '@libdir@/regress@DLSUFFIX@', 'regress_mkdir'
LANGUAGE C STRICT\;
REVOKE ALL ON FUNCTION mkdir FROM public;
SELECT mkdir('@testtablespace@');
CREATE TABLESPACE regress_tblspace LOCATION '@testtablespace@';
I doubt that all the Windows environments would be able to get their
hands on that.
And I am not sure either that this would work when it
comes to the CI case, again on Windows.
How might a CI provider break that?
On Wed, Mar 24, 2021 at 07:56:59PM -0700, Noah Misch wrote:
That would entail forbidding various shell metacharacters in @testtablespace@.
One could avoid imposing such a restriction by adding a mkdir() function to
regress.c and writing it like:CREATE FUNCTION mkdir(text)
RETURNS void AS '@libdir@/regress@DLSUFFIX@', 'regress_mkdir'
LANGUAGE C STRICT\;
REVOKE ALL ON FUNCTION mkdir FROM public;
SELECT mkdir('@testtablespace@');
CREATE TABLESPACE regress_tblspace LOCATION '@testtablespace@';
Sounds simple.
I doubt that all the Windows environments would be able to get their
hands on that.And I am not sure either that this would work when it
comes to the CI case, again on Windows.How might a CI provider break that?
I am wondering about potential issues when it comes to create the
base tablespace path from the Postgres backend, while HEAD does it
while relying on a restricted token obtained when starting
pg_regress.
I have compiled a simple patch that uses a SQL function for the base
tablespace directory creation, that I have tested on Linux and MSVC.
To get some coverage with the CF bot, I am adding a CF entry with this
thread.
I am still not sure if people would prefer this approach over what's
on HEAD. So if there are any opinions, please feel free.
--
Michael
Attachments:
regress-mkdir.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/test/regress/input/tablespace.source b/src/test/regress/input/tablespace.source
index c133e73499..0c1a9929c2 100644
--- a/src/test/regress/input/tablespace.source
+++ b/src/test/regress/input/tablespace.source
@@ -1,3 +1,10 @@
+-- Create the tablespace path.
+CREATE FUNCTION mkdir(text)
+ RETURNS void AS '@libdir@/regress@DLSUFFIX@', 'regress_mkdir'
+ LANGUAGE C STRICT;
+SELECT mkdir('@testtablespace@');
+DROP FUNCTION mkdir(text);
+
-- create a tablespace using WITH clause
CREATE TABLESPACE regress_tblspacewith LOCATION '@testtablespace@' WITH (some_nonexistent_parameter = true); -- fail
CREATE TABLESPACE regress_tblspacewith LOCATION '@testtablespace@' WITH (random_page_cost = 3.0); -- ok
diff --git a/src/test/regress/output/tablespace.source b/src/test/regress/output/tablespace.source
index 1bbe7e0323..ea07211d1a 100644
--- a/src/test/regress/output/tablespace.source
+++ b/src/test/regress/output/tablespace.source
@@ -1,3 +1,14 @@
+-- Create the tablespace path.
+CREATE FUNCTION mkdir(text)
+ RETURNS void AS '@libdir@/regress@DLSUFFIX@', 'regress_mkdir'
+ LANGUAGE C STRICT;
+SELECT mkdir('@testtablespace@');
+ mkdir
+-------
+
+(1 row)
+
+DROP FUNCTION mkdir(text);
-- create a tablespace using WITH clause
CREATE TABLESPACE regress_tblspacewith LOCATION '@testtablespace@' WITH (some_nonexistent_parameter = true); -- fail
ERROR: unrecognized parameter "some_nonexistent_parameter"
diff --git a/src/test/regress/pg_regress.c b/src/test/regress/pg_regress.c
index b7d80bd9bb..dbb2e8bea7 100644
--- a/src/test/regress/pg_regress.c
+++ b/src/test/regress/pg_regress.c
@@ -506,23 +506,6 @@ convert_sourcefiles_in(const char *source_subdir, const char *dest_dir, const ch
snprintf(testtablespace, MAXPGPATH, "%s/testtablespace", outputdir);
- /*
- * Clean out the test tablespace dir, or create it if it doesn't exist. On
- * Windows, doing this cleanup here makes possible to run the regression
- * tests as a Windows administrative user account with the restricted
- * token obtained when starting pg_regress.
- */
- if (directory_exists(testtablespace))
- {
- if (!rmtree(testtablespace, true))
- {
- fprintf(stderr, _("\n%s: could not remove test tablespace \"%s\"\n"),
- progname, testtablespace);
- exit(2);
- }
- }
- make_directory(testtablespace);
-
/* finally loop on each file and do the replacement */
for (name = names; *name; name++)
{
diff --git a/src/test/regress/regress.c b/src/test/regress/regress.c
index 1990cbb6a1..a884b5ed71 100644
--- a/src/test/regress/regress.c
+++ b/src/test/regress/regress.c
@@ -18,6 +18,7 @@
#include <math.h>
#include <signal.h>
+#include <sys/stat.h>
#include "access/detoast.h"
#include "access/htup_details.h"
@@ -80,6 +81,43 @@ static void regress_lseg_construct(LSEG *lseg, Point *pt1, Point *pt2);
PG_MODULE_MAGIC;
+static bool
+directory_exists(const char *dir)
+{
+ struct stat st;
+
+ if (stat(dir, &st) != 0)
+ return false;
+ if (S_ISDIR(st.st_mode))
+ return true;
+ return false;
+}
+
+/*
+ * Create a new directory for the tests, to be used for tablespaces.
+ * If this directory exists, clean up its contents and create it again
+ * from scratch.
+ */
+PG_FUNCTION_INFO_V1(regress_mkdir);
+
+Datum
+regress_mkdir(PG_FUNCTION_ARGS)
+{
+ char *dir = text_to_cstring(PG_GETARG_TEXT_PP(0));
+
+ if (!superuser())
+ elog(ERROR, "must be superuser to create paths");
+
+ if (directory_exists(dir))
+ {
+ if (!rmtree(dir, true))
+ elog(ERROR, "could not remove path \"%s\"\n", dir);
+ }
+ if (mkdir(dir, S_IRWXU | S_IRWXG | S_IRWXO) < 0)
+ elog(ERROR, "could not create directory \"%s\": %m", dir);
+
+ PG_RETURN_VOID();
+}
/* return the point where two paths intersect, or NULL if no intersection. */
PG_FUNCTION_INFO_V1(interpt_pp);
On Fri, Apr 09, 2021 at 03:00:31PM +0900, Michael Paquier wrote:
I have compiled a simple patch that uses a SQL function for the base
tablespace directory creation, that I have tested on Linux and MSVC.
I am still not sure if people would prefer this approach over what's
on HEAD. So if there are any opinions, please feel free.
"pg_regress --outputdir" is not a great location for a file or directory
created by a user other than the user running pg_regress. If one does "make
check" and then "make installcheck" against a cluster running as a different
user, the rmtree() will fail, assuming typical umask values. An rmtree() at
the end of the tablespace test would mostly prevent that, but that can't help
in the event of a mid-test crash.
I'm not sure we should support installcheck against a server running as a
different user. If we should support it, then I'd probably look at letting
the caller pass in a server-writable directory. That directory would house
the tablespace instead of outputdir doing so.
On Fri, Apr 09, 2021 at 08:07:10PM -0700, Noah Misch wrote:
"pg_regress --outputdir" is not a great location for a file or directory
created by a user other than the user running pg_regress. If one does "make
check" and then "make installcheck" against a cluster running as a different
user, the rmtree() will fail, assuming typical umask values. An rmtree() at
the end of the tablespace test would mostly prevent that, but that can't help
in the event of a mid-test crash.
Yeah, I really don't think that we need to worry about multi-user
scenarios with pg_regress like that though.
I'm not sure we should support installcheck against a server running as a
different user. If we should support it, then I'd probably look at letting
the caller pass in a server-writable directory. That directory would house
the tablespace instead of outputdir doing so.
But, then, we would be back to the pre-13 position where we'd need to
have something external to pg_regress in charge of cleaning up and
creating the tablespace path, no? That's basically what we want to
avoid with the Makefile rules. I can get that it could be interesting
to be able to pass down a custom path for the test tablespace, but do
we really have a need for that?
It took some time for the CF bot to run the patch of this thread, but
from what I can see the tests are passing on Windows under Cirrus CI:
http://commitfest.cputube.org/michael-paquier.html
So it looks like this could be a different answer.
--
Michael
Re: Michael Paquier
http://commitfest.cputube.org/michael-paquier.html
So it looks like this could be a different answer.
The mkdir() function looks like a sane and clean approach to me.
Christoph
On Mon, Apr 12, 2021 at 02:25:53PM +0900, Michael Paquier wrote:
On Fri, Apr 09, 2021 at 08:07:10PM -0700, Noah Misch wrote:
"pg_regress --outputdir" is not a great location for a file or directory
created by a user other than the user running pg_regress. If one does "make
check" and then "make installcheck" against a cluster running as a different
user, the rmtree() will fail, assuming typical umask values. An rmtree() at
the end of the tablespace test would mostly prevent that, but that can't help
in the event of a mid-test crash.Yeah, I really don't think that we need to worry about multi-user
scenarios with pg_regress like that though.
Christoph Berg's first message on this thread reported doing that. If
supporting server_user!=pg_regress_user is unwarranted and Christoph Berg
should stop, then already-committed code suffices.
I'm not sure we should support installcheck against a server running as a
different user. If we should support it, then I'd probably look at letting
the caller pass in a server-writable directory. That directory would house
the tablespace instead of outputdir doing so.But, then, we would be back to the pre-13 position where we'd need to
have something external to pg_regress in charge of cleaning up and
creating the tablespace path, no?
Correct.
That's basically what we want to
avoid with the Makefile rules.
The race that commit 6c788d9 fixed is not inherent to Makefile rules. For
example, you could have instead caused test.sh to issue 'make
installcheck-parallel TABLESPACEDIR="$outputdir"/testtablespace' and have the
makefiles consult $(TABLESPACEDIR) rather than hard-code ./testtablespace.
(That said, I like how 6c788d9 consolidated Windows and non-Windows paths.)
I can get that it could be interesting
to be able to pass down a custom path for the test tablespace, but do
we really have a need for that?
I don't know. I never considered server_user!=pg_regress_user before this
thread, and I don't plan to use it myself. Your latest patch originated to
make that case work, and my last message was reporting that the patch works
for a rather-narrow interpretation of server_user!=pg_regress_user, failing on
variations thereof. That might be fine.
On Mon, Apr 12, 2021 at 10:36:10PM -0700, Noah Misch wrote:
Christoph Berg's first message on this thread reported doing that. If
supporting server_user!=pg_regress_user is unwarranted and Christoph Berg
should stop, then already-committed code suffices.
Not sure that we have ever claimed that. It is unfortunate that this
has broken a case that used to work, perhaps accidentally. At the
same time, Christoph has a workaround for the Debian suite, so it does
not seem like there is anything to do now, anyway.
The race that commit 6c788d9 fixed is not inherent to Makefile rules. For
example, you could have instead caused test.sh to issue 'make
installcheck-parallel TABLESPACEDIR="$outputdir"/testtablespace' and have the
makefiles consult $(TABLESPACEDIR) rather than hard-code ./testtablespace.
(That said, I like how 6c788d9 consolidated Windows and non-Windows paths.)
FWIW, I don't really want to split again this code path across
platforms. Better to have one to rule them all.
I don't know. I never considered server_user!=pg_regress_user before this
thread, and I don't plan to use it myself. Your latest patch originated to
make that case work, and my last message was reporting that the patch works
for a rather-narrow interpretation of server_user!=pg_regress_user, failing on
variations thereof. That might be fine.
Okay. So.. As I am not sure if there is anything that needs to be
acted on here for the moment, I would just close the case. If there
are more voices in favor of the SQL function using mkdir(), I would
not object to use that, as that looks to work for all the cases where
pg_regress is used.
--
Michael
Michael Paquier <michael@paquier.xyz> writes:
Move tablespace path re-creation from the makefiles to pg_regress
So this didn't seem like a problem at the time, but while building
beta1 tarballs I discovered that it leaves behind "testtablespace"
subdirectories in various places where they aren't cleaned by
"make distclean", resulting in scary noise in my diff against the
tarballs:
Only in /home/postgres/pgsql/contrib/dblink: testtablespace
Only in /home/postgres/pgsql/contrib/file_fdw: testtablespace
Only in /home/postgres/pgsql/src/pl/plpgsql/src: testtablespace
This appears to be because pg_regress.c will now create the
tablespace directory in any directory that has an "input"
subdirectory (and that randomness is because somebody saw
fit to drop the code into convert_sourcefiles_in(), where
it surely has no business being, not least because that
means it's run twice).
(BTW, the reason we don't see git complaining about this seems
to be that it doesn't complain about empty subdirectories.)
I think what we want to do is have this code invoked only in
test directories that explicitly ask for it, say with a new
"--make-testtablespace" switch for pg_regress.
regards, tom lane
I wrote:
I think what we want to do is have this code invoked only in
test directories that explicitly ask for it, say with a new
"--make-testtablespace" switch for pg_regress.
Say, as attached. (Windows part is untested.)
regards, tom lane
Attachments:
fix-testtablespace-creation.patchtext/x-diff; charset=us-ascii; name=fix-testtablespace-creation.patchDownload
diff --git a/src/test/regress/GNUmakefile b/src/test/regress/GNUmakefile
index 5dc4bbcb00..fe6e0c98aa 100644
--- a/src/test/regress/GNUmakefile
+++ b/src/test/regress/GNUmakefile
@@ -119,7 +119,8 @@ submake-contrib-spi: | submake-libpgport submake-generated-headers
## Run tests
##
-REGRESS_OPTS = --dlpath=. --max-concurrent-tests=20 $(EXTRA_REGRESS_OPTS)
+REGRESS_OPTS = --dlpath=. --max-concurrent-tests=20 --make-testtablespace-dir \
+ $(EXTRA_REGRESS_OPTS)
check: all
$(pg_regress_check) $(REGRESS_OPTS) --schedule=$(srcdir)/parallel_schedule $(MAXCONNOPT) $(EXTRA_TESTS)
diff --git a/src/test/regress/pg_regress.c b/src/test/regress/pg_regress.c
index b7d80bd9bb..5918e8b412 100644
--- a/src/test/regress/pg_regress.c
+++ b/src/test/regress/pg_regress.c
@@ -504,25 +504,9 @@ convert_sourcefiles_in(const char *source_subdir, const char *dest_dir, const ch
if (!directory_exists(outdir_sub))
make_directory(outdir_sub);
+ /* We might need to replace @testtablespace@ */
snprintf(testtablespace, MAXPGPATH, "%s/testtablespace", outputdir);
- /*
- * Clean out the test tablespace dir, or create it if it doesn't exist. On
- * Windows, doing this cleanup here makes possible to run the regression
- * tests as a Windows administrative user account with the restricted
- * token obtained when starting pg_regress.
- */
- if (directory_exists(testtablespace))
- {
- if (!rmtree(testtablespace, true))
- {
- fprintf(stderr, _("\n%s: could not remove test tablespace \"%s\"\n"),
- progname, testtablespace);
- exit(2);
- }
- }
- make_directory(testtablespace);
-
/* finally loop on each file and do the replacement */
for (name = names; *name; name++)
{
@@ -601,6 +585,32 @@ convert_sourcefiles(void)
convert_sourcefiles_in("output", outputdir, "expected", "out");
}
+/*
+ * Clean out the test tablespace dir, or create it if it doesn't exist.
+ *
+ * On Windows, doing this cleanup here makes it possible to run the
+ * regression tests under a Windows administrative user account with the
+ * restricted token obtained when starting pg_regress.
+ */
+static void
+prepare_testtablespace_dir(void)
+{
+ char testtablespace[MAXPGPATH];
+
+ snprintf(testtablespace, MAXPGPATH, "%s/testtablespace", outputdir);
+
+ if (directory_exists(testtablespace))
+ {
+ if (!rmtree(testtablespace, true))
+ {
+ fprintf(stderr, _("\n%s: could not remove test tablespace \"%s\"\n"),
+ progname, testtablespace);
+ exit(2);
+ }
+ }
+ make_directory(testtablespace);
+}
+
/*
* Scan resultmap file to find which platform-specific expected files to use.
*
@@ -2062,6 +2072,7 @@ help(void)
printf(_(" (default is 0, meaning unlimited)\n"));
printf(_(" --max-concurrent-tests=N maximum number of concurrent tests in schedule\n"));
printf(_(" (default is 0, meaning unlimited)\n"));
+ printf(_(" --make-testtablespace-dir create testtablespace directory\n"));
printf(_(" --outputdir=DIR place output files in DIR (default \".\")\n"));
printf(_(" --schedule=FILE use test ordering schedule from FILE\n"));
printf(_(" (can be used multiple times to concatenate)\n"));
@@ -2116,10 +2127,12 @@ regression_main(int argc, char *argv[],
{"load-extension", required_argument, NULL, 22},
{"config-auth", required_argument, NULL, 24},
{"max-concurrent-tests", required_argument, NULL, 25},
+ {"make-testtablespace-dir", no_argument, NULL, 26},
{NULL, 0, NULL, 0}
};
bool use_unix_sockets;
+ bool make_testtablespace_dir = false;
_stringlist *sl;
int c;
int i;
@@ -2245,6 +2258,9 @@ regression_main(int argc, char *argv[],
case 25:
max_concurrent_tests = atoi(optarg);
break;
+ case 26:
+ make_testtablespace_dir = true;
+ break;
default:
/* getopt_long already emitted a complaint */
fprintf(stderr, _("\nTry \"%s -h\" for more information.\n"),
@@ -2297,6 +2313,9 @@ regression_main(int argc, char *argv[],
unlimit_core_size();
#endif
+ if (make_testtablespace_dir)
+ prepare_testtablespace_dir();
+
if (temp_instance)
{
FILE *pg_conf;
diff --git a/src/tools/msvc/vcregress.pl b/src/tools/msvc/vcregress.pl
index 1852c34109..35e8f67f01 100644
--- a/src/tools/msvc/vcregress.pl
+++ b/src/tools/msvc/vcregress.pl
@@ -118,6 +118,7 @@ sub installcheck_internal
"--bindir=../../../$Config/psql",
"--schedule=${schedule}_schedule",
"--max-concurrent-tests=20",
+ "--make-testtablespace-dir",
"--encoding=SQL_ASCII",
"--no-locale");
push(@args, $maxconn) if $maxconn;
@@ -152,6 +153,7 @@ sub check
"--bindir=",
"--schedule=${schedule}_schedule",
"--max-concurrent-tests=20",
+ "--make-testtablespace-dir",
"--encoding=SQL_ASCII",
"--no-locale",
"--temp-instance=./tmp_check");
On Mon, May 17, 2021 at 05:51:54PM -0400, Tom Lane wrote:
I wrote:
I think what we want to do is have this code invoked only in
test directories that explicitly ask for it, say with a new
"--make-testtablespace" switch for pg_regress.Say, as attached. (Windows part is untested.)
Thanks. I was going to produce something this morning, but you have
been faster than me.
One thing that's changing in this patch is that testtablespace would
be created even if the input directory does not exist when using
--make-testtablespace-dir. I would have kept the creation of the
tablespace path within convert_sourcefiles_in() for this reason.
Worth noting that snprintf() is used twice instead of once to build
the tablespace path string. The Windows part works correctly.
--
Michael
Michael Paquier <michael@paquier.xyz> writes:
On Mon, May 17, 2021 at 05:51:54PM -0400, Tom Lane wrote:
Say, as attached. (Windows part is untested.)
One thing that's changing in this patch is that testtablespace would
be created even if the input directory does not exist when using
--make-testtablespace-dir.
Yeah, I do not see a reason for there to be a connection. It's not
pg_regress' job to decide whether the testtablespace directory is
needed or not.
Worth noting that snprintf() is used twice instead of once to build
the tablespace path string.
Yeah. I considered making a global variable so that there'd be
just one instance of that, but didn't think it amounted to less
mess in the end.
The Windows part works correctly.
Thanks for testing! I'll push this after the release is tagged.
regards, tom lane
On Mon, May 17, 2021 at 08:57:55PM -0400, Tom Lane wrote:
Michael Paquier <michael@paquier.xyz> writes:
One thing that's changing in this patch is that testtablespace would
be created even if the input directory does not exist when using
--make-testtablespace-dir.Yeah, I do not see a reason for there to be a connection. It's not
pg_regress' job to decide whether the testtablespace directory is
needed or not.
Fine by me. I don't think that's a big deal either way.
Worth noting that snprintf() is used twice instead of once to build
the tablespace path string.Yeah. I considered making a global variable so that there'd be
just one instance of that, but didn't think it amounted to less
mess in the end.
No problem from me here either.
The Windows part works correctly.
Thanks for testing! I'll push this after the release is tagged.
Thanks!
--
Michael