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

[ 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

diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index f13dce9..8cc7d2a 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -357,6 +357,11 @@ CreateSubscription(CreateSubscriptionStmt *stmt, bool isTopLevel)
                 (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
                  (errmsg("must be superuser to create subscriptions"))));

+#ifdef ENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS
+    if (strncmp(stmt->subname, "regress_", 8) != 0)
+        elog(WARNING, "subscriptions created by regression test cases must be named regress_xxx");
+#endif
+
     rel = table_open(SubscriptionRelationId, RowExclusiveLock);

     /* Check if name is used */
diff --git a/src/backend/commands/tablespace.c b/src/backend/commands/tablespace.c
index 5e43867..209b114 100644
--- a/src/backend/commands/tablespace.c
+++ b/src/backend/commands/tablespace.c
@@ -307,6 +307,11 @@ CreateTableSpace(CreateTableSpaceStmt *stmt)
                         stmt->tablespacename),
                  errdetail("The prefix \"pg_\" is reserved for system tablespaces.")));

+#ifdef ENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS
+    if (strncmp(stmt->tablespacename, "regress_", 8) != 0)
+        elog(WARNING, "tablespaces created by regression test cases must be named regress_xxx");
+#endif
+
     /*
      * Check that there is no other tablespace by this name.  (The unique
      * index would catch this anyway, but might as well give a friendlier
@@ -957,6 +962,11 @@ RenameTableSpace(const char *oldname, const char *newname)
                  errmsg("unacceptable tablespace name \"%s\"", newname),
                  errdetail("The prefix \"pg_\" is reserved for system tablespaces.")));

+#ifdef ENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS
+    if (strncmp(newname, "regress_", 8) != 0)
+        elog(WARNING, "tablespaces created by regression test cases must be named regress_xxx");
+#endif
+
     /* Make sure the new name doesn't exist */
     ScanKeyInit(&entry[0],
                 Anum_pg_tablespace_spcname,
diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c
index ccc586d..db3bc63 100644
--- a/src/backend/commands/user.c
+++ b/src/backend/commands/user.c
@@ -326,6 +326,11 @@ CreateRole(ParseState *pstate, CreateRoleStmt *stmt)
                         stmt->role),
                  errdetail("Role names starting with \"pg_\" are reserved.")));

+#ifdef ENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS
+    if (strncmp(stmt->role, "regress_", 8) != 0)
+        elog(WARNING, "roles created by regression test cases must be named regress_xxx");
+#endif
+
     /*
      * Check the pg_authid relation to be certain the role doesn't already
      * exist.
@@ -1212,6 +1217,11 @@ RenameRole(const char *oldname, const char *newname)
                         newname),
                  errdetail("Role names starting with \"pg_\" are reserved.")));

+#ifdef ENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS
+    if (strncmp(newname, "regress_", 8) != 0)
+        elog(WARNING, "roles created by regression test cases must be named regress_xxx");
+#endif
+
     /* make sure the new name doesn't exist */
     if (SearchSysCacheExists1(AUTHNAME, CStringGetDatum(newname)))
         ereport(ERROR,
diff --git a/src/backend/replication/logical/origin.c b/src/backend/replication/logical/origin.c
index ff4d54d..d24e59f 100644
--- a/src/backend/replication/logical/origin.c
+++ b/src/backend/replication/logical/origin.c
@@ -249,6 +249,11 @@ replorigin_create(char *roname)
     SysScanDesc scan;
     ScanKeyData key;

+#ifdef ENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS
+    if (strncmp(roname, "regress_", 8) != 0)
+        elog(WARNING, "replication origins created by regression test cases must be named regress_xxx");
+#endif
+
     roname_d = CStringGetTextDatum(roname);

     Assert(IsTransactionState());