Cleanup isolation specs from unused steps
Hi all,
I have been looking at the isolation tests, and we have in some specs
steps which are defined but not used in any permutations. In order to
detect them, I have been using the attached trick to track which
permutations are used. This allows to find immediately any
over-engineered spec by generating diffs about steps defined by not
used in any permutations. On HEAD, we have six specs entering in this
category.
Would that be useful?
--
Michael
Attachments:
isolation-steps-unused-v1.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/test/isolation/isolationtester.c b/src/test/isolation/isolationtester.c
index f98bb1cf64..402bb1f546 100644
--- a/src/test/isolation/isolationtester.c
+++ b/src/test/isolation/isolationtester.c
@@ -239,6 +239,17 @@ main(int argc, char **argv)
*/
run_testspec(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);
+ }
+
return 0;
}
@@ -467,7 +478,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; }
Michael Paquier <michael@paquier.xyz> writes:
I have been looking at the isolation tests, and we have in some specs
steps which are defined but not used in any permutations.
Hmm, might any of those represent actual bugs? Or are they just
leftovers from test development?
In order to
detect them, I have been using the attached trick to track which
permutations are used. This allows to find immediately any
over-engineered spec by generating diffs about steps defined by not
used in any permutations. On HEAD, we have six specs entering in this
category.
Seems like a good idea; I'm surprised we've got so many cases.
regards, tom lane
On Mon, Aug 19, 2019 at 1:08 AM Michael Paquier <michael@paquier.xyz> wrote:
Hi all,
I have been looking at the isolation tests, and we have in some specs
steps which are defined but not used in any permutations. In order to
detect them, I have been using the attached trick to track which
permutations are used. This allows to find immediately any
over-engineered spec by generating diffs about steps defined by not
used in any permutations. On HEAD, we have six specs entering in this
category.Would that be useful?
I think it is useful.
could you do the check that all steps have been used in dry_run mode
instead of when running the tests for real?
--
Melanie Plageman
On Mon, Aug 19, 2019 at 11:02:42AM -0400, Tom Lane wrote:
Michael Paquier <michael@paquier.xyz> writes:
I have been looking at the isolation tests, and we have in some specs
steps which are defined but not used in any permutations.Hmm, might any of those represent actual bugs? Or are they just
leftovers from test development?
I cannot yet enter the minds of each test author back this much in
time, but I think that's a mix of both. When working on a new
isolation spec, I personally tend to do a lot of copy-pasting of the
same queries for multiple sessions and then manipulate the
permutations to produce a set of useful tests. It is rather easy to
forget to remove some steps when doing that. I guess that's what
happened with tuplelock-upgrade, insert-conflict-do-update* and
freeze-the-dead.
--
Michael
On Mon, Aug 19, 2019 at 10:23:19AM -0700, Melanie Plageman wrote:
Could you do the check that all steps have been used in dry_run mode
instead of when running the tests for real?
Sure, I was hesitating to do so. I have no issue in moving the check
into run_testspec(). So done as attached.
It is rather a pain to pass down custom options to isolationtester.
For example, I have tested the updated version attached after
hijacking -n into isolation_start_test(). Ugly hack, but for testing
that's enough. Do you make use of this tool in a particular way in
greenplum? Just wondering.
(Could it make sense to have long options for isolationtester by the
way?)
--
Michael
Attachments:
isolation-steps-unused-v2.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/test/isolation/isolationtester.c b/src/test/isolation/isolationtester.c
index f98bb1cf64..f8ec7ca7b4 100644
--- a/src/test/isolation/isolationtester.c
+++ b/src/test/isolation/isolationtester.c
@@ -251,10 +251,24 @@ 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. This check happens here so as dry-run is able
+ * to use it.
+ */
+ for (i = 0; i < testspec->nallsteps; i++)
+ {
+ if (!testspec->allsteps[i]->used)
+ fprintf(stderr, "unused step name: %s\n",
+ testspec->allsteps[i]->name);
+ }
}
/*
@@ -457,7 +471,11 @@ run_permutation(TestSpec *testspec, int nsteps, Step **steps)
{
printf("permutation");
for (i = 0; i < nsteps; i++)
+ {
+ /* Track the permutation as in-use */
+ steps[i]->used = true;
printf(" \"%s\"", steps[i]->name);
+ }
printf("\n");
return;
}
@@ -467,7 +485,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; }
On 2019-Aug-20, Michael Paquier wrote:
On Mon, Aug 19, 2019 at 10:23:19AM -0700, Melanie Plageman wrote:
Could you do the check that all steps have been used in dry_run mode
instead of when running the tests for real?Sure, I was hesitating to do so. I have no issue in moving the check
into run_testspec(). So done as attached.
I created the dry-run mode to be able to easily generate the set of
possible permutations for a new test, then edit the result and put it
back in the spec file; but after the deadlock tests were added (with
necessary hacking of the lock-detection in isolationtester) that manner
of operation became almost completely useless. Maybe we need to rethink
what facilities isolationtester offers -- possibly making dry-run have a
completely different behavior than currently, which I doubt anybody is
using.
All that being said, I have no objections to this patch (but I didn't
review it closely).
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Tue, Aug 20, 2019 at 12:34:45AM -0400, Alvaro Herrera wrote:
I created the dry-run mode to be able to easily generate the set of
possible permutations for a new test, then edit the result and put it
back in the spec file; but after the deadlock tests were added (with
necessary hacking of the lock-detection in isolationtester) that manner
of operation became almost completely useless. Maybe we need to rethink
what facilities isolationtester offers -- possibly making dry-run have a
completely different behavior than currently, which I doubt anybody is
using.
I am not sure exactly how it could be redesigned, and with n!
permutations that easily leads to bloat of the generated output. I
think that --dry-run (well -n) is a bit misleading as option name
though as it prints only permutations. Still, keeping it around has
no real cost, so it is not a big deal.
(Looking at the gpdb code, it does not seem to be used.)
All that being said, I have no objections to this patch (but I didn't
review it closely).
Thanks.
--
Michael
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
On 2019-Aug-20, Michael Paquier wrote:
On Mon, Aug 19, 2019 at 10:23:19AM -0700, Melanie Plageman wrote:
Could you do the check that all steps have been used in dry_run mode
instead of when running the tests for real?
Sure, I was hesitating to do so. I have no issue in moving the check
into run_testspec(). So done as attached.
I created the dry-run mode to be able to easily generate the set of
possible permutations for a new test, then edit the result and put it
back in the spec file; but after the deadlock tests were added (with
necessary hacking of the lock-detection in isolationtester) that manner
of operation became almost completely useless. Maybe we need to rethink
what facilities isolationtester offers -- possibly making dry-run have a
completely different behavior than currently, which I doubt anybody is
using.
Hm, does that mean that this version of the patch would fail to warn
during a normal run? Doesn't sound good, since as Alvaro says,
hardly anyone uses dry-run.
If you can warn in both cases, that'd be OK perhaps. But Alvaro's
description of the intended use of dry-run makes it sound like
it would be expected for there to be unreferenced steps, since there'd
be no permutations yet in the input.
regards, tom lane
On 2019-Aug-20, Tom Lane wrote:
If you can warn in both cases, that'd be OK perhaps. But Alvaro's
description of the intended use of dry-run makes it sound like
it would be expected for there to be unreferenced steps, since there'd
be no permutations yet in the input.
Well, Heikki/Kevin's original intention was that if no permutations are
specified, then all possible permutations are generated internally and
the spec is run with that. The intended use of dry-run was to do just
that (generate all possible permutations) and print that list, so that
it could be trimmed down by the test author. In this mode of operation,
all steps are always used, so there'd be no warning printed. However,
when a test file has a largish number of steps then the list is, um, a
bit long. Before the deadlock-test hacking, you could run with such a
list anyway and any permutations that caused a blockage would be
reported right away as an invalid permutation -- quick enough.
Currently it sleeps for absurdly long on those cases, so this is no
longer feasible.
This is why I say that the current dry-run mode could be removed with no
loss of useful functionality.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Tue, Aug 20, 2019 at 09:54:56AM -0400, Alvaro Herrera wrote:
On 2019-Aug-20, Tom Lane wrote:
If you can warn in both cases, that'd be OK perhaps. But Alvaro's
description of the intended use of dry-run makes it sound like
it would be expected for there to be unreferenced steps, since there'd
be no permutations yet in the input.
v2 of the patch warns of any unused steps in dry-run mode. If no
permutations are defined in the input spec, all steps get used
(actually that's not a factorial as the per-session ordering is
preserved), so I would expect no warnings to be generated and the
patch does that. If the test includes some permutations, then I would
expect dry-run to complain about the steps which are defined in the
spec but not used. The patch also does that. Do you see a problem
with that?
Well, Heikki/Kevin's original intention was that if no permutations are
specified, then all possible permutations are generated internally and
the spec is run with that. The intended use of dry-run was to do just
that (generate all possible permutations) and print that list, so that
it could be trimmed down by the test author. In this mode of operation,
all steps are always used, so there'd be no warning printed. However,
when a test file has a largish number of steps then the list is, um, a
bit long. Before the deadlock-test hacking, you could run with such a
list anyway and any permutations that caused a blockage would be
reported right away as an invalid permutation -- quick enough.
Currently it sleeps for absurdly long on those cases, so this is no
longer feasible.This is why I say that the current dry-run mode could be removed with no
loss of useful functionality.
Hmm. Even if one does not do something deadlock-specific, the list
printed could still be useful, no? This for example works now that I
look at it:
./isolationtester -n < specs/my_spec.spec
I am wondering if we should not actually keep dry_run, but rename it
to something like --print-permutations to print the set of
permutations which would be run as part of the spec, and also have an
option which is able to print out all permutations possible, like
--print-all-permutations. Simply ripping out the mode would be fine
by me as it does not seem to be used, but keeping it around does not
induce really much extra maintenance cost.
--
Michael
On Mon, Aug 19, 2019 at 7:01 PM Michael Paquier <michael@paquier.xyz> wrote:
It is rather a pain to pass down custom options to isolationtester.
For example, I have tested the updated version attached after
hijacking -n into isolation_start_test(). Ugly hack, but for testing
that's enough. Do you make use of this tool in a particular way in
greenplum? Just wondering.(Could it make sense to have long options for isolationtester by the
way?)
In Greenplum, we mainly add new tests to a separate isolation
framework (called isolation2) which uses a completely different
syntax. It doesn't use isolationtester at all. So, I haven't had a use
case to add long options to isolationtester yet :)
--
Melanie Plageman
On Tue, Aug 20, 2019 at 6:34 PM Michael Paquier <michael@paquier.xyz> wrote:
On Tue, Aug 20, 2019 at 09:54:56AM -0400, Alvaro Herrera wrote:
On 2019-Aug-20, Tom Lane wrote:
If you can warn in both cases, that'd be OK perhaps. But Alvaro's
description of the intended use of dry-run makes it sound like
it would be expected for there to be unreferenced steps, since there'd
be no permutations yet in the input.v2 of the patch warns of any unused steps in dry-run mode. If no
permutations are defined in the input spec, all steps get used
(actually that's not a factorial as the per-session ordering is
preserved), so I would expect no warnings to be generated and the
patch does that. If the test includes some permutations, then I would
expect dry-run to complain about the steps which are defined in the
spec but not used. The patch also does that. Do you see a problem
with that?Well, Heikki/Kevin's original intention was that if no permutations are
specified, then all possible permutations are generated internally and
the spec is run with that. The intended use of dry-run was to do just
that (generate all possible permutations) and print that list, so that
it could be trimmed down by the test author. In this mode of operation,
all steps are always used, so there'd be no warning printed. However,
when a test file has a largish number of steps then the list is, um, a
bit long. Before the deadlock-test hacking, you could run with such a
list anyway and any permutations that caused a blockage would be
reported right away as an invalid permutation -- quick enough.
Currently it sleeps for absurdly long on those cases, so this is no
longer feasible.This is why I say that the current dry-run mode could be removed with no
loss of useful functionality.Hmm. Even if one does not do something deadlock-specific, the list
printed could still be useful, no? This for example works now that I
look at it:
./isolationtester -n < specs/my_spec.specI am wondering if we should not actually keep dry_run, but rename it
to something like --print-permutations to print the set of
permutations which would be run as part of the spec, and also have an
option which is able to print out all permutations possible, like
--print-all-permutations. Simply ripping out the mode would be fine
by me as it does not seem to be used, but keeping it around does not
induce really much extra maintenance cost.
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.
+1 to renaming it to --print-permutations and, potentially,
adding --print-all-permutations
--
Melanie Plageman
On 2019-Aug-21, Melanie Plageman wrote:
In Greenplum, we mainly add new tests to a separate isolation
framework (called isolation2) which uses a completely different
syntax. It doesn't use isolationtester at all. So, I haven't had a use
case to add long options to isolationtester yet :)
Is that other framework somehow more capable?
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
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
Attachments:
v3-0001-Remove-dry-run-mode-from-isolationtester.patchtext/x-diff; charset=us-asciiDownload
From d0e756bba3668af0b1f40b0b1d5bf198b83a97fd Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@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
v3-0002-Improve-detection-of-unused-steps-in-isolation-sp.patchtext/x-diff; charset=us-asciiDownload
From 1ee92ce4cf359c9f6e7f8ce32e4057ed9bb0412e Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@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-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.23.0
On Wed, Aug 21, 2019 at 12:16 PM Alvaro Herrera <alvherre@2ndquadrant.com>
wrote:
On 2019-Aug-21, Melanie Plageman wrote:
In Greenplum, we mainly add new tests to a separate isolation
framework (called isolation2) which uses a completely different
syntax. It doesn't use isolationtester at all. So, I haven't had a use
case to add long options to isolationtester yet :)Is that other framework somehow more capable?
So, there is some historical context as to why it is a separate test suite.
And some of the differences are specific to Greenplum -- e.g. needing to
connect
to a specific database in "utility mode" to do something.
However, the other differences are actually pretty handy and would be
applicable
to upstream as well.
We use a different syntax than the isolation framework and have some nice
features. Most notably, explicit control over blocking.
The syntax for what would be a "step" in isolation is like this:
[<#>[flag]:] <sql> | ! <shell scripts or command>
where # is the session number and flags include the following:
&: expect blocking behavior
: running in background without blocking
<: join an existing session
q: quit the given session
See the script [1]https://github.com/greenplum-db/gpdb/blob/master/src/test/isolation2/sql_isolation_testcase.py for parsing the test cases for more details on the
syntax and
capabilities (it is in Python).
[1]: https://github.com/greenplum-db/gpdb/blob/master/src/test/isolation2/sql_isolation_testcase.py
https://github.com/greenplum-db/gpdb/blob/master/src/test/isolation2/sql_isolation_testcase.py
--
Melanie Plageman
On Thu, Aug 22, 2019 at 1:45 PM Melanie Plageman
<melanieplageman@gmail.com> wrote:
On Wed, Aug 21, 2019 at 12:16 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
On 2019-Aug-21, Melanie Plageman wrote:
In Greenplum, we mainly add new tests to a separate isolation
framework (called isolation2) which uses a completely different
syntax. It doesn't use isolationtester at all. So, I haven't had a use
case to add long options to isolationtester yet :)Is that other framework somehow more capable?
So, there is some historical context as to why it is a separate test suite.
And some of the differences are specific to Greenplum -- e.g. needing to connect
to a specific database in "utility mode" to do something.However, the other differences are actually pretty handy and would be applicable
to upstream as well.
We use a different syntax than the isolation framework and have some nice
features. Most notably, explicit control over blocking.
Asim submitted this framework just yesterday:
/messages/by-id/CANXE4TdxdESX1jKw48xet-5GvBFVSq=4cgNeioTQff372KO45A@mail.gmail.com
-- Rob
Show quoted text
The syntax for what would be a "step" in isolation is like this:
[<#>[flag]:] <sql> | ! <shell scripts or command>
where # is the session number and flags include the following:
&: expect blocking behavior
: running in background without blocking
<: join an existing session
q: quit the given sessionSee the script [1] for parsing the test cases for more details on the syntax and
capabilities (it is in Python).[1] https://github.com/greenplum-db/gpdb/blob/master/src/test/isolation2/sql_isolation_testcase.py
--
Melanie Plageman
On Thu, Aug 22, 2019 at 10:20:48AM -0700, Melanie Plageman wrote:
So, there is some historical context as to why it is a separate test suite.
And some of the differences are specific to Greenplum -- e.g. needing to
connect to a specific database in "utility mode" to do something.
What is "utility mode"?
The syntax for what would be a "step" in isolation is like this:
[<#>[flag]:] <sql> | ! <shell scripts or command>
where # is the session number and flags include the following:
&: expect blocking behavior
: running in background without blocking
<: join an existing session
q: quit the given session
These could be transposed as new meta commands for the existing
specs? Of course not as "step" per-se, but new dedicated commands?
See the script [1] for parsing the test cases for more details on the
syntax and capabilities (it is in Python).
Hmm. The bar to add a new hard language dependency in the test
suites is very high. I am not sure that we'd want something with a
python dependency for the tests, also knowing how Python likes
breaking compatibility (isolation2_main() also mentions a dependency
to Python).
--
Michael
On Thu, Aug 22, 2019 at 6:53 PM Michael Paquier <michael@paquier.xyz> wrote:
On Thu, Aug 22, 2019 at 10:20:48AM -0700, Melanie Plageman wrote:
So, there is some historical context as to why it is a separate test
suite.
And some of the differences are specific to Greenplum -- e.g. needing to
connect to a specific database in "utility mode" to do something.What is "utility mode"?
It's a connection parameter that allows you to connect to a single Postgres
node
in a Greenplum cluster. I only included it as an example of the kind of
"Greenplum-specific" things that are in the test framework.
The syntax for what would be a "step" in isolation is like this:
[<#>[flag]:] <sql> | ! <shell scripts or command>
where # is the session number and flags include the following:
&: expect blocking behavior
: running in background without blocking
<: join an existing session
q: quit the given sessionThese could be transposed as new meta commands for the existing
specs? Of course not as "step" per-se, but new dedicated commands?
Yes, I think you could definitely add some of the flags as meta-commands for
existing steps -- the current implementation of "blocking" for isolation is
really limiting.
However, features aside, I actually find the existing "step" syntax in
isolation
clunkier than the syntax used in Greenplum's "isolation2" framework.
See the script [1] for parsing the test cases for more details on the
syntax and capabilities (it is in Python).Hmm. The bar to add a new hard language dependency in the test
suites is very high. I am not sure that we'd want something with a
python dependency for the tests, also knowing how Python likes
breaking compatibility (isolation2_main() also mentions a dependency
to Python).
Agreed, I don't think it needs to be in Python.
My point was that some of our "isolation2" framework has to be different
because
it is enabling us to test features that are in Greenplum and not in
Postgres.
However, many of the features it has would actually be really handy to have
for
testing Postgres.
It wasn't initially suggested upstream because it is actually mainly ported
from
a separate standalone testing framework that was written at Greenplum in
Python.
I've heard Greenplum folks talk about re-writing our "isolation2" framework
in
(probably) C and making it a better fit to contribute. It's definitely on
my wishlist.
--
Melanie Plageman
On Thu, Aug 22, 2019 at 09:19:47PM -0700, Melanie Plageman wrote:
It's a connection parameter that allows you to connect to a single Postgres
node in a Greenplum cluster. I only included it as an example of the kind of
"Greenplum-specific" things that are in the test framework.
Ah, I see. I had the same stuff for XC to connect to data nodes.
I've heard Greenplum folks talk about re-writing our "isolation2" framework
in (probably) C and making it a better fit to contribute. It's definitely on
my wishlist.
Nice to hear that.
--
Michael
On Thu, Aug 22, 2019 at 12:47 AM Alvaro Herrera <alvherre@2ndquadrant.com>
wrote:
On 2019-Aug-21, Melanie Plageman wrote:
In Greenplum, we mainly add new tests to a separate isolation
framework (called isolation2) which uses a completely different
syntax. It doesn't use isolationtester at all. So, I haven't had a use
case to add long options to isolationtester yet :)Is that other framework somehow more capable?
The ability to declare a step as blocking, as Melanie mentioned upthread
("&" prefix), makes it more capable. The tester, when encounters such a
step, makes sure that the command in that step is blocking and moves on to
run subsequent commands. The isolationtester, on the other hand, treats a
step as blocking only when the command waits on a lock. That seems
restrictive. E.g. what if a command waits on a latch, as part of a valid
interleaving of concurrent transactions? The isolation tester cannot
detect such a case and it will keep waiting and eventually fail the test
with a timeout.
As part of the fault injector patch set [1]/messages/by-id/CANXE4TdxdESX1jKw48xet-5GvBFVSq=4cgNeioTQff372KO45A@mail.gmail.com, I added a new "blocking"
keyword to isolation grammar so that a step can be declared as blocking.
See patch 0002-Add-syntax-to-declare-a-step-that-is-expected-to-block.
Asim
[1]: /messages/by-id/CANXE4TdxdESX1jKw48xet-5GvBFVSq=4cgNeioTQff372KO45A@mail.gmail.com
/messages/by-id/CANXE4TdxdESX1jKw48xet-5GvBFVSq=4cgNeioTQff372KO45A@mail.gmail.com
On 2019-Aug-23, Asim R P wrote:
As part of the fault injector patch set [1], I added a new "blocking"
keyword to isolation grammar so that a step can be declared as blocking.
See patch 0002-Add-syntax-to-declare-a-step-that-is-expected-to-block.
One point to that implementation is that in that design a step is
globally declared to be blocking, but in reality that's the wrong way to
see things: a step might block in some permutations and not others. So
I think we should do as Michael suggested: it's the permutation that has
to have a way to mark a given step as blocking, not the step itself.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
(Re-adding pgsql-hackers)
On Fri, Aug 23, 2019 at 05:28:35PM +0530, Asim R P wrote:
Both the patches look good to me, thank you. +1 to removing dry-run and
tracking unused steps.
Thanks, both have been committed. There have been issues with the
isolation tests of logical decoding I have taken care of.
--
Michael
Import Notes
Reply to msg id not found: CANXE4Tcyyb3bd8inf3cGeFW+b3o7DMqv8k0tOeYoc1CE6eqw@mail.gmail.com
On Fri, Aug 23, 2019 at 9:08 PM Alvaro Herrera <alvherre@2ndquadrant.com>
wrote:
On 2019-Aug-23, Asim R P wrote:
As part of the fault injector patch set [1], I added a new "blocking"
keyword to isolation grammar so that a step can be declared as blocking.
See patch 0002-Add-syntax-to-declare-a-step-that-is-expected-to-block.One point to that implementation is that in that design a step is
globally declared to be blocking, but in reality that's the wrong way to
see things: a step might block in some permutations and not others. So
I think we should do as Michael suggested: it's the permutation that has
to have a way to mark a given step as blocking, not the step itself.
Thank you for the feedback. I've changed patch 0002 accordingly, please
take another look:
/messages/by-id/CANXE4TdvSi7Yia_5sV82+MHf0WcUSN9u6_X8VEUBv-YStphd=Q@mail.gmail.com
Asim
On Tue, Aug 27, 2019 at 07:05:50PM +0530, Asim R P wrote:
Thank you for the feedback. I've changed patch 0002 accordingly, please
take another look:
/messages/by-id/CANXE4TdvSi7Yia_5sV82+MHf0WcUSN9u6_X8VEUBv-YStphd=Q@mail.gmail.com
Thanks! Let's move the discussion on the other thread then.
--
Michael