On Wed, Aug 21, 2019 at 11:07:19AM -0700, Melanie Plageman wrote: > So, I think I completely misunderstood the purpose of 'dry-run'. If no > one is using it, having a check for unused steps in dry-run may not be > useful.
Okay. After sleeping on it and seeing how this thread evolves, it looks that we have more arguments in favor of just let dry-run go to the void. So attached is an updated patch set: - 0001 removes the dry-run mode from isolationtester. - 0002 cleans up the specs of unused steps and adds the discussed sanity checks, as proposed for this thread. -- Michael
From d0e756bba3668af0b1f40b0b1d5bf198b83a97fd Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Thu, 22 Aug 2019 11:46:29 +0900
Subject: [PATCH v3 1/2] Remove dry-run mode from isolationtester
---
src/test/isolation/isolationtester.c | 31 +---------------------------
1 file changed, 1 insertion(+), 30 deletions(-)
diff --git a/src/test/isolation/isolationtester.c b/src/test/isolation/isolationtester.c
index f98bb1cf64..66ebe3b27e 100644
--- a/src/test/isolation/isolationtester.c
+++ b/src/test/isolation/isolationtester.c
@@ -31,9 +31,6 @@ static int *backend_pids = NULL;
static const char **backend_pid_strs = NULL;
static int nconns = 0;
-/* In dry run only output permutations to be run by the tester. */
-static int dry_run = false;
-
static void run_testspec(TestSpec *testspec);
static void run_all_permutations(TestSpec *testspec);
static void run_all_permutations_recurse(TestSpec *testspec, int nsteps,
@@ -76,13 +73,10 @@ main(int argc, char **argv)
int nallsteps;
Step **allsteps;
- while ((opt = getopt(argc, argv, "nV")) != -1)
+ while ((opt = getopt(argc, argv, "V")) != -1)
{
switch (opt)
{
- case 'n':
- dry_run = true;
- break;
case 'V':
puts("isolationtester (PostgreSQL) " PG_VERSION);
exit(0);
@@ -144,16 +138,6 @@ main(int argc, char **argv)
}
}
- /*
- * In dry-run mode, just print the permutations that would be run, and
- * exit.
- */
- if (dry_run)
- {
- run_testspec(testspec);
- return 0;
- }
-
printf("Parsed test spec with %d sessions\n", testspec->nsessions);
/*
@@ -449,19 +433,6 @@ run_permutation(TestSpec *testspec, int nsteps, Step **steps)
Step **waiting;
Step **errorstep;
- /*
- * In dry run mode, just display the permutation in the same format used
- * by spec files, and return.
- */
- if (dry_run)
- {
- printf("permutation");
- for (i = 0; i < nsteps; i++)
- printf(" \"%s\"", steps[i]->name);
- printf("\n");
- return;
- }
-
waiting = pg_malloc(sizeof(Step *) * testspec->nsessions);
errorstep = pg_malloc(sizeof(Step *) * testspec->nsessions);
--
2.23.0
From 1ee92ce4cf359c9f6e7f8ce32e4057ed9bb0412e Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Thu, 22 Aug 2019 11:51:30 +0900
Subject: [PATCH v3 2/2] Improve detection of unused steps in isolation specs
This is useful for developers to find out if an isolation spec is
over-engineering or if it needs more work by warning at the end of a
test run if a step is not used, generating a failure with extra diffs.
While on it, clean up all the specs which include steps not used in any
permutations.
---
src/test/isolation/isolationtester.c | 17 +++++++++++++++++
src/test/isolation/isolationtester.h | 1 +
src/test/isolation/specparse.y | 1 +
src/test/isolation/specs/freeze-the-dead.spec | 3 ---
.../specs/insert-conflict-do-nothing.spec | 1 -
.../specs/insert-conflict-do-update-2.spec | 1 -
.../specs/insert-conflict-do-update.spec | 1 -
src/test/isolation/specs/sequence-ddl.spec | 1 -
.../specs/tuplelock-upgrade-no-deadlock.spec | 3 ---
9 files changed, 19 insertions(+), 10 deletions(-)
diff --git a/src/test/isolation/isolationtester.c b/src/test/isolation/isolationtester.c
index 66ebe3b27e..fe0315be77 100644
--- a/src/test/isolation/isolationtester.c
+++ b/src/test/isolation/isolationtester.c
@@ -235,10 +235,23 @@ static int *piles;
static void
run_testspec(TestSpec *testspec)
{
+ int i;
+
if (testspec->permutations)
run_named_permutations(testspec);
else
run_all_permutations(testspec);
+
+ /*
+ * Verify that all steps have been used, complaining about anything
+ * defined but not used.
+ */
+ for (i = 0; i < testspec->nallsteps; i++)
+ {
+ if (!testspec->allsteps[i]->used)
+ fprintf(stderr, "unused step name: %s\n",
+ testspec->allsteps[i]->name);
+ }
}
/*
@@ -438,7 +451,11 @@ run_permutation(TestSpec *testspec, int nsteps, Step **steps)
printf("\nstarting permutation:");
for (i = 0; i < nsteps; i++)
+ {
+ /* Track the permutation as in-use */
+ steps[i]->used = true;
printf(" %s", steps[i]->name);
+ }
printf("\n");
/* Perform setup */
diff --git a/src/test/isolation/isolationtester.h b/src/test/isolation/isolationtester.h
index 7f91e6433f..d9d2a14ecf 100644
--- a/src/test/isolation/isolationtester.h
+++ b/src/test/isolation/isolationtester.h
@@ -29,6 +29,7 @@ struct Session
struct Step
{
int session;
+ bool used;
char *name;
char *sql;
char *errormsg;
diff --git a/src/test/isolation/specparse.y b/src/test/isolation/specparse.y
index fb8a4d706c..2dfe3533ff 100644
--- a/src/test/isolation/specparse.y
+++ b/src/test/isolation/specparse.y
@@ -145,6 +145,7 @@ step:
$$ = pg_malloc(sizeof(Step));
$$->name = $2;
$$->sql = $3;
+ $$->used = false;
$$->errormsg = NULL;
}
;
diff --git a/src/test/isolation/specs/freeze-the-dead.spec b/src/test/isolation/specs/freeze-the-dead.spec
index 8c3649902a..915bf15b92 100644
--- a/src/test/isolation/specs/freeze-the-dead.spec
+++ b/src/test/isolation/specs/freeze-the-dead.spec
@@ -19,7 +19,6 @@ session "s1"
step "s1_begin" { BEGIN; }
step "s1_update" { UPDATE tab_freeze SET x = x + 1 WHERE id = 3; }
step "s1_commit" { COMMIT; }
-step "s1_vacuum" { VACUUM FREEZE tab_freeze; }
step "s1_selectone" {
BEGIN;
SET LOCAL enable_seqscan = false;
@@ -28,7 +27,6 @@ step "s1_selectone" {
COMMIT;
}
step "s1_selectall" { SELECT * FROM tab_freeze ORDER BY name, id; }
-step "s1_reindex" { REINDEX TABLE tab_freeze; }
session "s2"
step "s2_begin" { BEGIN; }
@@ -40,7 +38,6 @@ session "s3"
step "s3_begin" { BEGIN; }
step "s3_key_share" { SELECT id FROM tab_freeze WHERE id = 3 FOR KEY SHARE; }
step "s3_commit" { COMMIT; }
-step "s3_vacuum" { VACUUM FREEZE tab_freeze; }
# This permutation verifies that a previous bug
# https://postgr.es/m/e5711e62-8fdf-4dca-a888-c200bf6b5...@amazon.com
diff --git a/src/test/isolation/specs/insert-conflict-do-nothing.spec b/src/test/isolation/specs/insert-conflict-do-nothing.spec
index 9b92c35cec..71acc380c7 100644
--- a/src/test/isolation/specs/insert-conflict-do-nothing.spec
+++ b/src/test/isolation/specs/insert-conflict-do-nothing.spec
@@ -33,7 +33,6 @@ setup
step "donothing2" { INSERT INTO ints(key, val) VALUES(1, 'donothing2') ON CONFLICT DO NOTHING; }
step "select2" { SELECT * FROM ints; }
step "c2" { COMMIT; }
-step "a2" { ABORT; }
# Regular case where one session block-waits on another to determine if it
# should proceed with an insert or do nothing.
diff --git a/src/test/isolation/specs/insert-conflict-do-update-2.spec b/src/test/isolation/specs/insert-conflict-do-update-2.spec
index f5b4f601b5..12f6be8000 100644
--- a/src/test/isolation/specs/insert-conflict-do-update-2.spec
+++ b/src/test/isolation/specs/insert-conflict-do-update-2.spec
@@ -32,7 +32,6 @@ setup
step "insert2" { INSERT INTO upsert(key, payload) VALUES('FOOFOO', 'insert2') ON CONFLICT (lower(key)) DO UPDATE set key = EXCLUDED.key, payload = upsert.payload || ' updated by insert2'; }
step "select2" { SELECT * FROM upsert; }
step "c2" { COMMIT; }
-step "a2" { ABORT; }
# One session (session 2) block-waits on another (session 1) to determine if it
# should proceed with an insert or update. The user can still usefully UPDATE
diff --git a/src/test/isolation/specs/insert-conflict-do-update.spec b/src/test/isolation/specs/insert-conflict-do-update.spec
index 5d335a3444..7c8cb47100 100644
--- a/src/test/isolation/specs/insert-conflict-do-update.spec
+++ b/src/test/isolation/specs/insert-conflict-do-update.spec
@@ -30,7 +30,6 @@ setup
step "insert2" { INSERT INTO upsert(key, val) VALUES(1, 'insert2') ON CONFLICT (key) DO UPDATE set val = upsert.val || ' updated by insert2'; }
step "select2" { SELECT * FROM upsert; }
step "c2" { COMMIT; }
-step "a2" { ABORT; }
# One session (session 2) block-waits on another (session 1) to determine if it
# should proceed with an insert or update. Notably, this entails updating a
diff --git a/src/test/isolation/specs/sequence-ddl.spec b/src/test/isolation/specs/sequence-ddl.spec
index a04fd1cc7e..f2a3ff628b 100644
--- a/src/test/isolation/specs/sequence-ddl.spec
+++ b/src/test/isolation/specs/sequence-ddl.spec
@@ -15,7 +15,6 @@ setup { BEGIN; }
step "s1alter" { ALTER SEQUENCE seq1 MAXVALUE 10; }
step "s1alter2" { ALTER SEQUENCE seq1 MAXVALUE 20; }
step "s1restart" { ALTER SEQUENCE seq1 RESTART WITH 5; }
-step "s1setval" { SELECT setval('seq1', 5); }
step "s1commit" { COMMIT; }
session "s2"
diff --git a/src/test/isolation/specs/tuplelock-upgrade-no-deadlock.spec b/src/test/isolation/specs/tuplelock-upgrade-no-deadlock.spec
index 73f71b17a7..106c2465c0 100644
--- a/src/test/isolation/specs/tuplelock-upgrade-no-deadlock.spec
+++ b/src/test/isolation/specs/tuplelock-upgrade-no-deadlock.spec
@@ -19,7 +19,6 @@ teardown
session "s0"
step "s0_begin" { begin; }
step "s0_keyshare" { select id from tlu_job where id = 1 for key share;}
-step "s0_share" { select id from tlu_job where id = 1 for share;}
step "s0_rollback" { rollback; }
session "s1"
@@ -28,7 +27,6 @@ step "s1_keyshare" { select id from tlu_job where id = 1 for key share;}
step "s1_share" { select id from tlu_job where id = 1 for share; }
step "s1_fornokeyupd" { select id from tlu_job where id = 1 for no key update; }
step "s1_update" { update tlu_job set name = 'b' where id = 1; }
-step "s1_delete" { delete from tlu_job where id = 1; }
step "s1_savept_e" { savepoint s1_e; }
step "s1_savept_f" { savepoint s1_f; }
step "s1_rollback_e" { rollback to s1_e; }
@@ -44,7 +42,6 @@ step "s2_for_update" { select id from tlu_job where id = 1 for update; }
step "s2_update" { update tlu_job set name = 'b' where id = 1; }
step "s2_delete" { delete from tlu_job where id = 1; }
step "s2_rollback" { rollback; }
-step "s2_commit" { commit; }
session "s3"
setup { begin; }
--
2.23.0
signature.asc
Description: PGP signature
