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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
