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

Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Jun 25, 2019 at 11:33 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Comments?

> LGTM.

Thanks for looking!

> s/must/should/ ?

Sure, if you like.

Further on the rolenames test mess: I started to work on removing
that script's creation of out-of-spec user names, but my heart just
sank to the floor when I noticed that it was also doing stuff like
this:

ALTER USER ALL SET application_name to 'SLAP';
ALTER USER ALL RESET application_name;

The extent to which that's Not OK inside a production installation
is hard to overstate.

At the same time, I can see that we'd want to have some coverage
for that code path, so just deleting those tests isn't attractive.

So I think the only real solution is to cordon off this test script
in some environment where it won't get run by "make installcheck".
I thought briefly about recasting it as a TAP test, but that looked
like a huge amount of make-work.

What I propose that we do instead is invent an empty "module" under
src/test/modules/ and install rolenames as a test for that.  We
already have this in src/test/modules/README:

    src/test/modules contains PostgreSQL extensions that are primarily or
    entirely intended for testing PostgreSQL and/or to serve as example
    code. The extensions here aren't intended to be installed in a
    production server and aren't suitable for "real work".

So I think we could just extend that verbiage to insist that neither "make
install" nor "make installcheck" are good ideas against production
servers.  Perhaps like

    Furthermore, while you can do "make install" and "make installcheck"
    in this directory or its children, it is HIGHLY NOT ADVISED to do so
    with a server containing valuable data.  Some of these tests may have
    undesirable side-effects on roles or other global objects within the
    tested server.

Defining things this way also makes it a non-problem that
src/test/modules/test_pg_dump creates global objects and doesn't drop
them.

(src/test/Makefile is already on board with this definition.)

Now, this doesn't in itself fix the problem that my proposed patch will
emit warnings about the rolenames test script creating "Public" and so on.
We could fix that by maintaining a variant expected-file that includes
those warnings, but probably a less painful answer is just to jack
client_min_messages up to ERROR for that short segment of the test script.

We could make the new subdirectory be something specific like
"src/test/modules/test_rolenames", but I think very likely we'll be
wanting some additional test scripts that we likewise deem unsafe to
run during "installcheck".  So I'd rather choose a more generic module
name, but I'm not sure what ... "unsafe_tests"?

Comments?

            regards, tom lane