From 989d23b04beac0c26f44c379b04ac781eaa4265e Mon Sep 17 00:00:00 2001 From: Michael Paquier Date: Sat, 24 Aug 2019 11:45:05 +0900 Subject: [PATCH] Detect unused steps in isolation specs and do some cleanup This is useful for developers to find out if an isolation spec is over-engineered 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 to simplify them. Author: Michael Paquier Reviewed-by: Asim Praveen, Melanie Plageman Discussion: https://postgr.es/m/20190819080820.GG18166@paquier.xyz --- src/test/isolation/isolationtester.c | 19 ++++++++++++++++++- 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, 20 insertions(+), 11 deletions(-) diff --git a/src/test/isolation/isolationtester.c b/src/test/isolation/isolationtester.c index 66ebe3b27e..556b46d93f 100644 --- a/src/test/isolation/isolationtester.c +++ b/src/test/isolation/isolationtester.c @@ -81,7 +81,7 @@ main(int argc, char **argv) puts("isolationtester (PostgreSQL) " PG_VERSION); exit(0); default: - fprintf(stderr, "Usage: isolationtester [-n] [CONNINFO]\n"); + fprintf(stderr, "Usage: isolationtester [CONNINFO]\n"); return EXIT_FAILURE; } } @@ -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-C200BF6B5742@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.40.0