Обсуждение: Re: pgsql: Move tablespace path re-creation from the makefiles to pg_regres

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

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