pg.ddx.io pgsql-committers@postgresql.org mailing list archive
help / color / mirror / Atom feedpgtcl - libpgtcl: Mark that we're releasing version 1.7.
28+ messages / 9 participants
[nested] [flat]
* pgtcl - libpgtcl: Mark that we're releasing version 1.7.
@ 2009-04-06 15:22 User Karl <karl@pgfoundry.org>
0 siblings, 0 replies; 28+ messages in thread
From: karl@pgfoundry.org @ 2009-04-06 15:22 UTC (permalink / raw)
To: pgsql-committers
Log Message:
-----------
Mark that we're releasing version 1.7.
Modified Files:
--------------
libpgtcl:
ChangeLog (r1.56 -> r1.57)
(http://cvs.pgfoundry.org/cgi-bin/cvsweb.cgi/pgtcl/libpgtcl/ChangeLog.diff?r1=1.56&r2=1.57)
^ permalink raw reply [nested|flat] 28+ messages in thread
* pgsql: Clean up role created in new subscription test.
@ 2023-03-30 17:07 Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 28+ messages in thread
From: Tom Lane @ 2023-03-30 17:07 UTC (permalink / raw)
To: pgsql-committers@lists.postgresql.org
Clean up role created in new subscription test.
This oversight broke repeated runs of "make installcheck".
Branch
------
master
Details
-------
https://git.postgresql.org/pg/commitdiff/e9d202a1499d6a70e80d080fcdba07fe6707845d
Modified Files
--------------
src/test/regress/expected/subscription.out | 1 +
src/test/regress/sql/subscription.sql | 1 +
2 files changed, 2 insertions(+)
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: pgsql: Clean up role created in new subscription test.
@ 2023-03-30 18:19 Robert Haas <robertmhaas@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 28+ messages in thread
From: Robert Haas @ 2023-03-30 18:19 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
On Thu, Mar 30, 2023 at 1:07 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Clean up role created in new subscription test.
>
> This oversight broke repeated runs of "make installcheck".
GAAAAH. You would think that I would have learned better by now, but
evidently not. Is there some way we can add an automated guard against
this?
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: pgsql: Clean up role created in new subscription test.
@ 2023-03-30 18:44 Tom Lane <tgl@sss.pgh.pa.us>
parent: Robert Haas <robertmhaas@gmail.com>
0 siblings, 1 reply; 28+ messages in thread
From: Tom Lane @ 2023-03-30 18:44 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
Robert Haas <robertmhaas@gmail.com> writes:
> On Thu, Mar 30, 2023 at 1:07 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> This oversight broke repeated runs of "make installcheck".
> GAAAAH. You would think that I would have learned better by now, but
> evidently not. Is there some way we can add an automated guard against
> this?
Hm. We could add a final test step that prints out all still-existing
roles, but the trick is to have it not fail in a legitimate installcheck
context (ie, when there are indeed some pre-existing roles).
Maybe it'd be close enough to expect there to be no roles named
"regress_xxx". In combination with
-DENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS, that would prevent us
from accidentally leaving stuff behind, and we could hope that it doesn't
cause false failures in real installations.
Another idea could be for pg_regress to enforce that "select count(*)
from pg_roles" gives the same answer before and after the test run.
That would then be enforced against all pg_regress suites not just
the main one, but perhaps that's good.
Likewise for tablespaces, subscriptions, and other globally-visible
objects, of course.
regards, tom lane
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: pgsql: Clean up role created in new subscription test.
@ 2023-03-30 19:19 Daniel Gustafsson <daniel@yesql.se>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 28+ messages in thread
From: Daniel Gustafsson @ 2023-03-30 19:19 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Robert Haas <robertmhaas@gmail.com>; PostgreSQL Hackers <pgsql-hackers@postgresql.org>
> On 30 Mar 2023, at 20:44, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Maybe it'd be close enough to expect there to be no roles named
> "regress_xxx". In combination with
> -DENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS, that would prevent us
> from accidentally leaving stuff behind, and we could hope that it doesn't
> cause false failures in real installations.
Would that check be always on or only when pg_regress is compiled with
-DENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS?
> Another idea could be for pg_regress to enforce that "select count(*)
> from pg_roles" gives the same answer before and after the test run.
That wouldn't prevent the contents of pg_roles to have changed though, so there
is a (slim) false positive risk with that no?
--
Daniel Gustafsson
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: pgsql: Clean up role created in new subscription test.
@ 2023-03-30 20:29 Tom Lane <tgl@sss.pgh.pa.us>
parent: Daniel Gustafsson <daniel@yesql.se>
0 siblings, 2 replies; 28+ messages in thread
From: Tom Lane @ 2023-03-30 20:29 UTC (permalink / raw)
To: Daniel Gustafsson <daniel@yesql.se>; +Cc: Robert Haas <robertmhaas@gmail.com>; PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Daniel Gustafsson <daniel@yesql.se> writes:
>> On 30 Mar 2023, at 20:44, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Maybe it'd be close enough to expect there to be no roles named
>> "regress_xxx". In combination with
>> -DENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS, that would prevent us
>> from accidentally leaving stuff behind, and we could hope that it doesn't
>> cause false failures in real installations.
> Would that check be always on or only when pg_regress is compiled with
> -DENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS?
I envisioned it as being on all the time.
>> Another idea could be for pg_regress to enforce that "select count(*)
>> from pg_roles" gives the same answer before and after the test run.
> That wouldn't prevent the contents of pg_roles to have changed though, so there
> is a (slim) false positive risk with that no?
Well, we could do "select rolname from pg_roles order by 1" and
actually compare the results of the two selects. That might be
advisable anyway, in order to produce a complaint with useful
detail when there is something wrong.
regards, tom lane
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: pgsql: Clean up role created in new subscription test.
@ 2023-03-30 20:53 Daniel Gustafsson <daniel@yesql.se>
parent: Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 0 replies; 28+ messages in thread
From: Daniel Gustafsson @ 2023-03-30 20:53 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Robert Haas <robertmhaas@gmail.com>; PostgreSQL Hackers <pgsql-hackers@postgresql.org>
> On 30 Mar 2023, at 22:29, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Daniel Gustafsson <daniel@yesql.se> writes:
>>> On 30 Mar 2023, at 20:44, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> Another idea could be for pg_regress to enforce that "select count(*)
>>> from pg_roles" gives the same answer before and after the test run.
>
>> That wouldn't prevent the contents of pg_roles to have changed though, so there
>> is a (slim) false positive risk with that no?
>
> Well, we could do "select rolname from pg_roles order by 1" and
> actually compare the results of the two selects. That might be
> advisable anyway, in order to produce a complaint with useful
> detail when there is something wrong.
I can see the value in doing something like this to keep us honest.
--
Daniel Gustafsson
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: pgsql: Clean up role created in new subscription test.
@ 2023-05-15 08:59 Daniel Gustafsson <daniel@yesql.se>
parent: Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 1 reply; 28+ messages in thread
From: Daniel Gustafsson @ 2023-05-15 08:59 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Robert Haas <robertmhaas@gmail.com>; PostgreSQL Hackers <pgsql-hackers@postgresql.org>
> On 30 Mar 2023, at 22:29, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Well, we could do "select rolname from pg_roles order by 1" and
> actually compare the results of the two selects. That might be
> advisable anyway, in order to produce a complaint with useful
> detail when there is something wrong.
I took a look at this and came up with the attached. This adds a new parameter
to pg_regress for specifying a test which will be executed before and after the
suite, where the first invocation creates the expectfile for the second. For
storing the expecfile the temp dir creation is somewhat refactored. I've added
a sample test in the patch (to regress, not ECPG), but I'm sure it can be
expanded to be a bit more interesting. The comment which is now incorrectly
formatted was left like that to make review easier, if this gets committed it
will be fixed then.
I opted for this to use the machinery that pg_regress already has rather than
add a new mechanism (and dependency) for running and verifying queries. This
also avoids hardcoding the test making it easier to have custom queries during
hacking etc.
Looking at this I also found a bug introduced in the TAP format patch, which
made failed single run tests report as 0ms due to the parameters being mixed up
in the report function call. This is in 0002, which I'll apply to HEAD
regardless of 0001 as they are unrelated.
--
Daniel Gustafsson
Attachments:
[application/octet-stream] v1-0002-pg_regress-Fix-reported-runtime-for-failed-single.patch (1.2K, ../../0134C9EC-5F6B-4EAC-B2D5-BB4249BEBD4D@yesql.se/2-v1-0002-pg_regress-Fix-reported-runtime-for-failed-single.patch)
download | inline diff:
From 964f9baf8f5152529e0164cd7bbfafbab475b580 Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <dgustafsson@postgresql.org>
Date: Mon, 15 May 2023 10:45:18 +0200
Subject: [PATCH v1 2/2] pg_regress: Fix reported runtime for failed single
tests
The order of paramters to test_status_failed() was incorrect making
failed single tests report a runtime of 0ms. Parallel tests were not
affected.
---
src/test/regress/pg_regress.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/test/regress/pg_regress.c b/src/test/regress/pg_regress.c
index 7a950b662d..ec6799a358 100644
--- a/src/test/regress/pg_regress.c
+++ b/src/test/regress/pg_regress.c
@@ -1969,14 +1969,14 @@ run_single_test(const char *test, test_start_function startfunc,
if (exit_status != 0)
{
- test_status_failed(test, false, INSTR_TIME_GET_MILLISEC(stoptime));
+ test_status_failed(test, INSTR_TIME_GET_MILLISEC(stoptime), false);
log_child_failure(exit_status);
}
else
{
if (differ)
{
- test_status_failed(test, false, INSTR_TIME_GET_MILLISEC(stoptime));
+ test_status_failed(test, INSTR_TIME_GET_MILLISEC(stoptime), false);
}
else
{
--
2.32.1 (Apple Git-133)
=
[application/octet-stream] v1-0001-pg_regress-Add-database-verification-test.patch (11.2K, ../../0134C9EC-5F6B-4EAC-B2D5-BB4249BEBD4D@yesql.se/3-v1-0001-pg_regress-Add-database-verification-test.patch)
download | inline diff:
From af8f6f75e851256a8233e37ee9ac4e5018273191 Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <dgustafsson@postgresql.org>
Date: Mon, 15 May 2023 10:31:14 +0200
Subject: [PATCH v1 1/2] pg_regress: Add database verification test
This adds a new parameter to pg_regress for specifying a test which
is used to verify that the test suite didn't have side effects. The
test is executed before the testsuite, and the resultsfile is saved
as the expectfile for when the test is re-run after the suite. The
results is then diffed against the previously saved expectfile.
Discussion: https://postgr.es/m/267900.1680201870@sss.pgh.pa.us
---
src/test/regress/GNUmakefile | 6 +-
src/test/regress/pg_regress.c | 149 ++++++++++++++++++++++++----
src/test/regress/sql/sideeffect.sql | 5 +
3 files changed, 136 insertions(+), 24 deletions(-)
create mode 100644 src/test/regress/sql/sideeffect.sql
diff --git a/src/test/regress/GNUmakefile b/src/test/regress/GNUmakefile
index 38c3a1f85b..d6119dc9e5 100644
--- a/src/test/regress/GNUmakefile
+++ b/src/test/regress/GNUmakefile
@@ -115,16 +115,16 @@ 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) --verifydb=sideeffect --schedule=$(srcdir)/parallel_schedule $(MAXCONNOPT) $(EXTRA_TESTS)
check-tests: all | temp-install
$(pg_regress_check) $(REGRESS_OPTS) $(MAXCONNOPT) $(TESTS) $(EXTRA_TESTS)
installcheck: all
- $(pg_regress_installcheck) $(REGRESS_OPTS) --schedule=$(srcdir)/parallel_schedule --max-connections=1 $(EXTRA_TESTS)
+ $(pg_regress_installcheck) $(REGRESS_OPTS) --verifydb=sideeffect --schedule=$(srcdir)/parallel_schedule --max-connections=1 $(EXTRA_TESTS)
installcheck-parallel: all
- $(pg_regress_installcheck) $(REGRESS_OPTS) --schedule=$(srcdir)/parallel_schedule $(MAXCONNOPT) $(EXTRA_TESTS)
+ $(pg_regress_installcheck) $(REGRESS_OPTS) --verifydb=sideeffect --schedule=$(srcdir)/parallel_schedule $(MAXCONNOPT) $(EXTRA_TESTS)
installcheck-tests: all
$(pg_regress_installcheck) $(REGRESS_OPTS) $(TESTS) $(EXTRA_TESTS)
diff --git a/src/test/regress/pg_regress.c b/src/test/regress/pg_regress.c
index 48008fa8c3..7a950b662d 100644
--- a/src/test/regress/pg_regress.c
+++ b/src/test/regress/pg_regress.c
@@ -93,6 +93,7 @@ bool debug = false;
char *inputdir = ".";
char *outputdir = ".";
char *expecteddir = ".";
+char *verifydb_test = NULL;
char *bindir = PGBINDIR;
char *launcher = NULL;
static _stringlist *loadextension = NULL;
@@ -112,6 +113,7 @@ static char *dlpath = PKGLIBDIR;
static char *user = NULL;
static _stringlist *extraroles = NULL;
static char *config_auth_datadir = NULL;
+static char *verifydb_expect = NULL;
/* internal variables */
static const char *progname;
@@ -119,11 +121,10 @@ static char *logfilename;
static FILE *logfile;
static char *difffilename;
static const char *sockdir;
-static const char *temp_sockdir;
-static char sockself[MAXPGPATH];
-static char socklock[MAXPGPATH];
+static const char *temp_dir = NULL;
static StringInfo failed_tests = NULL;
static bool in_note = false;
+static _stringlist *removefiles = NULL;
static _resultmap *resultmap = NULL;
@@ -135,6 +136,7 @@ static int fail_count = 0;
static bool directory_exists(const char *dir);
static void make_directory(const char *dir);
+static void run_verification(const char *test, test_start_function startfunc, bool validate);
static void test_status_print(bool ok, const char *testname, double runtime, bool parallel);
static void test_status_ok(const char *testname, double runtime, bool parallel);
@@ -435,7 +437,8 @@ stop_postmaster(void)
}
/*
- * Remove the socket temporary directory. pg_regress never waits for a
+ * Remove the temporary directory and remove all files which have been added
+ * to it. pg_regress never waits for a
* postmaster exit, so it is indeterminate whether the postmaster has yet to
* unlink the socket and lock file. Unlink them here so we can proceed to
* remove the directory. Ignore errors; leaking a temporary directory is
@@ -446,10 +449,15 @@ stop_postmaster(void)
static void
remove_temp(void)
{
- Assert(temp_sockdir);
- unlink(sockself);
- unlink(socklock);
- rmdir(temp_sockdir);
+ _stringlist *rf;
+
+ Assert(temp_dir);
+
+ /* Remove any files which has been added to the temporary directory */
+ for (rf = removefiles; rf != NULL; rf = rf->next)
+ unlink(rf->str);
+
+ rmdir(temp_dir);
}
/*
@@ -465,7 +473,9 @@ signal_remove_temp(SIGNAL_ARGS)
}
/*
- * Create a temporary directory suitable for the server's Unix-domain socket.
+ * Create a temporary directory suitable for the server's Unix-domain socket
+ * as well as any intermediate expectfiles.
+ *
* The directory will have mode 0700 or stricter, so no other OS user can open
* our socket to exploit our use of trust authentication. Most systems
* constrain the length of socket paths well below _POSIX_PATH_MAX, so we
@@ -477,22 +487,23 @@ signal_remove_temp(SIGNAL_ARGS)
* the build/test user.
*/
static const char *
-make_temp_sockdir(void)
+make_temp_dir(void)
{
- char *template = psprintf("%s/pg_regress-XXXXXX",
- getenv("TMPDIR") ? getenv("TMPDIR") : "/tmp");
+ char *template;
- temp_sockdir = mkdtemp(template);
- if (temp_sockdir == NULL)
+ if (temp_dir != NULL)
+ return temp_dir;
+
+ template = psprintf("%s/pg_regress-XXXXXX",
+ getenv("TMPDIR") ? getenv("TMPDIR") : "/tmp");
+
+ temp_dir = mkdtemp(template);
+ if (temp_dir == NULL)
{
- bail("could not create directory \"%s\": %s",
+ bail("could not create temporary directory \"%s\": %s",
template, strerror(errno));
}
- /* Stage file names for remove_temp(). Unsafe in a signal handler. */
- UNIXSOCK_PATH(sockself, port, temp_sockdir);
- snprintf(socklock, sizeof(socklock), "%s.lock", sockself);
-
/* Remove the directory during clean exit. */
atexit(remove_temp);
@@ -505,7 +516,7 @@ make_temp_sockdir(void)
pqsignal(SIGPIPE, signal_remove_temp);
pqsignal(SIGTERM, signal_remove_temp);
- return temp_sockdir;
+ return temp_dir;
}
/*
@@ -828,7 +839,20 @@ initialize_environment(void)
{
sockdir = getenv("PG_REGRESS_SOCK_DIR");
if (!sockdir)
- sockdir = make_temp_sockdir();
+ {
+ char sockself[MAXPGPATH];
+ char socklock[MAXPGPATH];
+
+ sockdir = make_temp_dir();
+ /*
+ * Stage socket file names for remove_temp(). Unsafe in a
+ * signal handler.
+ */
+ UNIXSOCK_PATH(sockself, port, temp_dir);
+ snprintf(socklock, sizeof(socklock), "%s.lock", sockself);
+ add_stringlist_item(&removefiles, sockself);
+ add_stringlist_item(&removefiles, socklock);
+ }
setenv("PGHOST", sockdir, 1);
}
unsetenv("PGHOSTADDR");
@@ -1840,6 +1864,60 @@ run_schedule(const char *schedule, test_start_function startfunc,
fclose(scf);
}
+/*
+ * Run the database verification test. Database validation tests are intended
+ * to be used for validating that the testsuite doesn't modify the database.
+ * When validate is set to false, the test results are not validated but are
+ * saved to a temporary folder to be used as a future expectfile. When validate
+ * is set to true the test results are validated against the previously stored
+ * expectfile.
+ */
+static void
+run_verification(const char *test, test_start_function startfunc, bool validate)
+{
+ PID_TYPE pid;
+ instr_time starttime;
+ instr_time stoptime;
+ int exit_status;
+ _stringlist *resultfiles = NULL;
+ _stringlist *expectfiles = NULL;
+ _stringlist *tags = NULL;
+
+ pid = (startfunc) (test, &resultfiles, &expectfiles, &tags);
+ INSTR_TIME_SET_CURRENT(starttime);
+ wait_for_tests(&pid, &exit_status, &stoptime, NULL, 1);
+
+ if (exit_status != 0)
+ {
+ test_status_failed(test, INSTR_TIME_GET_MILLISEC(stoptime), false);
+ log_child_failure(exit_status);
+ }
+
+ /*
+ * Save the resultfile as the expectfile for the next invocation after the
+ * the testsuite has finished.
+ */
+ if (!validate)
+ {
+ errno = 0;
+ if (rename(resultfiles[0].str, verifydb_expect) != 0)
+ bail("could not write temporary expectfile \"%s\": %s",
+ verifydb_expect, strerror(errno));
+ return;
+ }
+
+ INSTR_TIME_SUBTRACT(stoptime, starttime);
+
+ /*
+ * When executed after the testsuite we validate the results against the
+ * resultfile from the initial run, whcih has been saved as an expectfile.
+ */
+ if (results_differ(test, resultfiles[0].str, verifydb_expect))
+ test_status_failed(test, INSTR_TIME_GET_MILLISEC(stoptime), false);
+ else
+ test_status_ok(test, INSTR_TIME_GET_MILLISEC(stoptime), false);
+}
+
/*
* Run a single test
*/
@@ -2049,6 +2127,7 @@ help(void)
printf(_(" (can be used multiple times to concatenate)\n"));
printf(_(" --temp-instance=DIR create a temporary instance in DIR\n"));
printf(_(" --use-existing use an existing installation\n"));
+ printf(_(" --verifydb=TEST run TEST before and after and ensure output matches\n"));
printf(_(" -V, --version output version information, then exit\n"));
printf(_("\n"));
printf(_("Options for \"temp-instance\" mode:\n"));
@@ -2099,6 +2178,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},
+ {"verifydb", required_argument, NULL, 27},
{NULL, 0, NULL, 0}
};
@@ -2228,6 +2308,9 @@ regression_main(int argc, char *argv[],
case 26:
expecteddir = pg_strdup(optarg);
break;
+ case 27:
+ verifydb_test = pg_strdup(optarg);
+ break;
default:
/* getopt_long already emitted a complaint */
pg_log_error_hint("Try \"%s --help\" for more information.",
@@ -2535,6 +2618,21 @@ regression_main(int argc, char *argv[],
create_role(sl->str, dblist);
}
+ /*
+ * If there is database verification test, make sure to run it after we
+ * set up extra databases and roles, but before any test has been run. A
+ * temporary expectdir also needs to be set up for this test since the
+ * outputfile from this test will be the expected file for when we run it
+ * again after the test(s).
+ */
+ if (verifydb_test)
+ {
+ make_temp_dir();
+ verifydb_expect = psprintf("%s/%s.sql", temp_dir, verifydb_test);
+ add_stringlist_item(&removefiles, verifydb_expect);
+ run_verification(verifydb_test, startfunc, false);
+ }
+
/*
* Ready to run the tests
*/
@@ -2548,6 +2646,15 @@ regression_main(int argc, char *argv[],
run_single_test(sl->str, startfunc, postfunc);
}
+ /*
+ * Run the verification test again and compare the results with the saved
+ * results from before.
+ */
+ if (verifydb_test)
+ {
+ run_verification(verifydb_test, startfunc, true);
+ }
+
/*
* Shut down temp installation's postmaster
*/
diff --git a/src/test/regress/sql/sideeffect.sql b/src/test/regress/sql/sideeffect.sql
new file mode 100644
index 0000000000..4726a2bc8c
--- /dev/null
+++ b/src/test/regress/sql/sideeffect.sql
@@ -0,0 +1,5 @@
+-- Queries to ensure no sideeffects from the regress suite. This test does not
+-- have a corresponding expectfile stored.
+SELECT rolname FROM pg_roles ORDER BY 1;
+SELECT spcname FROM pg_tablespace ORDER BY 1;
+SELECT subname FROM pg_subscription ORDER BY 1;
--
2.32.1 (Apple Git-133)
=
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: pgsql: Clean up role created in new subscription test.
@ 2023-05-16 09:17 Daniel Gustafsson <daniel@yesql.se>
parent: Daniel Gustafsson <daniel@yesql.se>
0 siblings, 1 reply; 28+ messages in thread
From: Daniel Gustafsson @ 2023-05-16 09:17 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Robert Haas <robertmhaas@gmail.com>; PostgreSQL Hackers <pgsql-hackers@postgresql.org>
> On 15 May 2023, at 10:59, Daniel Gustafsson <daniel@yesql.se> wrote:
> Looking at this I also found a bug introduced in the TAP format patch, which
> made failed single run tests report as 0ms due to the parameters being mixed up
> in the report function call. This is in 0002, which I'll apply to HEAD
> regardless of 0001 as they are unrelated.
With 0002 applied, attached is just the 0001 rebased to keep the CFBot from
being angry when applying an already applied patch. Parked in the July CF for
now.
--
Daniel Gustafsson
Attachments:
[application/octet-stream] v2-0001-pg_regress-Add-database-verification-test.patch (11.2K, ../../3C83CEE2-0758-400C-979C-FD9E828C4709@yesql.se/2-v2-0001-pg_regress-Add-database-verification-test.patch)
download | inline diff:
From d19a6ff6304003a946c587571d2cbef8b6a8045c Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <dgustafsson@postgresql.org>
Date: Mon, 15 May 2023 10:31:14 +0200
Subject: [PATCH v2] pg_regress: Add database verification test
This adds a new parameter to pg_regress for specifying a test which
is used to verify that the test suite didn't have side effects. The
test is executed before the testsuite, and the resultsfile is saved
as the expectfile for when the test is re-run after the suite. The
results is then diffed against the previously saved expectfile.
Discussion: https://postgr.es/m/267900.1680201870@sss.pgh.pa.us
---
src/test/regress/GNUmakefile | 6 +-
src/test/regress/pg_regress.c | 149 ++++++++++++++++++++++++----
src/test/regress/sql/sideeffect.sql | 5 +
3 files changed, 136 insertions(+), 24 deletions(-)
create mode 100644 src/test/regress/sql/sideeffect.sql
diff --git a/src/test/regress/GNUmakefile b/src/test/regress/GNUmakefile
index 38c3a1f85b..d6119dc9e5 100644
--- a/src/test/regress/GNUmakefile
+++ b/src/test/regress/GNUmakefile
@@ -115,16 +115,16 @@ 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) --verifydb=sideeffect --schedule=$(srcdir)/parallel_schedule $(MAXCONNOPT) $(EXTRA_TESTS)
check-tests: all | temp-install
$(pg_regress_check) $(REGRESS_OPTS) $(MAXCONNOPT) $(TESTS) $(EXTRA_TESTS)
installcheck: all
- $(pg_regress_installcheck) $(REGRESS_OPTS) --schedule=$(srcdir)/parallel_schedule --max-connections=1 $(EXTRA_TESTS)
+ $(pg_regress_installcheck) $(REGRESS_OPTS) --verifydb=sideeffect --schedule=$(srcdir)/parallel_schedule --max-connections=1 $(EXTRA_TESTS)
installcheck-parallel: all
- $(pg_regress_installcheck) $(REGRESS_OPTS) --schedule=$(srcdir)/parallel_schedule $(MAXCONNOPT) $(EXTRA_TESTS)
+ $(pg_regress_installcheck) $(REGRESS_OPTS) --verifydb=sideeffect --schedule=$(srcdir)/parallel_schedule $(MAXCONNOPT) $(EXTRA_TESTS)
installcheck-tests: all
$(pg_regress_installcheck) $(REGRESS_OPTS) $(TESTS) $(EXTRA_TESTS)
diff --git a/src/test/regress/pg_regress.c b/src/test/regress/pg_regress.c
index 88ab255ce6..ec6799a358 100644
--- a/src/test/regress/pg_regress.c
+++ b/src/test/regress/pg_regress.c
@@ -93,6 +93,7 @@ bool debug = false;
char *inputdir = ".";
char *outputdir = ".";
char *expecteddir = ".";
+char *verifydb_test = NULL;
char *bindir = PGBINDIR;
char *launcher = NULL;
static _stringlist *loadextension = NULL;
@@ -112,6 +113,7 @@ static char *dlpath = PKGLIBDIR;
static char *user = NULL;
static _stringlist *extraroles = NULL;
static char *config_auth_datadir = NULL;
+static char *verifydb_expect = NULL;
/* internal variables */
static const char *progname;
@@ -119,11 +121,10 @@ static char *logfilename;
static FILE *logfile;
static char *difffilename;
static const char *sockdir;
-static const char *temp_sockdir;
-static char sockself[MAXPGPATH];
-static char socklock[MAXPGPATH];
+static const char *temp_dir = NULL;
static StringInfo failed_tests = NULL;
static bool in_note = false;
+static _stringlist *removefiles = NULL;
static _resultmap *resultmap = NULL;
@@ -135,6 +136,7 @@ static int fail_count = 0;
static bool directory_exists(const char *dir);
static void make_directory(const char *dir);
+static void run_verification(const char *test, test_start_function startfunc, bool validate);
static void test_status_print(bool ok, const char *testname, double runtime, bool parallel);
static void test_status_ok(const char *testname, double runtime, bool parallel);
@@ -435,7 +437,8 @@ stop_postmaster(void)
}
/*
- * Remove the socket temporary directory. pg_regress never waits for a
+ * Remove the temporary directory and remove all files which have been added
+ * to it. pg_regress never waits for a
* postmaster exit, so it is indeterminate whether the postmaster has yet to
* unlink the socket and lock file. Unlink them here so we can proceed to
* remove the directory. Ignore errors; leaking a temporary directory is
@@ -446,10 +449,15 @@ stop_postmaster(void)
static void
remove_temp(void)
{
- Assert(temp_sockdir);
- unlink(sockself);
- unlink(socklock);
- rmdir(temp_sockdir);
+ _stringlist *rf;
+
+ Assert(temp_dir);
+
+ /* Remove any files which has been added to the temporary directory */
+ for (rf = removefiles; rf != NULL; rf = rf->next)
+ unlink(rf->str);
+
+ rmdir(temp_dir);
}
/*
@@ -465,7 +473,9 @@ signal_remove_temp(SIGNAL_ARGS)
}
/*
- * Create a temporary directory suitable for the server's Unix-domain socket.
+ * Create a temporary directory suitable for the server's Unix-domain socket
+ * as well as any intermediate expectfiles.
+ *
* The directory will have mode 0700 or stricter, so no other OS user can open
* our socket to exploit our use of trust authentication. Most systems
* constrain the length of socket paths well below _POSIX_PATH_MAX, so we
@@ -477,22 +487,23 @@ signal_remove_temp(SIGNAL_ARGS)
* the build/test user.
*/
static const char *
-make_temp_sockdir(void)
+make_temp_dir(void)
{
- char *template = psprintf("%s/pg_regress-XXXXXX",
- getenv("TMPDIR") ? getenv("TMPDIR") : "/tmp");
+ char *template;
- temp_sockdir = mkdtemp(template);
- if (temp_sockdir == NULL)
+ if (temp_dir != NULL)
+ return temp_dir;
+
+ template = psprintf("%s/pg_regress-XXXXXX",
+ getenv("TMPDIR") ? getenv("TMPDIR") : "/tmp");
+
+ temp_dir = mkdtemp(template);
+ if (temp_dir == NULL)
{
- bail("could not create directory \"%s\": %s",
+ bail("could not create temporary directory \"%s\": %s",
template, strerror(errno));
}
- /* Stage file names for remove_temp(). Unsafe in a signal handler. */
- UNIXSOCK_PATH(sockself, port, temp_sockdir);
- snprintf(socklock, sizeof(socklock), "%s.lock", sockself);
-
/* Remove the directory during clean exit. */
atexit(remove_temp);
@@ -505,7 +516,7 @@ make_temp_sockdir(void)
pqsignal(SIGPIPE, signal_remove_temp);
pqsignal(SIGTERM, signal_remove_temp);
- return temp_sockdir;
+ return temp_dir;
}
/*
@@ -828,7 +839,20 @@ initialize_environment(void)
{
sockdir = getenv("PG_REGRESS_SOCK_DIR");
if (!sockdir)
- sockdir = make_temp_sockdir();
+ {
+ char sockself[MAXPGPATH];
+ char socklock[MAXPGPATH];
+
+ sockdir = make_temp_dir();
+ /*
+ * Stage socket file names for remove_temp(). Unsafe in a
+ * signal handler.
+ */
+ UNIXSOCK_PATH(sockself, port, temp_dir);
+ snprintf(socklock, sizeof(socklock), "%s.lock", sockself);
+ add_stringlist_item(&removefiles, sockself);
+ add_stringlist_item(&removefiles, socklock);
+ }
setenv("PGHOST", sockdir, 1);
}
unsetenv("PGHOSTADDR");
@@ -1840,6 +1864,60 @@ run_schedule(const char *schedule, test_start_function startfunc,
fclose(scf);
}
+/*
+ * Run the database verification test. Database validation tests are intended
+ * to be used for validating that the testsuite doesn't modify the database.
+ * When validate is set to false, the test results are not validated but are
+ * saved to a temporary folder to be used as a future expectfile. When validate
+ * is set to true the test results are validated against the previously stored
+ * expectfile.
+ */
+static void
+run_verification(const char *test, test_start_function startfunc, bool validate)
+{
+ PID_TYPE pid;
+ instr_time starttime;
+ instr_time stoptime;
+ int exit_status;
+ _stringlist *resultfiles = NULL;
+ _stringlist *expectfiles = NULL;
+ _stringlist *tags = NULL;
+
+ pid = (startfunc) (test, &resultfiles, &expectfiles, &tags);
+ INSTR_TIME_SET_CURRENT(starttime);
+ wait_for_tests(&pid, &exit_status, &stoptime, NULL, 1);
+
+ if (exit_status != 0)
+ {
+ test_status_failed(test, INSTR_TIME_GET_MILLISEC(stoptime), false);
+ log_child_failure(exit_status);
+ }
+
+ /*
+ * Save the resultfile as the expectfile for the next invocation after the
+ * the testsuite has finished.
+ */
+ if (!validate)
+ {
+ errno = 0;
+ if (rename(resultfiles[0].str, verifydb_expect) != 0)
+ bail("could not write temporary expectfile \"%s\": %s",
+ verifydb_expect, strerror(errno));
+ return;
+ }
+
+ INSTR_TIME_SUBTRACT(stoptime, starttime);
+
+ /*
+ * When executed after the testsuite we validate the results against the
+ * resultfile from the initial run, whcih has been saved as an expectfile.
+ */
+ if (results_differ(test, resultfiles[0].str, verifydb_expect))
+ test_status_failed(test, INSTR_TIME_GET_MILLISEC(stoptime), false);
+ else
+ test_status_ok(test, INSTR_TIME_GET_MILLISEC(stoptime), false);
+}
+
/*
* Run a single test
*/
@@ -2049,6 +2127,7 @@ help(void)
printf(_(" (can be used multiple times to concatenate)\n"));
printf(_(" --temp-instance=DIR create a temporary instance in DIR\n"));
printf(_(" --use-existing use an existing installation\n"));
+ printf(_(" --verifydb=TEST run TEST before and after and ensure output matches\n"));
printf(_(" -V, --version output version information, then exit\n"));
printf(_("\n"));
printf(_("Options for \"temp-instance\" mode:\n"));
@@ -2099,6 +2178,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},
+ {"verifydb", required_argument, NULL, 27},
{NULL, 0, NULL, 0}
};
@@ -2228,6 +2308,9 @@ regression_main(int argc, char *argv[],
case 26:
expecteddir = pg_strdup(optarg);
break;
+ case 27:
+ verifydb_test = pg_strdup(optarg);
+ break;
default:
/* getopt_long already emitted a complaint */
pg_log_error_hint("Try \"%s --help\" for more information.",
@@ -2535,6 +2618,21 @@ regression_main(int argc, char *argv[],
create_role(sl->str, dblist);
}
+ /*
+ * If there is database verification test, make sure to run it after we
+ * set up extra databases and roles, but before any test has been run. A
+ * temporary expectdir also needs to be set up for this test since the
+ * outputfile from this test will be the expected file for when we run it
+ * again after the test(s).
+ */
+ if (verifydb_test)
+ {
+ make_temp_dir();
+ verifydb_expect = psprintf("%s/%s.sql", temp_dir, verifydb_test);
+ add_stringlist_item(&removefiles, verifydb_expect);
+ run_verification(verifydb_test, startfunc, false);
+ }
+
/*
* Ready to run the tests
*/
@@ -2548,6 +2646,15 @@ regression_main(int argc, char *argv[],
run_single_test(sl->str, startfunc, postfunc);
}
+ /*
+ * Run the verification test again and compare the results with the saved
+ * results from before.
+ */
+ if (verifydb_test)
+ {
+ run_verification(verifydb_test, startfunc, true);
+ }
+
/*
* Shut down temp installation's postmaster
*/
diff --git a/src/test/regress/sql/sideeffect.sql b/src/test/regress/sql/sideeffect.sql
new file mode 100644
index 0000000000..4726a2bc8c
--- /dev/null
+++ b/src/test/regress/sql/sideeffect.sql
@@ -0,0 +1,5 @@
+-- Queries to ensure no sideeffects from the regress suite. This test does not
+-- have a corresponding expectfile stored.
+SELECT rolname FROM pg_roles ORDER BY 1;
+SELECT spcname FROM pg_tablespace ORDER BY 1;
+SELECT subname FROM pg_subscription ORDER BY 1;
--
2.32.1 (Apple Git-133)
=
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: pgsql: Clean up role created in new subscription test.
@ 2023-07-05 22:00 Daniel Gustafsson <daniel@yesql.se>
parent: Daniel Gustafsson <daniel@yesql.se>
0 siblings, 1 reply; 28+ messages in thread
From: Daniel Gustafsson @ 2023-07-05 22:00 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Robert Haas <robertmhaas@gmail.com>; PostgreSQL Hackers <pgsql-hackers@postgresql.org>
> On 16 May 2023, at 11:17, Daniel Gustafsson <daniel@yesql.se> wrote:
> Parked in the July CF for now.
Rebased to fix a trivial conflict highlighted by the CFBot.
--
Daniel Gustafsson
Attachments:
[application/octet-stream] v3-0001-pg_regress-Add-database-verification-test.patch (11.2K, ../../4447BBC5-26DA-4BDB-838B-50819E8E6EA7@yesql.se/2-v3-0001-pg_regress-Add-database-verification-test.patch)
download | inline diff:
From be2684b6efd2e6388bfab0c3520f1f73b3ee7288 Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <dgustafsson@postgresql.org>
Date: Wed, 5 Jul 2023 23:50:51 +0200
Subject: [PATCH v3] pg_regress: Add database verification test
This adds a new parameter to pg_regress for specifying a test which
is used to verify that the test suite didn't have side effects. The
test is executed before the testsuite, and the resultsfile is saved
as the expectfile for when the test is re-run after the suite. The
results is then diffed against the previously saved expectfile.
Discussion: https://postgr.es/m/267900.1680201870@sss.pgh.pa.us
---
src/test/regress/GNUmakefile | 6 +-
src/test/regress/pg_regress.c | 149 ++++++++++++++++++++++++----
src/test/regress/sql/sideeffect.sql | 5 +
3 files changed, 136 insertions(+), 24 deletions(-)
create mode 100644 src/test/regress/sql/sideeffect.sql
diff --git a/src/test/regress/GNUmakefile b/src/test/regress/GNUmakefile
index 38c3a1f85b..d6119dc9e5 100644
--- a/src/test/regress/GNUmakefile
+++ b/src/test/regress/GNUmakefile
@@ -115,16 +115,16 @@ 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) --verifydb=sideeffect --schedule=$(srcdir)/parallel_schedule $(MAXCONNOPT) $(EXTRA_TESTS)
check-tests: all | temp-install
$(pg_regress_check) $(REGRESS_OPTS) $(MAXCONNOPT) $(TESTS) $(EXTRA_TESTS)
installcheck: all
- $(pg_regress_installcheck) $(REGRESS_OPTS) --schedule=$(srcdir)/parallel_schedule --max-connections=1 $(EXTRA_TESTS)
+ $(pg_regress_installcheck) $(REGRESS_OPTS) --verifydb=sideeffect --schedule=$(srcdir)/parallel_schedule --max-connections=1 $(EXTRA_TESTS)
installcheck-parallel: all
- $(pg_regress_installcheck) $(REGRESS_OPTS) --schedule=$(srcdir)/parallel_schedule $(MAXCONNOPT) $(EXTRA_TESTS)
+ $(pg_regress_installcheck) $(REGRESS_OPTS) --verifydb=sideeffect --schedule=$(srcdir)/parallel_schedule $(MAXCONNOPT) $(EXTRA_TESTS)
installcheck-tests: all
$(pg_regress_installcheck) $(REGRESS_OPTS) $(TESTS) $(EXTRA_TESTS)
diff --git a/src/test/regress/pg_regress.c b/src/test/regress/pg_regress.c
index 3e4fd918dd..1c5988bbd7 100644
--- a/src/test/regress/pg_regress.c
+++ b/src/test/regress/pg_regress.c
@@ -93,6 +93,7 @@ bool debug = false;
char *inputdir = ".";
char *outputdir = ".";
char *expecteddir = ".";
+char *verifydb_test = NULL;
char *bindir = PGBINDIR;
char *launcher = NULL;
static _stringlist *loadextension = NULL;
@@ -112,6 +113,7 @@ static char *dlpath = PKGLIBDIR;
static char *user = NULL;
static _stringlist *extraroles = NULL;
static char *config_auth_datadir = NULL;
+static char *verifydb_expect = NULL;
/* internal variables */
static const char *progname;
@@ -119,11 +121,10 @@ static char *logfilename;
static FILE *logfile;
static char *difffilename;
static const char *sockdir;
-static const char *temp_sockdir;
-static char sockself[MAXPGPATH];
-static char socklock[MAXPGPATH];
+static const char *temp_dir = NULL;
static StringInfo failed_tests = NULL;
static bool in_note = false;
+static _stringlist *removefiles = NULL;
static _resultmap *resultmap = NULL;
@@ -135,6 +136,7 @@ static int fail_count = 0;
static bool directory_exists(const char *dir);
static void make_directory(const char *dir);
+static void run_verification(const char *test, test_start_function startfunc, bool validate);
static void test_status_print(bool ok, const char *testname, double runtime, bool parallel);
static void test_status_ok(const char *testname, double runtime, bool parallel);
@@ -435,7 +437,8 @@ stop_postmaster(void)
}
/*
- * Remove the socket temporary directory. pg_regress never waits for a
+ * Remove the temporary directory and remove all files which have been added
+ * to it. pg_regress never waits for a
* postmaster exit, so it is indeterminate whether the postmaster has yet to
* unlink the socket and lock file. Unlink them here so we can proceed to
* remove the directory. Ignore errors; leaking a temporary directory is
@@ -446,10 +449,15 @@ stop_postmaster(void)
static void
remove_temp(void)
{
- Assert(temp_sockdir);
- unlink(sockself);
- unlink(socklock);
- rmdir(temp_sockdir);
+ _stringlist *rf;
+
+ Assert(temp_dir);
+
+ /* Remove any files which has been added to the temporary directory */
+ for (rf = removefiles; rf != NULL; rf = rf->next)
+ unlink(rf->str);
+
+ rmdir(temp_dir);
}
/*
@@ -465,7 +473,9 @@ signal_remove_temp(SIGNAL_ARGS)
}
/*
- * Create a temporary directory suitable for the server's Unix-domain socket.
+ * Create a temporary directory suitable for the server's Unix-domain socket
+ * as well as any intermediate expectfiles.
+ *
* The directory will have mode 0700 or stricter, so no other OS user can open
* our socket to exploit our use of trust authentication. Most systems
* constrain the length of socket paths well below _POSIX_PATH_MAX, so we
@@ -477,22 +487,23 @@ signal_remove_temp(SIGNAL_ARGS)
* the build/test user.
*/
static const char *
-make_temp_sockdir(void)
+make_temp_dir(void)
{
- char *template = psprintf("%s/pg_regress-XXXXXX",
- getenv("TMPDIR") ? getenv("TMPDIR") : "/tmp");
+ char *template;
- temp_sockdir = mkdtemp(template);
- if (temp_sockdir == NULL)
+ if (temp_dir != NULL)
+ return temp_dir;
+
+ template = psprintf("%s/pg_regress-XXXXXX",
+ getenv("TMPDIR") ? getenv("TMPDIR") : "/tmp");
+
+ temp_dir = mkdtemp(template);
+ if (temp_dir == NULL)
{
- bail("could not create directory \"%s\": %s",
+ bail("could not create temporary directory \"%s\": %s",
template, strerror(errno));
}
- /* Stage file names for remove_temp(). Unsafe in a signal handler. */
- UNIXSOCK_PATH(sockself, port, temp_sockdir);
- snprintf(socklock, sizeof(socklock), "%s.lock", sockself);
-
/* Remove the directory during clean exit. */
atexit(remove_temp);
@@ -505,7 +516,7 @@ make_temp_sockdir(void)
pqsignal(SIGPIPE, signal_remove_temp);
pqsignal(SIGTERM, signal_remove_temp);
- return temp_sockdir;
+ return temp_dir;
}
/*
@@ -829,7 +840,20 @@ initialize_environment(void)
{
sockdir = getenv("PG_REGRESS_SOCK_DIR");
if (!sockdir)
- sockdir = make_temp_sockdir();
+ {
+ char sockself[MAXPGPATH];
+ char socklock[MAXPGPATH];
+
+ sockdir = make_temp_dir();
+ /*
+ * Stage socket file names for remove_temp(). Unsafe in a
+ * signal handler.
+ */
+ UNIXSOCK_PATH(sockself, port, temp_dir);
+ snprintf(socklock, sizeof(socklock), "%s.lock", sockself);
+ add_stringlist_item(&removefiles, sockself);
+ add_stringlist_item(&removefiles, socklock);
+ }
setenv("PGHOST", sockdir, 1);
}
unsetenv("PGHOSTADDR");
@@ -1841,6 +1865,60 @@ run_schedule(const char *schedule, test_start_function startfunc,
fclose(scf);
}
+/*
+ * Run the database verification test. Database validation tests are intended
+ * to be used for validating that the testsuite doesn't modify the database.
+ * When validate is set to false, the test results are not validated but are
+ * saved to a temporary folder to be used as a future expectfile. When validate
+ * is set to true the test results are validated against the previously stored
+ * expectfile.
+ */
+static void
+run_verification(const char *test, test_start_function startfunc, bool validate)
+{
+ PID_TYPE pid;
+ instr_time starttime;
+ instr_time stoptime;
+ int exit_status;
+ _stringlist *resultfiles = NULL;
+ _stringlist *expectfiles = NULL;
+ _stringlist *tags = NULL;
+
+ pid = (startfunc) (test, &resultfiles, &expectfiles, &tags);
+ INSTR_TIME_SET_CURRENT(starttime);
+ wait_for_tests(&pid, &exit_status, &stoptime, NULL, 1);
+
+ if (exit_status != 0)
+ {
+ test_status_failed(test, INSTR_TIME_GET_MILLISEC(stoptime), false);
+ log_child_failure(exit_status);
+ }
+
+ /*
+ * Save the resultfile as the expectfile for the next invocation after the
+ * the testsuite has finished.
+ */
+ if (!validate)
+ {
+ errno = 0;
+ if (rename(resultfiles[0].str, verifydb_expect) != 0)
+ bail("could not write temporary expectfile \"%s\": %s",
+ verifydb_expect, strerror(errno));
+ return;
+ }
+
+ INSTR_TIME_SUBTRACT(stoptime, starttime);
+
+ /*
+ * When executed after the testsuite we validate the results against the
+ * resultfile from the initial run, whcih has been saved as an expectfile.
+ */
+ if (results_differ(test, resultfiles[0].str, verifydb_expect))
+ test_status_failed(test, INSTR_TIME_GET_MILLISEC(stoptime), false);
+ else
+ test_status_ok(test, INSTR_TIME_GET_MILLISEC(stoptime), false);
+}
+
/*
* Run a single test
*/
@@ -2050,6 +2128,7 @@ help(void)
printf(_(" (can be used multiple times to concatenate)\n"));
printf(_(" --temp-instance=DIR create a temporary instance in DIR\n"));
printf(_(" --use-existing use an existing installation\n"));
+ printf(_(" --verifydb=TEST run TEST before and after and ensure output matches\n"));
printf(_(" -V, --version output version information, then exit\n"));
printf(_("\n"));
printf(_("Options for \"temp-instance\" mode:\n"));
@@ -2100,6 +2179,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},
+ {"verifydb", required_argument, NULL, 27},
{NULL, 0, NULL, 0}
};
@@ -2229,6 +2309,9 @@ regression_main(int argc, char *argv[],
case 26:
expecteddir = pg_strdup(optarg);
break;
+ case 27:
+ verifydb_test = pg_strdup(optarg);
+ break;
default:
/* getopt_long already emitted a complaint */
pg_log_error_hint("Try \"%s --help\" for more information.",
@@ -2542,6 +2625,21 @@ regression_main(int argc, char *argv[],
create_role(sl->str, dblist);
}
+ /*
+ * If there is database verification test, make sure to run it after we
+ * set up extra databases and roles, but before any test has been run. A
+ * temporary expectdir also needs to be set up for this test since the
+ * outputfile from this test will be the expected file for when we run it
+ * again after the test(s).
+ */
+ if (verifydb_test)
+ {
+ make_temp_dir();
+ verifydb_expect = psprintf("%s/%s.sql", temp_dir, verifydb_test);
+ add_stringlist_item(&removefiles, verifydb_expect);
+ run_verification(verifydb_test, startfunc, false);
+ }
+
/*
* Ready to run the tests
*/
@@ -2555,6 +2653,15 @@ regression_main(int argc, char *argv[],
run_single_test(sl->str, startfunc, postfunc);
}
+ /*
+ * Run the verification test again and compare the results with the saved
+ * results from before.
+ */
+ if (verifydb_test)
+ {
+ run_verification(verifydb_test, startfunc, true);
+ }
+
/*
* Shut down temp installation's postmaster
*/
diff --git a/src/test/regress/sql/sideeffect.sql b/src/test/regress/sql/sideeffect.sql
new file mode 100644
index 0000000000..4726a2bc8c
--- /dev/null
+++ b/src/test/regress/sql/sideeffect.sql
@@ -0,0 +1,5 @@
+-- Queries to ensure no sideeffects from the regress suite. This test does not
+-- have a corresponding expectfile stored.
+SELECT rolname FROM pg_roles ORDER BY 1;
+SELECT spcname FROM pg_tablespace ORDER BY 1;
+SELECT subname FROM pg_subscription ORDER BY 1;
--
2.32.1 (Apple Git-133)
=
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: pgsql: Clean up role created in new subscription test.
@ 2023-11-08 07:55 Peter Eisentraut <peter@eisentraut.org>
parent: Daniel Gustafsson <daniel@yesql.se>
0 siblings, 2 replies; 28+ messages in thread
From: Peter Eisentraut @ 2023-11-08 07:55 UTC (permalink / raw)
To: Daniel Gustafsson <daniel@yesql.se>; Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Robert Haas <robertmhaas@gmail.com>; PostgreSQL Hackers <pgsql-hackers@postgresql.org>
On 06.07.23 00:00, Daniel Gustafsson wrote:
>> On 16 May 2023, at 11:17, Daniel Gustafsson <daniel@yesql.se> wrote:
>
>> Parked in the July CF for now.
>
> Rebased to fix a trivial conflict highlighted by the CFBot.
I think the problem with this approach is that one would need to reapply
it to each regression test suite separately. For example, there are
several tests under contrib/ that create roles. These would not be
covered by this automatically.
I think the earlier idea of just counting roles, tablespaces, etc.
before and after would be sufficient.
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: pgsql: Clean up role created in new subscription test.
@ 2023-11-08 11:42 Alvaro Herrera <alvherre@alvh.no-ip.org>
parent: Peter Eisentraut <peter@eisentraut.org>
1 sibling, 1 reply; 28+ messages in thread
From: Alvaro Herrera @ 2023-11-08 11:42 UTC (permalink / raw)
To: Peter Eisentraut <peter@eisentraut.org>; +Cc: Daniel Gustafsson <daniel@yesql.se>; Tom Lane <tgl@sss.pgh.pa.us>; Robert Haas <robertmhaas@gmail.com>; PostgreSQL Hackers <pgsql-hackers@postgresql.org>
On 2023-Nov-08, Peter Eisentraut wrote:
> I think the earlier idea of just counting roles, tablespaces, etc. before
> and after would be sufficient.
Maybe record global objects in a permanent table in test_setup.sql
create table global_objs as
select 'role', rolname from pg_roles
union all
select 'tablespace', spcname from pg_tablespace;
and at the end (maybe in test tablespace, though it's unrelated but it's
what runs last and drops regress_tablespace), have
(select 'role', rolname from pg_roles
union all
select 'tablespace', spcname from pg_tablespace)
except
select * from global_objs;
and check the expected as empty.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"La verdad no siempre es bonita, pero el hambre de ella sí"
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: pgsql: Clean up role created in new subscription test.
@ 2023-11-08 12:15 Daniel Gustafsson <daniel@yesql.se>
parent: Peter Eisentraut <peter@eisentraut.org>
1 sibling, 0 replies; 28+ messages in thread
From: Daniel Gustafsson @ 2023-11-08 12:15 UTC (permalink / raw)
To: Peter Eisentraut <peter@eisentraut.org>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Robert Haas <robertmhaas@gmail.com>; PostgreSQL Hackers <pgsql-hackers@postgresql.org>
> On 8 Nov 2023, at 08:55, Peter Eisentraut <peter@eisentraut.org> wrote:
>
> On 06.07.23 00:00, Daniel Gustafsson wrote:
>>> On 16 May 2023, at 11:17, Daniel Gustafsson <daniel@yesql.se> wrote:
>>> Parked in the July CF for now.
>> Rebased to fix a trivial conflict highlighted by the CFBot.
>
> I think the problem with this approach is that one would need to reapply it to each regression test suite separately. For example, there are several tests under contrib/ that create roles. These would not be covered by this automatically.
>
> I think the earlier idea of just counting roles, tablespaces, etc. before and after would be sufficient.
It's been a while but if memory serves me right, one of the reasons for this
approach was that pg_regress didn't use libpq so running queries and storing
results for comparisons other than diffing .out files was painful at best.
Since 66d6086cbc pg_regress does have a dependency on libpq so we can now
perform that bookkeeping a bit easier. I still find it more elegant to at
least compare the contents and not just the count, but I can take a stab at a
revised patch since this approach doesn't seem to appeal to the thread.
--
Daniel Gustafsson
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: pgsql: Clean up role created in new subscription test.
@ 2023-11-08 12:24 Daniel Gustafsson <daniel@yesql.se>
parent: Alvaro Herrera <alvherre@alvh.no-ip.org>
0 siblings, 1 reply; 28+ messages in thread
From: Daniel Gustafsson @ 2023-11-08 12:24 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@alvh.no-ip.org>; +Cc: Peter Eisentraut <peter@eisentraut.org>; Tom Lane <tgl@sss.pgh.pa.us>; Robert Haas <robertmhaas@gmail.com>; PostgreSQL Hackers <pgsql-hackers@postgresql.org>
> On 8 Nov 2023, at 12:42, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> On 2023-Nov-08, Peter Eisentraut wrote:
>> I think the earlier idea of just counting roles, tablespaces, etc. before
>> and after would be sufficient.
>
> Maybe record global objects in a permanent table in test_setup.sql
Since test_setup.sql is part of the regress schedule and not pg_regress we
would have to implement this for each test run (regress, contribs etc), which
is what Peter didn't like about the original suggestion.
--
Daniel Gustafsson
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: pgsql: Clean up role created in new subscription test.
@ 2023-11-08 12:32 Alvaro Herrera <alvherre@alvh.no-ip.org>
parent: Daniel Gustafsson <daniel@yesql.se>
0 siblings, 2 replies; 28+ messages in thread
From: Alvaro Herrera @ 2023-11-08 12:32 UTC (permalink / raw)
To: Daniel Gustafsson <daniel@yesql.se>; +Cc: Peter Eisentraut <peter@eisentraut.org>; Tom Lane <tgl@sss.pgh.pa.us>; Robert Haas <robertmhaas@gmail.com>; PostgreSQL Hackers <pgsql-hackers@postgresql.org>
On 2023-Nov-08, Daniel Gustafsson wrote:
> Since test_setup.sql is part of the regress schedule and not pg_regress we
> would have to implement this for each test run (regress, contribs etc), which
> is what Peter didn't like about the original suggestion.
Oh, somehow that aspect of his reply failed to register with me. I
agree with your approach of using libpq in pg_regress then.
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.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: pgsql: Clean up role created in new subscription test.
@ 2023-11-08 13:06 Daniel Gustafsson <daniel@yesql.se>
parent: Alvaro Herrera <alvherre@alvh.no-ip.org>
1 sibling, 0 replies; 28+ messages in thread
From: Daniel Gustafsson @ 2023-11-08 13:06 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@alvh.no-ip.org>; +Cc: Peter Eisentraut <peter@eisentraut.org>; Tom Lane <tgl@sss.pgh.pa.us>; Robert Haas <robertmhaas@gmail.com>; PostgreSQL Hackers <pgsql-hackers@postgresql.org>
> On 8 Nov 2023, at 13:32, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2023-Nov-08, Daniel Gustafsson wrote:
>
>> Since test_setup.sql is part of the regress schedule and not pg_regress we
>> would have to implement this for each test run (regress, contribs etc), which
>> is what Peter didn't like about the original suggestion.
>
> Oh, somehow that aspect of his reply failed to register with me. I
> agree with your approach of using libpq in pg_regress then.
>
> 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.
Correct, that's my plan. The rationale for the earlier patch was to avoid
adding a dependency on libpq, but with that already discussed and done we can
leverage the fact that we can run such queries easy.
--
Daniel Gustafsson
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: pgsql: Clean up role created in new subscription test.
@ 2023-12-01 11:22 Daniel Gustafsson <daniel@yesql.se>
parent: Alvaro Herrera <alvherre@alvh.no-ip.org>
1 sibling, 2 replies; 28+ messages in thread
From: Daniel Gustafsson @ 2023-12-01 11:22 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@alvh.no-ip.org>; +Cc: Peter Eisentraut <peter@eisentraut.org>; Tom Lane <tgl@sss.pgh.pa.us>; Robert Haas <robertmhaas@gmail.com>; PostgreSQL Hackers <pgsql-hackers@postgresql.org>
> 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.c
index 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.out
index 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.sql
index 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)
=
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: pgsql: Clean up role created in new subscription test.
@ 2023-12-01 11:37 Heikki Linnakangas <hlinnaka@iki.fi>
parent: Daniel Gustafsson <daniel@yesql.se>
1 sibling, 1 reply; 28+ messages in thread
From: Heikki Linnakangas @ 2023-12-01 11:37 UTC (permalink / raw)
To: Daniel Gustafsson <daniel@yesql.se>; Alvaro Herrera <alvherre@alvh.no-ip.org>; +Cc: Peter Eisentraut <peter@eisentraut.org>; Tom Lane <tgl@sss.pgh.pa.us>; Robert Haas <robertmhaas@gmail.com>; PostgreSQL Hackers <pgsql-hackers@postgresql.org>
On 01/12/2023 13:22, Daniel Gustafsson wrote:
>> 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.
Do we want to impose this policy to all extensions too?
> + /*
> + * 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);
> +
Strictly speaking, the order of this query isn't guaranteed to be
stable, although in practice it probably is. Maybe something like this:
(SELECT 'role', rolname AS obj FROM pg_catalog.pg_roles
UNION ALL
SELECT 'tablespace', spcname AS obj FROM pg_catalog.pg_tablespace
UNION ALL
SELECT 'subscription', subname AS obj FROM pg_catalog.pg_subscription
) ORDER BY 1, 2
Is it OK to leave behind extra databases?
--
Heikki Linnakangas
Neon (https://neon.tech)
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: pgsql: Clean up role created in new subscription test.
@ 2023-12-01 11:46 Daniel Gustafsson <daniel@yesql.se>
parent: Heikki Linnakangas <hlinnaka@iki.fi>
0 siblings, 0 replies; 28+ messages in thread
From: Daniel Gustafsson @ 2023-12-01 11:46 UTC (permalink / raw)
To: Heikki Linnakangas <hlinnaka@iki.fi>; +Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>; Peter Eisentraut <peter@eisentraut.org>; Tom Lane <tgl@sss.pgh.pa.us>; Robert Haas <robertmhaas@gmail.com>; PostgreSQL Hackers <pgsql-hackers@postgresql.org>
> On 1 Dec 2023, at 12:37, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>
> On 01/12/2023 13:22, Daniel Gustafsson wrote:
>>> 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.
>
> Do we want to impose this policy to all extensions too?
I don't think it would be bad, and as of today the policy holds for all of
check-world apart from this one test module.
>> + /*
>> + * 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);
>> +
>
> Strictly speaking, the order of this query isn't guaranteed to be stable, although in practice it probably is.
Of course, will fix. I originally had three separate query_to_stringlist calls
and had a brainfade when combining. It seemed like pointless use of cycles
when we can get everything in one connection.
> Is it OK to leave behind extra databases?
The test suite for pg_upgrade can make use of left behind databases to seed the
old cluster, so I think that's allowed by design.
--
Daniel Gustafsson
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: pgsql: Clean up role created in new subscription test.
@ 2023-12-01 12:19 Alvaro Herrera <alvherre@alvh.no-ip.org>
parent: Daniel Gustafsson <daniel@yesql.se>
1 sibling, 1 reply; 28+ messages in thread
From: Alvaro Herrera @ 2023-12-01 12:19 UTC (permalink / raw)
To: Daniel Gustafsson <daniel@yesql.se>; +Cc: Peter Eisentraut <peter@eisentraut.org>; Tom Lane <tgl@sss.pgh.pa.us>; Robert Haas <robertmhaas@gmail.com>; PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Isn't it simpler to use DROP OWNED BY in 0001?
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"Hay que recordar que la existencia en el cosmos, y particularmente la
elaboración de civilizaciones dentro de él no son, por desgracia,
nada idílicas" (Ijon Tichy)
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: pgsql: Clean up role created in new subscription test.
@ 2023-12-01 12:52 Daniel Gustafsson <daniel@yesql.se>
parent: Alvaro Herrera <alvherre@alvh.no-ip.org>
0 siblings, 1 reply; 28+ messages in thread
From: Daniel Gustafsson @ 2023-12-01 12:52 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@alvh.no-ip.org>; +Cc: Peter Eisentraut <peter@eisentraut.org>; Tom Lane <tgl@sss.pgh.pa.us>; Robert Haas <robertmhaas@gmail.com>; PostgreSQL Hackers <pgsql-hackers@postgresql.org>
> On 1 Dec 2023, at 13:19, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> Isn't it simpler to use DROP OWNED BY in 0001?
I suppose it is, I kind of like the explicit drops but we do use OWNED BY quite
a lot in the regression tests so changed to that in the attached v5.
--
Daniel Gustafsson
Attachments:
[application/octet-stream] v5-0001-Drop-global-objects-after-completed-test.patch (1.7K, ../../57373BD5-C0D9-486D-9096-276FE91411D7@yesql.se/2-v5-0001-Drop-global-objects-after-completed-test.patch)
download | inline diff:
From d379713114a2c3388bbb7ca207e0d615700ffe97 Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <dgustafsson@postgresql.org>
Date: Fri, 1 Dec 2023 12:04:49 +0100
Subject: [PATCH v5 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.out
index 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.sql
index 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)
=
[application/octet-stream] v5-0002-pg_regress-Detect-global-objects-left-over-after-.patch (4.1K, ../../57373BD5-C0D9-486D-9096-276FE91411D7@yesql.se/3-v5-0002-pg_regress-Detect-global-objects-left-over-after-.patch)
download | inline diff:
From 543f2b0fe8830f192f21a53513cedf2baa7bc283 Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <dgustafsson@postgresql.org>
Date: Fri, 1 Dec 2023 12:10:17 +0100
Subject: [PATCH v5 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.c
index 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)
=
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: pgsql: Clean up role created in new subscription test.
@ 2024-01-18 00:57 vignesh C <vignesh21@gmail.com>
parent: Daniel Gustafsson <daniel@yesql.se>
0 siblings, 1 reply; 28+ messages in thread
From: vignesh C @ 2024-01-18 00:57 UTC (permalink / raw)
To: Daniel Gustafsson <daniel@yesql.se>; +Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>; Peter Eisentraut <peter@eisentraut.org>; Tom Lane <tgl@sss.pgh.pa.us>; Robert Haas <robertmhaas@gmail.com>; PostgreSQL Hackers <pgsql-hackers@postgresql.org>
On Fri, 1 Dec 2023 at 18:23, Daniel Gustafsson <daniel@yesql.se> wrote:
>
> > On 1 Dec 2023, at 13:19, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> >
> > Isn't it simpler to use DROP OWNED BY in 0001?
>
> I suppose it is, I kind of like the explicit drops but we do use OWNED BY quite
> a lot in the regression tests so changed to that in the attached v5.
There are a lot of failures in CFBot at [1] with:
# test failed
----------------------------------- stderr -----------------------------------
# left over global object detected: regress_test_bypassrls
# 1 of 2 tests failed.
More details of the same are available at [2].
Do we need to clean up the objects leftover for the reported issues in the test?
[1] - https://cirrus-ci.com/task/6222185975513088
[2] - https://api.cirrus-ci.com/v1/artifact/task/6222185975513088/meson_log/build/meson-logs/testlog-runni...
Regards,
Vignesh
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: pgsql: Clean up role created in new subscription test.
@ 2024-01-19 14:26 Daniel Gustafsson <daniel@yesql.se>
parent: vignesh C <vignesh21@gmail.com>
0 siblings, 1 reply; 28+ messages in thread
From: Daniel Gustafsson @ 2024-01-19 14:26 UTC (permalink / raw)
To: vignesh C <vignesh21@gmail.com>; +Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>; Peter Eisentraut <peter@eisentraut.org>; Tom Lane <tgl@sss.pgh.pa.us>; Robert Haas <robertmhaas@gmail.com>; PostgreSQL Hackers <pgsql-hackers@postgresql.org>
> On 18 Jan 2024, at 01:57, vignesh C <vignesh21@gmail.com> wrote:
> There are a lot of failures in CFBot at [1] with:
> More details of the same are available at [2].
> Do we need to clean up the objects leftover for the reported issues in the test?
Not really, these should not need cleaning up, and it's quite odd that it only
happens on FreeBSD. I need to investigate further so I'll mark this waiting on
author in the meantime.
--
Daniel Gustafsson
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: pgsql: Clean up role created in new subscription test.
@ 2024-01-19 14:40 Peter Eisentraut <peter@eisentraut.org>
parent: Daniel Gustafsson <daniel@yesql.se>
0 siblings, 1 reply; 28+ messages in thread
From: Peter Eisentraut @ 2024-01-19 14:40 UTC (permalink / raw)
To: Daniel Gustafsson <daniel@yesql.se>; vignesh C <vignesh21@gmail.com>; +Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>; Tom Lane <tgl@sss.pgh.pa.us>; Robert Haas <robertmhaas@gmail.com>; PostgreSQL Hackers <pgsql-hackers@postgresql.org>
On 19.01.24 15:26, Daniel Gustafsson wrote:
>> On 18 Jan 2024, at 01:57, vignesh C <vignesh21@gmail.com> wrote:
>
>> There are a lot of failures in CFBot at [1] with:
>
>> More details of the same are available at [2].
>> Do we need to clean up the objects leftover for the reported issues in the test?
>
> Not really, these should not need cleaning up, and it's quite odd that it only
> happens on FreeBSD. I need to investigate further so I'll mark this waiting on
> author in the meantime
Most likely because only the FreeBSD job uses
ENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS.
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: pgsql: Clean up role created in new subscription test.
@ 2024-03-25 18:48 Andres Freund <andres@anarazel.de>
parent: Peter Eisentraut <peter@eisentraut.org>
0 siblings, 1 reply; 28+ messages in thread
From: Andres Freund @ 2024-03-25 18:48 UTC (permalink / raw)
To: Peter Eisentraut <peter@eisentraut.org>; +Cc: Daniel Gustafsson <daniel@yesql.se>; vignesh C <vignesh21@gmail.com>; Alvaro Herrera <alvherre@alvh.no-ip.org>; Tom Lane <tgl@sss.pgh.pa.us>; Robert Haas <robertmhaas@gmail.com>; PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Hi,
On 2024-01-19 15:40:21 +0100, Peter Eisentraut wrote:
> On 19.01.24 15:26, Daniel Gustafsson wrote:
> > > On 18 Jan 2024, at 01:57, vignesh C <vignesh21@gmail.com> wrote:
> >
> > > There are a lot of failures in CFBot at [1] with:
> >
> > > More details of the same are available at [2].
> > > Do we need to clean up the objects leftover for the reported issues in the test?
> >
> > Not really, these should not need cleaning up, and it's quite odd that it only
> > happens on FreeBSD. I need to investigate further so I'll mark this waiting on
> > author in the meantime
>
> Most likely because only the FreeBSD job uses
> ENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS.
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?
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: pgsql: Clean up role created in new subscription test.
@ 2024-03-27 13:42 Daniel Gustafsson <daniel@yesql.se>
parent: Andres Freund <andres@anarazel.de>
0 siblings, 1 reply; 28+ messages in thread
From: Daniel Gustafsson @ 2024-03-27 13:42 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: Peter Eisentraut <peter@eisentraut.org>; vignesh C <vignesh21@gmail.com>; Alvaro Herrera <alvherre@alvh.no-ip.org>; Tom Lane <tgl@sss.pgh.pa.us>; Robert Haas <robertmhaas@gmail.com>; PostgreSQL Hackers <pgsql-hackers@postgresql.org>
> 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/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)
=
[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.out
index 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.sql
index 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)
=
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: pgsql: Clean up role created in new subscription test.
@ 2024-03-27 15:26 Tom Lane <tgl@sss.pgh.pa.us>
parent: Daniel Gustafsson <daniel@yesql.se>
0 siblings, 1 reply; 28+ messages in thread
From: Tom Lane @ 2024-03-27 15:26 UTC (permalink / raw)
To: Daniel Gustafsson <daniel@yesql.se>; +Cc: Andres Freund <andres@anarazel.de>; Peter Eisentraut <peter@eisentraut.org>; vignesh C <vignesh21@gmail.com>; Alvaro Herrera <alvherre@alvh.no-ip.org>; Robert Haas <robertmhaas@gmail.com>; PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Daniel Gustafsson <daniel@yesql.se> writes:
> 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?
Yeah, that seems hardly worth it --- and it's only an accident of
implementation that we don't run the main tests in parallel with
something else, anyway. Too bad, but ...
regards, tom lane
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: pgsql: Clean up role created in new subscription test.
@ 2024-03-27 16:06 Daniel Gustafsson <daniel@yesql.se>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 28+ messages in thread
From: Daniel Gustafsson @ 2024-03-27 16:06 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Andres Freund <andres@anarazel.de>; Peter Eisentraut <peter@eisentraut.org>; vignesh C <vignesh21@gmail.com>; Alvaro Herrera <alvherre@alvh.no-ip.org>; Robert Haas <robertmhaas@gmail.com>; PostgreSQL Hackers <pgsql-hackers@postgresql.org>
> On 27 Mar 2024, at 16:26, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Daniel Gustafsson <daniel@yesql.se> writes:
>> 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?
>
> Yeah, that seems hardly worth it --- and it's only an accident of
> implementation that we don't run the main tests in parallel with
> something else, anyway. Too bad, but ...
Agreed. The excercise did catch the leftovers in 0001 so I'll go ahead with
those before closing the CF entry.
--
Daniel Gustafsson
^ permalink raw reply [nested|flat] 28+ messages in thread
end of thread, other threads:[~2024-03-27 16:06 UTC | newest]
Thread overview: 28+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2009-04-06 15:22 pgtcl - libpgtcl: Mark that we're releasing version 1.7. User Karl <karl@pgfoundry.org>
2023-03-30 17:07 pgsql: Clean up role created in new subscription test. Tom Lane <tgl@sss.pgh.pa.us>
2023-03-30 18:19 ` Re: pgsql: Clean up role created in new subscription test. Robert Haas <robertmhaas@gmail.com>
2023-03-30 18:44 ` Re: pgsql: Clean up role created in new subscription test. Tom Lane <tgl@sss.pgh.pa.us>
2023-03-30 19:19 ` Re: pgsql: Clean up role created in new subscription test. Daniel Gustafsson <daniel@yesql.se>
2023-03-30 20:29 ` Re: pgsql: Clean up role created in new subscription test. Tom Lane <tgl@sss.pgh.pa.us>
2023-03-30 20:53 ` Re: pgsql: Clean up role created in new subscription test. Daniel Gustafsson <daniel@yesql.se>
2023-05-15 08:59 ` Re: pgsql: Clean up role created in new subscription test. Daniel Gustafsson <daniel@yesql.se>
2023-05-16 09:17 ` Re: pgsql: Clean up role created in new subscription test. Daniel Gustafsson <daniel@yesql.se>
2023-07-05 22:00 ` Re: pgsql: Clean up role created in new subscription test. Daniel Gustafsson <daniel@yesql.se>
2023-11-08 07:55 ` Re: pgsql: Clean up role created in new subscription test. Peter Eisentraut <peter@eisentraut.org>
2023-11-08 11:42 ` Re: pgsql: Clean up role created in new subscription test. Alvaro Herrera <alvherre@alvh.no-ip.org>
2023-11-08 12:24 ` Re: pgsql: Clean up role created in new subscription test. Daniel Gustafsson <daniel@yesql.se>
2023-11-08 12:32 ` Re: pgsql: Clean up role created in new subscription test. Alvaro Herrera <alvherre@alvh.no-ip.org>
2023-11-08 13:06 ` Re: pgsql: Clean up role created in new subscription test. Daniel Gustafsson <daniel@yesql.se>
2023-12-01 11:22 ` Re: pgsql: Clean up role created in new subscription test. Daniel Gustafsson <daniel@yesql.se>
2023-12-01 11:37 ` Re: pgsql: Clean up role created in new subscription test. Heikki Linnakangas <hlinnaka@iki.fi>
2023-12-01 11:46 ` Re: pgsql: Clean up role created in new subscription test. Daniel Gustafsson <daniel@yesql.se>
2023-12-01 12:19 ` Re: pgsql: Clean up role created in new subscription test. Alvaro Herrera <alvherre@alvh.no-ip.org>
2023-12-01 12:52 ` Re: pgsql: Clean up role created in new subscription test. Daniel Gustafsson <daniel@yesql.se>
2024-01-18 00:57 ` Re: pgsql: Clean up role created in new subscription test. vignesh C <vignesh21@gmail.com>
2024-01-19 14:26 ` Re: pgsql: Clean up role created in new subscription test. Daniel Gustafsson <daniel@yesql.se>
2024-01-19 14:40 ` Re: pgsql: Clean up role created in new subscription test. Peter Eisentraut <peter@eisentraut.org>
2024-03-25 18:48 ` Re: pgsql: Clean up role created in new subscription test. Andres Freund <andres@anarazel.de>
2024-03-27 13:42 ` Re: pgsql: Clean up role created in new subscription test. Daniel Gustafsson <daniel@yesql.se>
2024-03-27 15:26 ` Re: pgsql: Clean up role created in new subscription test. Tom Lane <tgl@sss.pgh.pa.us>
2024-03-27 16:06 ` Re: pgsql: Clean up role created in new subscription test. Daniel Gustafsson <daniel@yesql.se>
2023-11-08 12:15 ` Re: pgsql: Clean up role created in new subscription test. Daniel Gustafsson <daniel@yesql.se>
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