Horton

Thread

Commits

  1. Move rolenames test out of the core regression tests.

  2. Add an enforcement mechanism for global object names in regression tests.

  3. Fix regression tests to use only global names beginning with "regress_".

  4. Disallow user-created replication origins named "pg_xxx".

  1. Regression tests vs existing users in an installation

    Tom Lane <tgl@sss.pgh.pa.us> — 2016-07-15T22:13:37Z

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

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.

I thought about trying to preserve all the existing test cases while still
keeping these roles inside a transaction, by inserting savepoints around
the intentional failures.  But there are enough intentional failures in
rolenames.sql that that would be really tedious.  The existing test cases
seem enormously duplicative to me anyway, so I think a fairly short
transaction with a few tests would be sufficient to cover this territory.

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.

Comments?

			regards, tom lane


LOG:  created role tablespace_testuser1
LOG:  created role tablespace_testuser2
LOG:  created role regtestrole
LOG:  created role regress_rol_op1
LOG:  created role regress_rol_op3
LOG:  created role regress_rol_op4
LOG:  created role regress_rol_op5
LOG:  created role regress_rol_op6
LOG:  created role regression_reindexuser
LOG:  created role regtest_unpriv_user
LOG:  created role test_def_superuser
LOG:  created role test_superuser
LOG:  created role Public
LOG:  created role None
LOG:  created role current_user
LOG:  created role session_user
LOG:  created role user
LOG:  created role testrol0
LOG:  created role testrolx
LOG:  created role testrol2
LOG:  created role testrol1
LOG:  created role test_def_inherit
LOG:  created role test_inherit
LOG:  created role test_def_createrole
LOG:  created role test_createrole
LOG:  created role test_def_createdb
LOG:  created role test_createdb
LOG:  created role test_def_role_canlogin
LOG:  created role test_role_canlogin
LOG:  created role test_def_user_canlogin
LOG:  created role test_user_canlogin
LOG:  created role test_def_replication
LOG:  created role test_replication
LOG:  created role tu1
LOG:  created role tr1
LOG:  created role tg1
LOG:  created role test_def_bypassrls
LOG:  created role test_bypassrls
LOG:  created role view_user1
LOG:  created role view_user2
LOG:  created role selinto_user
LOG:  created role regtest_addr_user
LOG:  created role regress_rls_alice
LOG:  created role regress_rls_bob
LOG:  created role regress_rls_carol
LOG:  created role regress_rls_exempt_user
LOG:  created role regress_rls_group1
LOG:  created role regress_rls_group2
LOG:  created role regress_rol_lock1
LOG:  created role regressuser1
LOG:  created role regressuser2
LOG:  created role regressuser3
LOG:  created role regressuser4
LOG:  created role regressuser5
LOG:  created role regressgroup1
LOG:  created role regressgroup2
LOG:  created role seclabel_user1
LOG:  created role seclabel_user2
LOG:  created role schemauser1
LOG:  created role schemauser2
LOG:  renamed role schemauser2 to schemauser_renamed
LOG:  created role locktable_user
LOG:  created role regress_rls_eve
LOG:  created role regress_rls_frank
LOG:  created role regress_rls_dob_role1
LOG:  created role regress_rls_dob_role2
LOG:  created role regress_user_mvtest
LOG:  created role regtest_alter_user3
LOG:  created role regtest_alter_user2
LOG:  created role regtest_alter_user1
LOG:  created role regtest_alter_user
LOG:  created role regtest_alter_user5
LOG:  created role regtest_alter_user6
LOG:  created role regression_user
LOG:  created role regression_user2
LOG:  created role regression_user3
LOG:  created role regression_group
LOG:  created role foreign_data_user
LOG:  created role regress_test_role
LOG:  created role regress_test_role2
LOG:  created role regress_test_role_super
LOG:  created role regress_test_indirect
LOG:  created role unprivileged_role
LOG:  created role regression_user0
LOG:  created role regression_user1
LOG:  created role regression_user2
LOG:  created role temp_reset_user
LOG:  created role clstr_user
LOG:  created role regress_alice
LOG:  created role conversion_test_user
LOG:  created role regresslo
LOG:  created role seq_user
LOG:  created role regression_bob
LOG:  created role dblink_regression_test
LOG:  created role file_fdw_superuser
LOG:  created role file_fdw_user
LOG:  created role no_priv_user
LOG:  created role regress_view_owner

  2. Re: Regression tests vs existing users in an installation

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2016-07-16T03:54:56Z

    Tom Lane 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 propose to go through the regression tests and fix this (in HEAD only).

