Обсуждение: Recent pg_regress changes break testing under SELinux
Who decided this part of pg_regress.c was a good idea?
/* try to create the test tablespace dir if it doesn't exist */snprintf(testtablespace, MAXPGPATH, "%s/testtablespace",
abs_builddir);if(directory_exists(testtablespace)) rmtree(testtablespace, true);make_directory(testtablespace);
The regression test Makefile is responsible for preparing that
directory, not pg_regress. This is important because in the Postgres
RPMs, we have to be careful to appease SELinux:
# The tests command the server to write into testtablespace and results.
# On a SELinux-enabled system this will fail unless we mark those directories
# as writable by the server.
cleandirs:-rm -rf testtablespace resultsmkdir testtablespace results[ -x /usr/bin/chcon ] && /usr/bin/chcon -u system_u
-robject_r -t postgresql_db_t testtablespace results
By blowing away the directory and remaking it, pg_regress causes this
carefully-applied labeling to be lost.
As far as I can see the rmtree/make_directory thrashing is useless, and
I'm going to remove it unless a pretty good counter-argument is made.
regards, tom lane
Tom Lane wrote: > Who decided this part of pg_regress.c was a good idea? > > /* try to create the test tablespace dir if it doesn't exist */ > snprintf(testtablespace, MAXPGPATH, "%s/testtablespace", abs_builddir); > if (directory_exists(testtablespace)) > rmtree(testtablespace, true); > make_directory(testtablespace); Hmm, AFAICS it was applied by me in 1.26, a patch from Magnus apparently. This was quite some time ago! I think I just passed that part verbatim from Magnus' patch. > The regression test Makefile is responsible for preparing that > directory, not pg_regress. This is important because in the Postgres > RPMs, we have to be careful to appease SELinux: Interesting. Certainly I didn't have that in mind when I applied it. > As far as I can see the rmtree/make_directory thrashing is useless, and > I'm going to remove it unless a pretty good counter-argument is made. Fine with me ... -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
Alvaro Herrera <alvherre@commandprompt.com> writes:
> Tom Lane wrote:
>> Who decided this part of pg_regress.c was a good idea?
> Hmm, AFAICS it was applied by me in 1.26, a patch from Magnus
> apparently. This was quite some time ago! I think I just passed that
> part verbatim from Magnus' patch.
Hmm ... I wonder if the unstated agenda was to run the regression tests
without having any supporting Makefile?
I'd be willing to make it #ifdef WIN32, instead of diking it out entirely,
if that's what the problem is. I would prefer to remove it though.
The original design had only the Makefile knowing exactly where the
testtablespace directory lurks, making it changeable in one place,
and also confining the whole business to src/test/regress which is good
because none of the other uses of pg_regress need it. As CVS tip
stands, the location of testtablespace is hard-wired into the pg_regress
executable, and I think it's mostly blind accident that it doesn't
clutter contrib/* and src/pl/* with testtablespace directories.
At this point what I'd really prefer is that pg_regress be completely
uninvolved with testtablespace, which we could manage by getting rid
of @testtablespace@ in the regression test files in favor of
@abs_builddir@/testtablespace. This would mean that both the Makefile
and the regression test files know where the directory lurks, but either
of those are easier to change than a precompiled binary.
Now this isn't gonna work if there's a prerequisite for pg_regress
to execute without any external setup, but surely we don't expect
users to enter all its switch arguments by hand. What's the actual
intended arrangement on Windows ... is there a .BAT script or something
that could make the directory?
regards, tom lane
Tom Lane wrote: > Alvaro Herrera <alvherre@commandprompt.com> writes: >> Tom Lane wrote: >>> Who decided this part of pg_regress.c was a good idea? > >> Hmm, AFAICS it was applied by me in 1.26, a patch from Magnus >> apparently. This was quite some time ago! I think I just passed that >> part verbatim from Magnus' patch. > > Hmm ... I wonder if the unstated agenda was to run the regression tests > without having any supporting Makefile? AFAIK, that was the *stated* agenda. Or rather, a part of the stated agenda. > I'd be willing to make it #ifdef WIN32, instead of diking it out entirely, > if that's what the problem is. I would prefer to remove it though. > The original design had only the Makefile knowing exactly where the > testtablespace directory lurks, making it changeable in one place, > and also confining the whole business to src/test/regress which is good > because none of the other uses of pg_regress need it. As CVS tip > stands, the location of testtablespace is hard-wired into the pg_regress > executable, and I think it's mostly blind accident that it doesn't > clutter contrib/* and src/pl/* with testtablespace directories. Commandline parameter, perhaps? > At this point what I'd really prefer is that pg_regress be completely > uninvolved with testtablespace, which we could manage by getting rid > of @testtablespace@ in the regression test files in favor of > @abs_builddir@/testtablespace. This would mean that both the Makefile > and the regression test files know where the directory lurks, but either > of those are easier to change than a precompiled binary. > > Now this isn't gonna work if there's a prerequisite for pg_regress > to execute without any external setup, but surely we don't expect > users to enter all its switch arguments by hand. What's the actual > intended arrangement on Windows ... is there a .BAT script or something > that could make the directory? Yeah, there could be that. The idea was to be able to run it without the need for mingw - meaning no Makefile support and no shellscript. But .BAT is supported on all Windows. //Magnus
Magnus Hagander <magnus@hagander.net> writes:
> Tom Lane wrote:
>> The original design had only the Makefile knowing exactly where the
>> testtablespace directory lurks, making it changeable in one place,
>> and also confining the whole business to src/test/regress which is good
>> because none of the other uses of pg_regress need it. As CVS tip
>> stands, the location of testtablespace is hard-wired into the pg_regress
>> executable, and I think it's mostly blind accident that it doesn't
>> clutter contrib/* and src/pl/* with testtablespace directories.
> Commandline parameter, perhaps?
I was trying to avoid that, but only because it'd make the patch bigger.
It would be one way to resolve the above gripes.
But I'm still thinking that it's fairly silly to expect Windows users to
type out the equivalent of
./pg_regress --psqldir=$(PSQLDIR) --schedule=$(srcdir)/parallel_schedule --srcdir=$(abs_srcdir)
--multibyte=$(MULTIBYTE)--load-language=plpgsql $(MAXCONNOPT) $(NOLOCALE)
especially when no other platform is that hard. (Admittedly, some of
these switches have usable defaults, but many don't.) So it really
seems like the right answer is a .BAT script, and I'm kinda surprised
to hear there's not one already.
regards, tom lane
Tom Lane wrote: > Magnus Hagander <magnus@hagander.net> writes: >> Tom Lane wrote: >>> The original design had only the Makefile knowing exactly where the >>> testtablespace directory lurks, making it changeable in one place, >>> and also confining the whole business to src/test/regress which is good >>> because none of the other uses of pg_regress need it. As CVS tip >>> stands, the location of testtablespace is hard-wired into the pg_regress >>> executable, and I think it's mostly blind accident that it doesn't >>> clutter contrib/* and src/pl/* with testtablespace directories. > >> Commandline parameter, perhaps? > > I was trying to avoid that, but only because it'd make the patch bigger. > It would be one way to resolve the above gripes. > > But I'm still thinking that it's fairly silly to expect Windows users to > type out the equivalent of > > ./pg_regress --psqldir=$(PSQLDIR) --schedule=$(srcdir)/parallel_schedule --srcdir=$(abs_srcdir) --multibyte=$(MULTIBYTE)--load-language=plpgsql $(MAXCONNOPT) $(NOLOCALE) Yeah, agreed. The original idea was to have a shortcut created by the installer to do that for you. > especially when no other platform is that hard. (Admittedly, some of > these switches have usable defaults, but many don't.) So it really > seems like the right answer is a .BAT script, and I'm kinda surprised > to hear there's not one already. Well, there is a way to run it from the msvc build. The idea was to be able to run after a binary install. Granted it doesn't make that much sense, but it was mostly a "customer feel good" thing. But there's nothing preventing that from being done from a .BAT file instead of a direct shortcut. //Magnus
Magnus Hagander <magnus@hagander.net> writes:
> Yeah, agreed. The original idea was to have a shortcut created by the
> installer to do that for you.
> ...
> Well, there is a way to run it from the msvc build. The idea was to be
> able to run after a binary install. Granted it doesn't make that much
> sense, but it was mostly a "customer feel good" thing. But there's
> nothing preventing that from being done from a .BAT file instead of a
> direct shortcut.
Well, I suppose anything along this line is more code churn than we
should engage in post-RC2. What I propose for now is that we just
"#ifdef WIN32" the directory deletion/remaking in pg_regress.c, with
a suitable comment, and then we can revisit the issue in 8.4 or later.
regards, tom lane
Tom Lane wrote: > Magnus Hagander <magnus@hagander.net> writes: >> Yeah, agreed. The original idea was to have a shortcut created by the >> installer to do that for you. >> ... >> Well, there is a way to run it from the msvc build. The idea was to be >> able to run after a binary install. Granted it doesn't make that much >> sense, but it was mostly a "customer feel good" thing. But there's >> nothing preventing that from being done from a .BAT file instead of a >> direct shortcut. > > Well, I suppose anything along this line is more code churn than we > should engage in post-RC2. What I propose for now is that we just > "#ifdef WIN32" the directory deletion/remaking in pg_regress.c, with > a suitable comment, and then we can revisit the issue in 8.4 or later. Works for me. //Magnus