From: Daniel Gustafsson <daniel@yesql.se>
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: Peter Eisentraut <peter@eisentraut.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: Fri, 1 Dec 2023 12:22:50 +0100
Message-ID: <E0EB10CE-9B58-44C0-8CBC-807BC8E5A84A@yesql.se> (raw)
In-Reply-To: <202311081232.rn3aho5ay6yx@alvherre.pgsql>
References: <202311081232.rn3aho5ay6yx@alvherre.pgsql>
> On 8 Nov 2023, at 13:32, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> I suppose you're just thinking of using PQexec() or whatever, run one
> query with sufficient ORDER BY, save the result, and at the end of the
> test run just run the same query and compare that they are cell-by-cell
> identical? This sounds a lot simpler than the patch you posted.
I found some spare cycles for this and came up with the attached. The idea was
to keep it in-line with how pg_regress already today manipulate and traverse
_stringlists for various things. With the addition of the 0001 patch to clean
up global objects left in test_pg_dump it passes check-world.
--
Daniel Gustafsson
Attachments:
[application/octet-stream] v4-0002-pg_regress-Detect-global-objects-left-over-after-.patch (4.1K, ../E0EB10CE-9B58-44C0-8CBC-807BC8E5A84A@yesql.se/2-v4-0002-pg_regress-Detect-global-objects-left-over-after-.patch)
download | inline diff:
From 1862409f720019249f22b059eaecc8fa11294c74 Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <dgustafsson@postgresql.org>
Date: Fri, 1 Dec 2023 12:10:17 +0100
Subject: [PATCH v4 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/pg_regress.c | 89 +++++++++++++++++++++++++++++++++++
1 file changed, 89 insertions(+)
diff --git a/src/test/regress/pg_regress.c b/src/test/regress/pg_regress.cindex a9b8246cb7..c30a853c8c 100644--- a/src/test/regress/pg_regress.c+++ b/src/test/regress/pg_regress.c@@ -132,6 +132,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 +246,60 @@ 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)+ {+ diag("left over global object detected: %s", slb->str);+ 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
@@ -2608,6 +2664,18 @@ 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.+ */+ 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
*/
@@ -2621,6 +2689,27 @@ 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.+ */+ 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] v4-0001-Drop-global-objects-after-completed-test.patch (2.5K, ../E0EB10CE-9B58-44C0-8CBC-807BC8E5A84A@yesql.se/3-v4-0001-Drop-global-objects-after-completed-test.patch)
download | inline diff:
From 3ce445ef8bc9d47d8ad45757df8ca3bbc7f1aa78 Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <dgustafsson@postgresql.org>
Date: Fri, 1 Dec 2023 12:04:49 +0100
Subject: [PATCH v4 1/2] Drop global objects after completed test
Project policy is to not leave global objects behind after a regress
test run.
---
.../modules/test_pg_dump/expected/test_pg_dump.out | 9 +++++++++
src/test/modules/test_pg_dump/sql/test_pg_dump.sql | 10 ++++++++++
2 files changed, 19 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..479d7fc898 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,12 @@ 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;
+REVOKE USAGE ON FOREIGN SERVER s0 FROM regress_dump_test_role;+REVOKE SELECT (c1) ON ft1 FROM regress_dump_test_role;+REVOKE SELECT ON ft1 FROM regress_dump_test_role;+REVOKE UPDATE ON test_pg_dump_mv1 FROM regress_dump_test_role;+REVOKE USAGE ON SCHEMA test_pg_dump_s1 FROM regress_dump_test_role;+REVOKE USAGE ON TYPE test_pg_dump_e1 FROM regress_dump_test_role;+REVOKE EXECUTE ON FUNCTION test_pg_dump(int) FROM regress_dump_test_role;+DROP EXTENSION test_pg_dump;+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..164d6408ee 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,13 @@ 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;
++REVOKE USAGE ON FOREIGN SERVER s0 FROM regress_dump_test_role;+REVOKE SELECT (c1) ON ft1 FROM regress_dump_test_role;+REVOKE SELECT ON ft1 FROM regress_dump_test_role;+REVOKE UPDATE ON test_pg_dump_mv1 FROM regress_dump_test_role;+REVOKE USAGE ON SCHEMA test_pg_dump_s1 FROM regress_dump_test_role;+REVOKE USAGE ON TYPE test_pg_dump_e1 FROM regress_dump_test_role;+REVOKE EXECUTE ON FUNCTION test_pg_dump(int) FROM regress_dump_test_role;+DROP EXTENSION test_pg_dump;+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, alvherre@alvh.no-ip.org, peter@eisentraut.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: <E0EB10CE-9B58-44C0-8CBC-807BC8E5A84A@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