I would propose that we have one test run near the beginning or right at
the beginning of the serial schedule that sets up a small number of
roles to cover most of the needs of every other test, so that most such
other tests do not need to create any roles at all.  (Of course, there
would be a few cases where this would defeat the purpose of the test
because creating or dropping the role is precisely what is being
created; those cases would have additional roles, with the proposed
prefix.)

So currently we have 97 roles?  Probably we can get away with a dozen or
so, maybe even less than that.

> 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.
>
> I thought about trying to preserve all the existing test cases while still
> keeping these roles inside a transaction, by inserting savepoints around
> the intentional failures.  But there are enough intentional failures in
> rolenames.sql that that would be really tedious.  The existing test cases
> seem enormously duplicative to me anyway, so I think a fairly short
> transaction with a few tests would be sufficient to cover this territory.


> A more aggressive answer would be to decide we don't need these test cases
> at all and drop them.

Hmm ... I think a blanket removal would go against generally accepted
principle that our tests need to cover more functionality.

Maybe we did go overboard on that one and the rolled-back creation is
enough test for that functionality.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  3. Re: Regression tests vs existing users in an installation

    Michael Paquier <michael.paquier@gmail.com> — 2016-07-16T11:59:30Z

    On Sat, Jul 16, 2016 at 7:13 AM, 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).

Thanks for doing this.

> 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 am -1 for dropping the tests. We could just have a CFLAGS that adds
an elog(ERROR) in CreateRole and checks that the created role has a
wanted prefix, or have a plugin that uses the utility hook to do this
filtering.
-- 
Michael

  4. Re: Regression tests vs existing users in an installation

    Greg Stark <stark@mit.edu> — 2016-07-16T12:19:58Z

    On 16 Jul 2016 12:59 pm, "Michael Paquier" <michael.paquier@gmail.com>
wrote:
>

> Thanks for doing this.

+1

Though I might highlight this as the kind of issue that a bug tracker would
help avoid falling through the cracks and make visible to newcomers.

> I am -1 for dropping the tests. We could just have a CFLAGS that adds
> an elog(ERROR) in CreateRole and checks that the created role has a
> wanted prefix, or have a plugin that uses the utility hook to do this
> filtering.

If we make a hidden regression_test_safety GUC then we could have
pg_regress enable it and have these specific tests disable it explicitly
with comments on why it's safe.

It might even be handy for other people writing application regression
tests depending on what other things it blocked.

A hook might even be possible to use the same way. pg_regress would have to
build and install a .so which might be tricky.

  5. Re: Regression tests vs existing users in an installation

    Stephen Frost <sfrost@snowman.net> — 2016-07-16T14:48:01Z

    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

  6. Re: Regression tests vs existing users in an installation

    Tom Lane <tgl@sss.pgh.pa.us> — 2016-07-16T15:29:15Z

    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

  7. Re: Regression tests vs existing users in an installation

    Tom Lane <tgl@sss.pgh.pa.us> — 2016-07-16T15:38:34Z

    Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Tom Lane 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 would propose that we have one test run near the beginning or right at
> the beginning of the serial schedule that sets up a small number of
> roles to cover most of the needs of every other test, so that most such
> other tests do not need to create any roles at all.

I don't think that's a very attractive idea.  It would create hazards for
concurrent test cases, I fear.  Moreover, an un-enforced convention of
"don't create roles" isn't really any safer than an un-enforced convention
of "only create roles named thus-and-such"; it just takes one person who
is not familiar with the convention, and one committer not paying close
attention, and we're right back where we started.

I'm coming to the conclusion that the only thing that will make this
materially better in the long run is automatic enforcement of a convention
about what role names may be created in the regression tests.  See my
response to Stephen just now for a concrete proposal.

			regards, tom lane

  8. Re: Regression tests vs existing users in an installation

    Tom Lane <tgl@sss.pgh.pa.us> — 2016-07-17T22:46:30Z

    I've gone ahead and pushed a patch that does all of the cosmetic renamings
needed to clean up the global-object-names situation.  I've not done
anything yet about those special cases in the rolenames test, since it's
open for discussion exactly what to do there.  I figured that this patch
was bulky enough, and mechanical enough, that there wasn't much point in
putting it up for review; the buildfarm will do a lot better at finding
any mistakes I may have made.

			regards, tom lane

  9. Re: Regression tests vs existing users in an installation

    Robert Haas <robertmhaas@gmail.com> — 2016-07-18T01:37:58Z

    On Sat, Jul 16, 2016 at 11:38 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I'm coming to the conclusion that the only thing that will make this
> materially better in the long run is automatic enforcement of a convention
> about what role names may be created in the regression tests.  See my
> response to Stephen just now for a concrete proposal.

We could also do this by loading a C module during the regression
tests, which seems maybe less ugly than adding a GUC.

I don't particularly like your suggestion of spooky action at a
distance between force_parallel_mode and regression_test_mode.  That
just seems kooky.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  10. Re: Regression tests vs existing users in an installation

    Michael Paquier <michael.paquier@gmail.com> — 2016-07-18T05:34:50Z

    On Mon, Jul 18, 2016 at 10:37 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Sat, Jul 16, 2016 at 11:38 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> We could also do this by loading a C module during the regression
> tests, which seems maybe less ugly than adding a GUC.
> I don't particularly like your suggestion of spooky action at a
> distance between force_parallel_mode and regression_test_mode.  That
> just seems kooky.

One downside of the plugin is that any users willing to do make
installcheck would need to install it as well.
-- 
Michael

  11. Re: Regression tests vs existing users in an installation

    Tom Lane <tgl@sss.pgh.pa.us> — 2016-07-18T15:13:02Z

    Robert Haas <robertmhaas@gmail.com> writes:
> On Sat, Jul 16, 2016 at 11:38 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I'm coming to the conclusion that the only thing that will make this
>> materially better in the long run is automatic enforcement of a convention
>> about what role names may be created in the regression tests.  See my
>> response to Stephen just now for a concrete proposal.

> We could also do this by loading a C module during the regression
> tests, which seems maybe less ugly than adding a GUC.

Meh, I'm not convinced.  As Michael points out, arranging for such a
module to get loaded in an installcheck context would be difficult ---
maybe not impossible, but complicated.  Also, we'd have to add hook
function calls in all the places it would need to get control; most
of those places would probably be one-off hooks with no other conceivable
use.  And we'd still need to have a GUC, because I think it's inevitable
that we'd need to be able to turn off the restrictions for specific
tests.  So that seems like a lot of work and complication just to make
a GUC be custom to some undocumented extension rather than built-in.
If we had no other debugging GUCs then there might be some point in
rejecting this one, but we have a bunch:
https://www.postgresql.org/docs/devel/static/runtime-config-developer.html

> I don't particularly like your suggestion of spooky action at a
> distance between force_parallel_mode and regression_test_mode.  That
> just seems kooky.

It's certainly a judgment call as to which way is cleaner, but I don't
understand your objection.  There are plenty of ways in which multiple
GUCs determine a given behavior already.  Also, breaking this behavior
into two variables would let us document the user-useful behavior (do
this to test parallel safety of functions) in a different place from the
developer-useful behavior (do this to make EXPLAIN lie to you, which
surely has no possible use except for regression testing).

Possibly a single "regression_test_mode" variable is a bad idea and
we should instead have distinct developer-oriented GUCs for each special
behavior we decide we need.  I'm not particularly set on that, but
to me it seems like less of a mess to have just one.

			regards, tom lane

  12. Re: Regression tests vs existing users in an installation

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2016-07-18T16:47:56Z

    On 7/15/16 6:13 PM, Tom Lane 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".

I'm not particularly sure that that is a useful goal anymore.  Setting
up a new instance is cheap, so if users are concerned, they should not
run the tests against their important instance.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  13. Re: Regression tests vs existing users in an installation

    Tom Lane <tgl@sss.pgh.pa.us> — 2016-07-18T16:58:45Z

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> On 7/15/16 6:13 PM, Tom Lane 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".

> I'm not particularly sure that that is a useful goal anymore.  Setting
> up a new instance is cheap, so if users are concerned, they should not
> run the tests against their important instance.

To my mind, the main point of "make installcheck" is to verify that
your actual installation, not some other instance, is working right.
This is far from a trivial issue; for instance on a SELinux machine,
you need to be able to verify that the installed policy allows the
DB to work, and that is very likely to be path-sensitive.

So this remains an important requirement to me.  It's true that it might
be something you need to run only right after making the installation ---
but that doesn't mean the DB is empty.  Consider wanting to test a freshly
pg_upgrade'd installation, for example.

			regards, tom lane

  14. Re: Regression tests vs existing users in an installation

    Robert Haas <robertmhaas@gmail.com> — 2016-07-21T16:26:35Z

    On Mon, Jul 18, 2016 at 1:34 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> One downside of the plugin is that any users willing to do make
> installcheck would need to install it as well.

Not really.  If the only purpose of the plugin is to verify that we're
not creating regression users whose names don't start with "regress",
it should be good enough to run it for "make check" but not for "make
installcheck".  It's not there to test functionality, just to verify
that we've followed our own rules for regression tests.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  15. Re: Regression tests vs existing users in an installation

    Robert Haas <robertmhaas@gmail.com> — 2016-07-21T16:42:33Z

    On Mon, Jul 18, 2016 at 11:13 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I don't particularly like your suggestion of spooky action at a
>> distance between force_parallel_mode and regression_test_mode.  That
>> just seems kooky.
>
> It's certainly a judgment call as to which way is cleaner, but I don't
> understand your objection.  There are plenty of ways in which multiple
> GUCs determine a given behavior already.  Also, breaking this behavior
> into two variables would let us document the user-useful behavior (do
> this to test parallel safety of functions) in a different place from the
> developer-useful behavior (do this to make EXPLAIN lie to you, which
> surely has no possible use except for regression testing).

There are certainly cases where the behavior that you get depends on
multiple GUCs.  For example, the vacuum cost limit stuff is like that,
and so are the cost factors which control the query planner.  But in
each of those cases, each GUC has a function that is fully orthogonal
to each other GUC.  That doesn't seem to be the case here.  You're
saying that force_parallel_mode will decide whether we get behavior A,
and regression_test_mode will decide whether we get behavior B, but if
you ask for both A and B you will also get an additional behavior C
which would not have been selected by either GUC taken alone.  Because
the different settings are now non-orthogonal, there's now no way to
get A and B without C, or A and C without B.

Moreover, I don't want to end up in a situation where
regression_test_mode=on enables a score of minor behavior changes that
can't be separated out and tested individually.  It's true that
checking the names of regression roles is such a very minor thing that
a generic name like regression_test_mode might be OK, with the idea
that anything else similarly minor that comes along later can be
folded into that as well.  But I fear that it will become a crutch: my
code makes the regression test fail.  Instead of writing better tests,
add another thing that's conditional on regression_test_mode!
Eventually, we'll have a bug that the regression tests fail to catch
because regression_test_mode papers over the problem.  We're some
distance from such a situation now, of course, but I bet the
temptation to be undisciplined about hooking more behavior into that
GUC will be almost irresistible for new developers, and in some cases
experienced ones, too.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  16. Re: [HACKERS] Regression tests vs existing users in an installation

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-06-26T03:33:22Z

    [ blast from the past department ]

So, this thread about ensuring the regression tests don't create random
globally-visible names seems to have got lost in the weeds.  I'm going
to resurrect it after noticing that two different places have grown
violations of the rule since I fixed things in 18555b132.

I think we were largely overdesigning the fix.  The right thing is to do
something approximately as attached, and then make at least one buildfarm
critter build with -DENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS.

This proposed patch intentionally emits only WARNINGs, not ERRORS.  This
is so that TAP tests won't fail if the warnings fire.  Since TAP tests
never run against an existing installation, there's no reason for them
to follow the restriction.  Admittedly, this is a pretty ad-hoc way of
getting that end result, but I'm tired of waiting for somebody to
implement a less ad-hoc way.

There are still two things we'd have to argue about before committing
this.  One is the already-discussed-upthread point that rolenames.sql
insists on creating and then deleting users with names like "user".
I remain of the opinion that that's just a damfool idea; there is nearly
zero chance that those test cases will ever catch a bug, and much more
than zero chance that they'll cause problems in some pre-existing
installation.  So my vote is to take them out.

The other is that we've also grown a bunch of tests that create
subscriptions and replication origins with randomly-chosen names.
Since those objects also have globally-visible names, I think the
"name them regress_something" policy should apply to them too, and
the attached patch tries to enforce it.  But of course that causes
a bunch of failures right now.

(While I'm looking at that, I wonder why we don't have a restriction
against "pg_xxx" names for those object types.)

Comments?

			regards, tom lane

  17. Re: [HACKERS] Regression tests vs existing users in an installation

    Robert Haas <robertmhaas@gmail.com> — 2019-06-27T17:31:10Z

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

LGTM.

