From: Daniel Gustafsson <daniel@yesql.se>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Robert Haas <robertmhaas@gmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Subject: Re: pgsql: Clean up role created in new subscription test.
Date: Mon, 15 May 2023 10:59:20 +0200
Message-ID: <0134C9EC-5F6B-4EAC-B2D5-BB4249BEBD4D@yesql.se> (raw)
In-Reply-To: <284965.1680208176@sss.pgh.pa.us>
References: <E1phvk7-000VAH-7k@gemulon.postgresql.org>
<CA+TgmoZ+K2OU0Ojs8_4SgqPWJJq7e0XiLd-uJGZ11R_WKF1URQ@mail.gmail.com>
<267900.1680201870@sss.pgh.pa.us>
<99112997-6732-42C7-989B-06406C911FE2@yesql.se>
<284965.1680208176@sss.pgh.pa.us>
> 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.cindex 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/GNUmakefileindex 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.cindex 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.sqlnew file mode 100644index 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)
=
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-committers@postgresql.org
Cc: daniel@yesql.se, tgl@sss.pgh.pa.us, robertmhaas@gmail.com, pgsql-hackers@postgresql.org
Subject: Re: pgsql: Clean up role created in new subscription test.
In-Reply-To: <0134C9EC-5F6B-4EAC-B2D5-BB4249BEBD4D@yesql.se>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox