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

In commit 18555b132 we tentatively established a rule that regression
tests should use names containing "regression" for databases, and names
starting with "regress_" for all other globally-visible object names, so
as to circumscribe the side-effects that "make installcheck" could have
on an existing installation.

This commit adds a simple enforcement mechanism for that rule: if the code
is compiled with ENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS defined, it
will emit a warning (not an error) whenever a database, role, tablespace,
subscription, or replication origin name is created that doesn't obey the
rule.  Running one or more buildfarm members with that symbol defined
should be enough to catch new violations, at least in the regular
regression tests.  Most TAP tests wouldn't notice such warnings, but
that's actually fine because TAP tests don't execute against an existing
server anyway.

Since it's already the case that running src/test/modules/ tests in
installcheck mode is deprecated, we can use that as a home for tests
that seem unsafe to run against an existing server, such as tests that
might have side-effects on existing roles.  Document that (though this
commit doesn't in itself make it any less safe than before).

Update regress.sgml to define these restrictions more clearly, and
to clean up assorted lack-of-up-to-date-ness in its descriptions of
the available regression tests.

Discussion: https://postgr.es/m/16638.1468620817@sss.pgh.pa.us

diff --git a/doc/src/sgml/regress.sgml b/doc/src/sgml/regress.sgml
index 448e2d19f7270e7245c10693d023638629fa4a02..7b68213266840278d5a00d50a9532f2740c837f1 100644 (file)
--- a/doc/src/sgml/regress.sgml
+++ b/doc/src/sgml/regress.sgml
@@ -47,7 +47,7 @@ make check
 
 
 =======================
- All 115 tests passed.
+ All 193 tests passed.
 =======================
 
 
@@ -98,7 +98,7 @@ make MAX_CONNECTIONS=10 check
 
   
    To run the tests after installation (see ),
-   initialize a data area and start the
+   initialize a data directory and start the
    server as explained in , then type:
 
 make installcheck
@@ -116,10 +116,10 @@ make installcheck-parallel
 
   
    The tests will also transiently create some cluster-wide objects, such as
-   roles and tablespaces.  These objects will have names beginning with
-   regress_.  Beware of using installcheck
-   mode in installations that have any actual users or tablespaces named
-   that way.
+   roles, tablespaces, and subscriptions.  These objects will have names
+   beginning with regress_.  Beware of
+   using installcheck mode with an installation that has
+   any actual global objects named that way.
   
   
 
@@ -130,7 +130,7 @@ make installcheck-parallel
    The make check and make installcheck commands
    run only the core regression tests, which test built-in
    functionality of the PostgreSQL server.  The source
-   distribution also contains additional test suites, most of them having
+   distribution contains many additional test suites, most of them having
    to do with add-on functionality such as optional procedural languages.
   
 
@@ -146,9 +146,24 @@ make installcheck-world
    already-installed server, respectively, just as previously explained
    for make check and make installcheck.  Other
    considerations are the same as previously explained for each method.
-   Note that make check-world builds a separate temporary
-   installation tree for each tested module, so it requires a great deal
-   more time and disk space than make installcheck-world.
+   Note that make check-world builds a separate instance
+   (temporary data directory) for each tested module, so it requires more
+   time and disk space than make installcheck-world.
+  
+
+  
+   On a modern machine with multiple CPU cores and no tight operating-system
+   limits, you can make things go substantially faster with parallelism.
+   The recipe that most PostgreSQL developers actually use for running all
+   tests is something like
+
+make check-world -j8 >/dev/null
+
+   with a -j limit near to or a bit more than the number
+   of available cores.  Discarding stdout
+   eliminates chatter that's not interesting when you just want to verify
+   success.  (In case of failure, the stderr
+   messages are usually enough to determine where to look closer.)
   
 
   
@@ -166,8 +181,7 @@ make installcheck-world
   
    
     
-     Regression tests for optional procedural languages (other than
-     PL/pgSQL, which is tested by the core tests).
+     Regression tests for optional procedural languages.
      These are located under src/pl.
     
    
@@ -184,6 +198,13 @@ make installcheck-world
      located in src/interfaces/ecpg/test.
     
    
+   
+    
+     Tests for core-supported authentication methods,
+     located in src/test/authentication.
+     (See below for additional authentication-related tests.)
+    
+   
    
     
      Tests stressing behavior of concurrent sessions,
@@ -192,21 +213,36 @@ make installcheck-world
    
    
     
-     Tests of client programs under src/bin.  See
-     also .
+     Tests for crash recovery and physical replication,
+     located in src/test/recovery.
+    
+   
+   
+    
+     Tests for logical replication,
+     located in src/test/subscription.
+    
+   
+   
+    
+     Tests of client programs, located under src/bin.
     
    
   
 
   
-   When using installcheck mode, these tests will destroy any
-   existing databases named pl_regression,
-   contrib_regression, isolation_regression,
-   ecpg1_regression, or ecpg2_regression, as well as
-   regression.
+   When using installcheck mode, these tests will create
+   and destroy test databases whose names
+   include regression, for
+   example pl_regression
+   or contrib_regression.  Beware of
+   using installcheck mode with an installation that has
+   any non-test databases named that way.
   
 
   
+   Some of these auxiliary test suites use the TAP infrastructure explained
+   in .
    The TAP-based tests are run only when PostgreSQL was configured with the
    option --enable-tap-tests.  This is recommended for
    development, but can be omitted if there is no suitable Perl installation.
@@ -259,6 +295,17 @@ make check-world PG_TEST_EXTRA='kerberos ldap ssl'
    configuration are not run even if they are mentioned in
    PG_TEST_EXTRA.
   
+
+  
+   In addition, there are tests in src/test/modules
+   which will be run by make check-world but not
+   by make installcheck-world.  This is because they
+   install non-production extensions or have other side-effects that are
+   considered undesirable for a production installation.  You can
+   use make install and make
+   installcheck in one of those subdirectories if you wish,
+   but it's not recommended to do so with a non-test server.
+  
   
 
   
@@ -737,6 +784,26 @@ make check PROVE_TESTS='t/001_test1.pl t/003_test3.pl'
     The TAP tests require the Perl module IPC::Run.
     This module is available from CPAN or an operating system package.
    
+
+   
+    Generically speaking, the TAP tests will test the executables in a
+    previously-installed installation tree if you say make
+    installcheck, or will build a new local installation tree from
+    current sources if you say make check.  In either
+    case they will initialize a local instance (data directory) and
+    transiently run a server in it.  Some of these tests run more than one
+    server.  Thus, these tests can be fairly resource-intensive.
+   
+
+   
+    It's important to realize that the TAP tests will start test server(s)
+    even when you say make installcheck; this is unlike
+    the traditional non-TAP testing infrastructure, which expects to use an
+    already-running test server in that case.  Some PostgreSQL
+    subdirectories contain both traditional-style and TAP-style tests,
+    meaning that make installcheck will produce a mix of
+    results from temporary servers and the already-running test server.
+   
   
 
   

diff --git a/src/backend/commands/alter.c b/src/backend/commands/alter.c
index 9229fe1a456ea3c1dc2197d05cc2d3d60cf0d1e9..70dbcb0756c3ab369cf1e279c4b3692b09699b46 100644 (file)
--- a/src/backend/commands/alter.c
+++ b/src/backend/commands/alter.c
@@ -274,6 +274,12 @@ AlterObjectRename_internal(Relation rel, Oid objectId, const char *new_name)
        if (SearchSysCacheExists2(SUBSCRIPTIONNAME, MyDatabaseId,
                                  CStringGetDatum(new_name)))
            report_name_conflict(classId, new_name);
+
+       /* Also enforce regression testing naming rules, if enabled */
+#ifdef ENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS
+       if (strncmp(new_name, "regress_", 8) != 0)
+           elog(WARNING, "subscriptions created by regression test cases should have names starting with \"regress_\"");
+#endif
    }
    else if (nameCacheId >= 0)
    {

diff --git a/src/backend/commands/dbcommands.c b/src/backend/commands/dbcommands.c
index 15207bf75a1766b55301e3c13f895dcfe09f2a38..863f89f19d24189e2d183e28d1c00319e2ce4af2 100644 (file)
--- a/src/backend/commands/dbcommands.c
+++ b/src/backend/commands/dbcommands.c
@@ -470,6 +470,16 @@ createdb(ParseState *pstate, const CreatedbStmt *stmt)
        /* Note there is no additional permission check in this path */
    }
 
+   /*
+    * If built with appropriate switch, whine when regression-testing
+    * conventions for database names are violated.  But don't complain during
+    * initdb.
+    */
+#ifdef ENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS
+   if (IsUnderPostmaster && strstr(dbname, "regression") == NULL)
+       elog(WARNING, "databases created by regression test cases should have names including \"regression\"");
+#endif
+
    /*
     * Check for db name conflict.  This is just to give a more friendly error
     * message than "unique index violation".  There's a race condition but
@@ -1008,6 +1018,15 @@ RenameDatabase(const char *oldname, const char *newname)
                (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
                 errmsg("permission denied to rename database")));
 
+   /*
+    * If built with appropriate switch, whine when regression-testing
+    * conventions for database names are violated.
+    */
+#ifdef ENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS
+   if (strstr(newname, "regression") == NULL)
+       elog(WARNING, "databases created by regression test cases should have names including \"regression\"");
+#endif
+
    /*
     * Make sure the new name doesn't exist.  See notes for same error in
     * CREATE DATABASE.

diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index f13dce90a116a614fc91ba3575b1c8c5a75b2f79..2e67a5889e581d5f89d3bf2a20b44802b81b3244 100644 (file)
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -357,6 +357,15 @@ CreateSubscription(CreateSubscriptionStmt *stmt, bool isTopLevel)
                (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
                 (errmsg("must be superuser to create subscriptions"))));
 
+   /*
+    * If built with appropriate switch, whine when regression-testing
+    * conventions for subscription names are violated.
+    */
+#ifdef ENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS
+   if (strncmp(stmt->subname, "regress_", 8) != 0)
+       elog(WARNING, "subscriptions created by regression test cases should have names starting with \"regress_\"");
+#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 5e43867e6f95b3fa1efbb6a2056b11bd12a2ad1e..502736be1aa0188938918ccac2f8cd54941cb31d 100644 (file)
--- a/src/backend/commands/tablespace.c
+++ b/src/backend/commands/tablespace.c
@@ -307,6 +307,15 @@ CreateTableSpace(CreateTableSpaceStmt *stmt)
                        stmt->tablespacename),
                 errdetail("The prefix \"pg_\" is reserved for system tablespaces.")));
 
+   /*
+    * If built with appropriate switch, whine when regression-testing
+    * conventions for tablespace names are violated.
+    */
+#ifdef ENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS
+   if (strncmp(stmt->tablespacename, "regress_", 8) != 0)
+       elog(WARNING, "tablespaces created by regression test cases should have names starting with \"regress_\"");
+#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 +966,15 @@ RenameTableSpace(const char *oldname, const char *newname)
                 errmsg("unacceptable tablespace name \"%s\"", newname),
                 errdetail("The prefix \"pg_\" is reserved for system tablespaces.")));
 
+   /*
+    * If built with appropriate switch, whine when regression-testing
+    * conventions for tablespace names are violated.
+    */
+#ifdef ENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS
+   if (strncmp(newname, "regress_", 8) != 0)
+       elog(WARNING, "tablespaces created by regression test cases should have names starting with \"regress_\"");
+#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 ccc586d8e854bb4058745937e1d25237528d0f79..aab5aa855d2851222dc7738c7575ad805f6bce94 100644 (file)
--- a/src/backend/commands/user.c
+++ b/src/backend/commands/user.c
@@ -326,6 +326,15 @@ CreateRole(ParseState *pstate, CreateRoleStmt *stmt)
                        stmt->role),
                 errdetail("Role names starting with \"pg_\" are reserved.")));
 
+   /*
+    * If built with appropriate switch, whine when regression-testing
+    * conventions for role names are violated.
+    */
+#ifdef ENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS
+   if (strncmp(stmt->role, "regress_", 8) != 0)
+       elog(WARNING, "roles created by regression test cases should have names starting with \"regress_\"");
+#endif
+
    /*
     * Check the pg_authid relation to be certain the role doesn't already
     * exist.
@@ -1212,6 +1221,15 @@ RenameRole(const char *oldname, const char *newname)
                        newname),
                 errdetail("Role names starting with \"pg_\" are reserved.")));
 
+   /*
+    * If built with appropriate switch, whine when regression-testing
+    * conventions for role names are violated.
+    */
+#ifdef ENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS
+   if (strncmp(newname, "regress_", 8) != 0)
+       elog(WARNING, "roles created by regression test cases should have names starting with \"regress_\"");
+#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 5bb804cece1d24b3ab0ccc59f73915cb513212ac..681132c922b2470433dcf7a25112880125fa16f0 100644 (file)
--- a/src/backend/replication/logical/origin.c
+++ b/src/backend/replication/logical/origin.c
@@ -1238,6 +1238,15 @@ pg_replication_origin_create(PG_FUNCTION_ARGS)
                        name),
                 errdetail("Origin names starting with \"pg_\" are reserved.")));
 
+   /*
+    * If built with appropriate switch, whine when regression-testing
+    * conventions for replication origin names are violated.
+    */
+#ifdef ENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS
+   if (strncmp(name, "regress_", 8) != 0)
+       elog(WARNING, "replication origins created by regression test cases should have names starting with \"regress_\"");
+#endif
+
    roident = replorigin_create(name);
 
    pfree(name);

diff --git a/src/test/modules/README b/src/test/modules/README
index 99f921d582ac99eefcd96ee87ebf5358c2991d9b..025ecac72436e7410cdf241dbee726098cad7389 100644 (file)
--- a/src/test/modules/README
+++ b/src/test/modules/README
@@ -6,6 +6,13 @@ 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".
 
+Furthermore, while you can do "make install" and "make installcheck" in
+this directory or its children, it is NOT ADVISABLE 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.
+"make installcheck-world" at the top level does not recurse into this
+directory.
+
 Most extensions have their own pg_regress tests or isolationtester specs. Some
 are also used by tests elsewhere in the tree.