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: https://postgr.es/m/[email protected]

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(at)debian(dot)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(at)debian(dot)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(at)debian(dot)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(at)(dot)
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(at)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(at)(dot)
> 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(at)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

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(at)paquier(dot)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

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(at)paquier(dot)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(at)paquier(dot)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