s/must/should/ ?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  18. Re: [HACKERS] Regression tests vs existing users in an installation

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-06-27T20:50:06Z

    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

  19. Re: [HACKERS] Regression tests vs existing users in an installation

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-06-27T21:21:48Z

    On 2019-Jun-27, Tom Lane wrote:

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

Uh-oh.  I don't remember doing that, but evidently I did :-(

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

Yeah ...

> What I propose that we do instead is invent an empty "module" under
> src/test/modules/ and install rolenames as a test for that.

Hmm, that's an idea, yes.

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

+1

> 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"?

+0 for unsafe_tests, -1 for test_rolenames.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  20. Re: [HACKERS] Regression tests vs existing users in an installation

    Stephen Frost <sfrost@snowman.net> — 2019-06-28T15:41:11Z

    Greetings,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
>     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.

Sounds like a good approach to me and I'm happy that it'd address the
test_pg_dump case too.

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

Seems alright.

> 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"?

Agreed but haven't got any particularly good suggestions on names..

Thanks,

Stephen

  21. Re: [HACKERS] Regression tests vs existing users in an installation

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-06-28T18:42:49Z

    OK, here's a completed patch to add checking for naming-rule violations.

I updated regress.sgml to clarify the naming rules (and failed to
resist the temptation to update a lot of other somewhat-obsolete
statements there, too).

Also worth noting is that I added an IsReservedName check to
pg_replication_origin_create(), ie it will no longer allow
user-selected origin names starting with "pg_".  This seems
like a good idea considering that we generate internal origin
names that look like that.  I have not done anything equivalent
for subscription names, but should we consider doing so?

I touched the TAP tests only to the extent necessary to make them pass
cleanly --- mostly I had to fool with pg_dump/t/010_dump_connstr.pl
because it whines if the restore step emits any warnings.  Perhaps
there's another way to address that with less invasive changes to that
test script; but I couldn't think of one other than ignoring warnings,
which didn't seem like a great idea.

This patch doesn't address the issue that we think rolenames.sql is
unsafe to run in "make installcheck" mode.  That seems like a separable
problem, and we'd have to adjust that script as shown here anyway.

Barring objections, I'll push this shortly (to HEAD only) and turn on
the enabling switch on one or two of my buildfarm critters.

			regards, tom lane

  22. Re: [HACKERS] Regression tests vs existing users in an installation

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-06-29T17:21:48Z

    Stephen Frost <sfrost@snowman.net> writes:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>> 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"?

> Agreed but haven't got any particularly good suggestions on names..

Hearing no better suggestions, I went with "unsafe_tests" in the
attached.

This patch just moves rolenames.sql lock-stock-and-barrel into
src/test/modules/unsafe_tests.  Another approach would be to split
the test script into a portion that doesn't violate any installcheck
rule and could be kept in the core tests, versus the unsafe tests.
I lack the interest to do that, but if somebody else is excited enough
about it, have at it.

I'm wondering whether we ought to back-patch this.  The odds that
somebody would be affected by "make installcheck" resetting the
application_name property of existing roles seem pretty small,
but it could be nasty if it did matter.  Perhaps squeezing this into
v12 is good enough.

Another idea would be to just take out the ALTER USER ALL tests
in the back branches.

Thoughts?

			regards, tom lane

  23. Re: [HACKERS] Regression tests vs existing users in an installation

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2019-11-26T21:10:15Z

    On 2019-06-29 19:21, Tom Lane wrote:
> Stephen Frost<sfrost@snowman.net>  writes:
>> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>>> 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"?
>> Agreed but haven't got any particularly good suggestions on names..
> Hearing no better suggestions, I went with "unsafe_tests" in the
> attached.

I found this directory, and it seems like a good place to add some more 
tests (other thread), but I'm puzzled why it's hidden under modules/, 
since it's not, well, a module.  Why is it not in src/test/?

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  24. Re: [HACKERS] Regression tests vs existing users in an installation

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-11-26T22:32:16Z

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> I found this directory, and it seems like a good place to add some more 
> tests (other thread), but I'm puzzled why it's hidden under modules/, 
> since it's not, well, a module.  Why is it not in src/test/?

Doing it like this allowed it to be picked up automatically by the
buildfarm script.  If we'd put it in a new src/test directory, the
buildfarm script would have had to be taught about that, and we'd
have no coverage until owners updated their machines.  So it
seemed expedient to do it like this.

			regards, tom lane