From 303fd690e09584ac6ae39e0e1bd3bc88e83cfefe Mon Sep 17 00:00:00 2001 From: Daniel Gustafsson Date: Fri, 1 Dec 2023 12:10:17 +0100 Subject: [PATCH v7 2/2] pg_regress: Detect global objects left over after test The project policy is that regress tests should not leave any global objects behind, but there was no check enforcing this. pg_regress will now extract a list of roles, tablespaces and subscriptions before and after running the test(s), and then compares the lists to ensure no new objects are present. Discussion: https://postgr.es/m/CA+TgmoZ+K2OU0Ojs8_4SgqPWJJq7e0XiLd-uJGZ11R_WKF1URQ@mail.gmail.com --- src/test/regress/GNUmakefile | 2 +- src/test/regress/pg_regress.c | 98 +++++++++++++++++++++++++++++++++++ 2 files changed, 99 insertions(+), 1 deletion(-) diff --git a/src/test/regress/GNUmakefile b/src/test/regress/GNUmakefile index 9003435aab..1f774693f4 100644 --- a/src/test/regress/GNUmakefile +++ b/src/test/regress/GNUmakefile @@ -115,7 +115,7 @@ REGRESS_OPTS = --dlpath=. --max-concurrent-tests=20 \ $(EXTRA_REGRESS_OPTS) check: all - $(pg_regress_check) $(REGRESS_OPTS) --schedule=$(srcdir)/parallel_schedule $(MAXCONNOPT) $(EXTRA_TESTS) + $(pg_regress_check) $(REGRESS_OPTS) --globals-check --schedule=$(srcdir)/parallel_schedule $(MAXCONNOPT) $(EXTRA_TESTS) check-tests: all | temp-install $(pg_regress_check) $(REGRESS_OPTS) $(MAXCONNOPT) $(TESTS) $(EXTRA_TESTS) diff --git a/src/test/regress/pg_regress.c b/src/test/regress/pg_regress.c index 76f01c73f5..c59b6b4964 100644 --- a/src/test/regress/pg_regress.c +++ b/src/test/regress/pg_regress.c @@ -120,6 +120,7 @@ static char *dlpath = PKGLIBDIR; static char *user = NULL; static _stringlist *extraroles = NULL; static char *config_auth_datadir = NULL; +static bool globals_check = false; /* internal variables */ static const char *progname; @@ -132,6 +133,8 @@ static char sockself[MAXPGPATH]; static char socklock[MAXPGPATH]; static StringInfo failed_tests = NULL; static bool in_note = false; +static _stringlist *globals_before = NULL; +static _stringlist *globals_after = NULL; static _resultmap *resultmap = NULL; @@ -244,6 +247,57 @@ split_to_stringlist(const char *s, const char *delim, _stringlist **listhead) free(sc); } +/* + * Query the cluster and save the first column of the results into a + * stringlist. + */ +static void +query_to_stringlist(const char *dbname, const char *query, _stringlist **listhead) +{ + PGconn *conn; + PGresult *res; + StringInfo buf = makeStringInfo(); + + appendStringInfo(buf, "dbname=%s", dbname); + + conn = PQconnectdb(buf->data); + if (PQstatus(conn) != CONNECTION_OK) + bail("unable to connect to cluster: %s", PQerrorMessage(conn)); + + res = PQexec(conn, query); + if (PQresultStatus(res) != PGRES_TUPLES_OK) + bail("unable to query database: %s", PQerrorMessage(conn)); + + for (int i = 0; i < PQntuples(res); i++) + add_stringlist_item(listhead, pstrdup(PQgetvalue(res, i, 0))); + + pfree(buf->data); + PQclear(res); + PQfinish(conn); +} + +/* + * Compare two stringlists for string equality + */ +static bool +diff_stringlist(_stringlist *a, _stringlist *b) +{ + _stringlist *sla = a; + _stringlist *slb = b; + + for (; sla && slb; sla = sla->next, slb = slb->next) + { + if (strcmp(sla->str, slb->str) != 0) + return false; + } + + /* If we reached the end of both lists at the same time, they are equal */ + if (sla == NULL && slb == NULL) + return true; + + return false; +} + /* * Bailing out is for unrecoverable errors which prevents further testing to * occur and after which the test run should be aborted. By passing noatexit @@ -2041,6 +2095,7 @@ help(void) printf(_(" --dlpath=DIR look for dynamic libraries in DIR\n")); printf(_(" --encoding=ENCODING use ENCODING as the encoding\n")); printf(_(" --expecteddir=DIR take expected files from DIR (default \".\")\n")); + printf(_(" --globals-check fail if global objects are left after test\n")); printf(_(" -h, --help show this help, then exit\n")); printf(_(" --inputdir=DIR take input files from DIR (default \".\")\n")); printf(_(" --launcher=CMD use CMD as launcher of psql\n")); @@ -2105,6 +2160,7 @@ regression_main(int argc, char *argv[], {"config-auth", required_argument, NULL, 24}, {"max-concurrent-tests", required_argument, NULL, 25}, {"expecteddir", required_argument, NULL, 26}, + {"globals-check", no_argument, NULL, 27}, {NULL, 0, NULL, 0} }; @@ -2233,6 +2289,9 @@ regression_main(int argc, char *argv[], case 26: expecteddir = pg_strdup(optarg); break; + case 27: + globals_check = true; + break; default: /* getopt_long already emitted a complaint */ pg_log_error_hint("Try \"%s --help\" for more information.", @@ -2612,6 +2671,21 @@ regression_main(int argc, char *argv[], create_role(sl->str, dblist); } + /* + * Store the global objects before the test starts such that we can check + * for any objects left behind after the tests finish. + */ + if (globals_check) + { + query_to_stringlist("postgres", + "(SELECT rolname AS obj FROM pg_catalog.pg_roles ORDER BY 1) " + "UNION ALL " + "(SELECT spcname AS obj FROM pg_catalog.pg_tablespace ORDER BY 1) " + "UNION ALL " + "(SELECT subname AS obj FROM pg_catalog.pg_subscription ORDER BY 1)", + &globals_before); + } + /* * Ready to run the tests */ @@ -2625,6 +2699,30 @@ regression_main(int argc, char *argv[], run_single_test(sl->str, startfunc, postfunc); } + /* + * Store the global objects again after the tests have finished and + * compare with the list from before the tests. + */ + if (globals_check) + { + query_to_stringlist("postgres", + "(SELECT rolname AS obj FROM pg_catalog.pg_roles ORDER BY 1) " + "UNION ALL " + "(SELECT spcname AS obj FROM pg_catalog.pg_tablespace ORDER BY 1) " + "UNION ALL " + "(SELECT subname AS obj FROM pg_catalog.pg_subscription ORDER BY 1)", + &globals_after); + + if (!diff_stringlist(globals_before, globals_after)) + { + test_status_failed("global_objects", 0.0, false); + } + else + { + test_status_ok("global_objects", 0.0, false); + } + } + /* * Shut down temp installation's postmaster */ -- 2.32.1 (Apple Git-133)