From: Tom Lane <tgl@sss.pgh.pa.us>
To: Stephen Frost <sfrost@snowman.net>
Cc: pgsql-hackers@postgreSQL.org, Christoph Berg <myon@debian.org>
Subject: Re: Regression tests vs existing users in an installation
Date: Sat, 16 Jul 2016 11:29:15 -0400
Message-ID: <24950.1468682955@sss.pgh.pa.us> (raw)
In-Reply-To: <20160716144800.GZ4028@tamriel.snowman.net>
References: <16638.1468620817@sss.pgh.pa.us>
<20160716144800.GZ4028@tamriel.snowman.net>
List-Unsubscribe: <mailto:majordomo@postgresql.org?body=unsub%20pgsql-hackers>
Stephen Frost <sfrost@snowman.net> writes:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>> One could certainly argue that these are safe enough because nobody would
>> ever create real roles by those names anyway. I'm not very comfortable
>> with that though; if we believe that, why did we go to the trouble of
>> making sure these cases work?
> Sadly, it's not quite so simple:
> https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=831234
Hah. Well, I have zero interest in supporting "user" as the name of the
bootstrap superuser. Even if that seemed like a good idea, why not also
current_user, session_user, Public, or any other name we might want to use
in the tests? The variant-output-files answer definitely doesn't scale.
What seems a more useful approach is for packagers to not allow the O/S
username to affect the results of "make check". initdb already has the
--username switch to override its choice of the bootstrap superuser name,
and pg_regress has a --user switch, so in principle it should be possible
for a package to ensure that its build tests are run with the standard
superuser name rather than something dependent on the environment. I'm
not sure how *easy* it is, mind you. We might want to add some Makefile
plumbing to make it easier to do that.
But that's not what I'm on about today ...
>> A more aggressive answer would be to decide we don't need these test cases
>> at all and drop them. An advantage of that is that then we could
>> configure some buildfarm animal to fail the next time somebody ignores
>> the "test role names should contain 'regress'" rule.
> I'd really like to have more test coverage rather than less, so I'd find
> this a bit hard to swallow, but it might still be better than the
> alternatives.
As Greg mentioned, we could improve things if we were willing to invent
something like a "regression_test_mode" GUC. The rough sketch would be:
1. pg_regress adds "regression_test_mode = on" to the ALTER DATABASE SET
settings it already applies to created databases.
2. One of the effects of the GUC would be to throw an error (or warning?)
for creation of disallowed-by-convention role names.
3. The rolenames test could turn off the GUC during creation of these
specific test names. Or if we make it report WARNING not ERROR, we don't
even have to do that, just include the warnings in the expected output.
I find myself liking this idea, because there would be other uses for such
a GUC. One thing that is awful darn tempting right now is to get rid of
the "force_parallel_mode = regress" wart, making that variable back into
a plain bool, and instead have that behavior emerge from having both
force_parallel_mode and regression_test_mode turned on.
We'd still want creation of these specific role names to be wrapped in
a rolled-back transaction, so that we could document that only role
names beginning with "regress_" are at hazard from "make installcheck".
But I don't think that doing that really represents any meaningful loss
of coverage.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tgl@sss.pgh.pa.us, sfrost@snowman.net, myon@debian.org
Subject: Re: Regression tests vs existing users in an installation
In-Reply-To: <24950.1468682955@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox