From: Daniel Gustafsson <daniel@yesql.se>
To: Andres Freund <andres@anarazel.de>
Cc: Peter Eisentraut <peter@eisentraut.org>
Cc: vignesh C <vignesh21@gmail.com>
Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Robert Haas <robertmhaas@gmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Subject: Re: pgsql: Clean up role created in new subscription test.
Date: Wed, 27 Mar 2024 14:42:02 +0100
Message-ID: <6BF8531D-355E-4D41-BA13-653505F97684@yesql.se> (raw)
In-Reply-To: <20240325184824.uxbh6o7w4442o4nv@awork3.anarazel.de>
References: <202312011219.aikuwve3cbku@alvherre.pgsql>
<57373BD5-C0D9-486D-9096-276FE91411D7@yesql.se>
<CALDaNm1eHOMxq0GmZstftKW+W8xxRsG6Ww5u4ncEVq7ZD+NrVw@mail.gmail.com>
<1ECB500B-F76F-4BBE-AE4D-91DDC5E59D7E@yesql.se>
<a6d4b632-e8fd-43e0-91ab-643d1e990941@eisentraut.org>
<20240325184824.uxbh6o7w4442o4nv@awork3.anarazel.de>
> On 25 Mar 2024, at 19:48, Andres Freund <andres@anarazel.de> wrote:
> I don't think it's that, but that the freebsd task tests the installcheck
> equivalent in meson. I haven't checked what your patch is doing, but perhaps
> the issue is that it's seeing global objects concurrently created by another
> test?
Sorry, I had a look when Peter replied a while back but forgot to update this
thread. The reason for the failure is that when running multiple pg_regress in
parallel against a single instance it is impossible to avoid global object
pollution from other tests concurrently executing. Since pg_regress has no
idea about the contents of the tests it also cannot apply any smarts in
filtering out such objects. The CI failures comes from the contrib tests which
run in parallel.
The only option is to make the check opt-in via a command-line parameter for
use it in the main regress tests, and not use it for the contrib tests. The
attached v7 does that and passes CI, but I wonder if it's worth it all with
that restriction?
The 0001 cleanup patch is still relevant (and was found by using this feature)
though but that might be all for this thread.
--
Daniel Gustafsson
Attachments:
[application/octet-stream] v7-0002-pg_regress-Detect-global-objects-left-over-after-.patch (6.3K, ../6BF8531D-355E-4D41-BA13-653505F97684@yesql.se/2-v7-0002-pg_regress-Detect-global-objects-left-over-after-.patch)
download | inline diff:
From 303fd690e09584ac6ae39e0e1bd3bc88e83cfefe Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <dgustafsson@postgresql.org>
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/GNUmakefileindex 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.cindex 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)
=
[application/octet-stream] v7-0001-Drop-global-objects-after-completed-test.patch (1.7K, ../6BF8531D-355E-4D41-BA13-653505F97684@yesql.se/3-v7-0001-Drop-global-objects-after-completed-test.patch)
download | inline diff:
From 78aef85ae769fbdce204e6fa15d482c7fee33e92 Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <dgustafsson@postgresql.org>
Date: Fri, 1 Dec 2023 12:04:49 +0100
Subject: [PATCH v7 1/2] Drop global objects after completed test
Project policy is to not leave global objects behind after a regress
test run.
---
src/test/modules/test_pg_dump/expected/test_pg_dump.out | 2 ++
src/test/modules/test_pg_dump/sql/test_pg_dump.sql | 3 +++
2 files changed, 5 insertions(+)
diff --git a/src/test/modules/test_pg_dump/expected/test_pg_dump.out b/src/test/modules/test_pg_dump/expected/test_pg_dump.outindex f14f3a6664..f57c96aeb9 100644--- a/src/test/modules/test_pg_dump/expected/test_pg_dump.out+++ b/src/test/modules/test_pg_dump/expected/test_pg_dump.out@@ -91,3 +91,5 @@ ALTER EXTENSION test_pg_dump DROP SERVER s0;
ALTER EXTENSION test_pg_dump DROP TABLE test_pg_dump_t1;
ALTER EXTENSION test_pg_dump DROP TYPE test_pg_dump_e1;
ALTER EXTENSION test_pg_dump DROP VIEW test_pg_dump_v1;
+DROP OWNED BY regress_dump_test_role RESTRICT;+DROP ROLE regress_dump_test_role;diff --git a/src/test/modules/test_pg_dump/sql/test_pg_dump.sql b/src/test/modules/test_pg_dump/sql/test_pg_dump.sqlindex a61a7c8c4c..4f1eb9d429 100644--- a/src/test/modules/test_pg_dump/sql/test_pg_dump.sql+++ b/src/test/modules/test_pg_dump/sql/test_pg_dump.sql@@ -106,3 +106,6 @@ ALTER EXTENSION test_pg_dump DROP SERVER s0;
ALTER EXTENSION test_pg_dump DROP TABLE test_pg_dump_t1;
ALTER EXTENSION test_pg_dump DROP TYPE test_pg_dump_e1;
ALTER EXTENSION test_pg_dump DROP VIEW test_pg_dump_v1;
++DROP OWNED BY regress_dump_test_role RESTRICT;+DROP ROLE regress_dump_test_role;--
2.32.1 (Apple Git-133)
=
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-committers@postgresql.org
Cc: daniel@yesql.se, andres@anarazel.de, peter@eisentraut.org, vignesh21@gmail.com, alvherre@alvh.no-ip.org, tgl@sss.pgh.pa.us, robertmhaas@gmail.com, pgsql-hackers@postgresql.org
Subject: Re: pgsql: Clean up role created in new subscription test.
In-Reply-To: <6BF8531D-355E-4D41-BA13-653505F97684@yesql.se>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox