Re: Regression tests vs existing users in an installation - Mailing list pgsql-hackers

Tom,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> We've talked before about how the regression tests should be circumspect
> about what role names they create/drop, so as to avoid possibly blowing
> up an installation's existing users during "make installcheck".  In
> particular I believe there was consensus that such names should begin
> with, or at least include, "regress".  I got around today to instrumenting
> CreateRole to see what names we were actually creating, and was quite
> depressed as to how thoroughly that guideline is being ignored (see
> attached).

I had started in on this but hadn't made as much progress as I had
hoped, so I'm glad to see that you're taking a look at it.

> I propose to go through the regression tests and fix this (in HEAD only).
> However, there's one place where it's not so easy to just substitute a
> different name, because rolenames.sql is intentionally doing this:
>
> CREATE ROLE "Public";
> CREATE ROLE "None";
> CREATE ROLE "current_user";
> CREATE ROLE "session_user";
> CREATE ROLE "user";
>
> in order to test whether we properly distinguish role-related keywords
> from quoted identifiers.  Obviously, modifying these would defeat the
> point of the test.
>
> 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

Note that Christoph went ahead and closed out the bug report as it was
just an issue in the regression test and not unexpected behavior, but if
we're going to do something in this area then we may wish to consider
fixing the case that caused that bug report.

> What I'm inclined to do with this is to reduce the test to be something
> like
>
> BEGIN;
> CREATE ROLE "Public";
> CREATE ROLE "None";
> CREATE ROLE "current_user";
> CREATE ROLE "session_user";
> CREATE ROLE "user";
> ROLLBACK;
>
> with maybe a couple of ALTERs and GRANTs inside the transaction to verify
> that the names can be referenced as well as created.  This would be safe
> against modifying any conflicting existing users; the only bad consequence
> would be a phony failure of the test.

Unfortunately, the above wouldn't fix the case where someone attempts to
run the regression tests as a Unix user named "user".

One suggestion discussed on the bug report was to include different
result files, but that does seem pretty special-cased and overkill,
though if the overall set of tests in rolenames.sql is reduced then
perhaps it isn't as much of an issue.  Then again, how many different
result files would be needed to cover all cases?  Seems like there could
be quite a few, though, with this specific case, it looks like we'd at
least only have to deal with one for each role which is allowed to exist
already (such as "user"), without any multiplicative factors (can't run
the regression test as more than one Unix user at a time).

> 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.

Thanks!

Stephen