Re: Regression tests vs existing users in an installation - Mailing list pgsql-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