From 78caa0b89b2611e1a6a8e27d88761a6f5ae5ce8e Mon Sep 17 00:00:00 2001 From: nkey Date: Tue, 14 Jan 2025 12:09:18 +0100 Subject: [PATCH v2] isolation tester: Fix synchronization of steps with blockers Fix a race condition in the isolation tester where steps with blockers could complete in an unexpected order. Previously, step completion was determined by checking if the blocking step was active in its session. This led to inconsistent behavior when steps were ordered differently in the permutation. The fix changes the completion logic to track the number of incomplete instances of each step, allowing more precise control over step synchronization. This ensures that a step with a blocker waits for all instances of its blocking step to complete before proceeding, regardless of the execution order in the permutation. As a side effect, the 'used' boolean field in the Step struct was replaced with 'non_complete' counter because there is not sense to keep both. Test case added to verify proper synchronization behavior with injection points in different orderings. --- src/test/isolation/README | 6 +- src/test/isolation/isolationtester.c | 16 +-- src/test/isolation/isolationtester.h | 2 +- src/test/isolation/specparse.y | 2 +- src/test/modules/injection_points/Makefile | 2 +- .../injection_points/expected/markers.out | 101 ++++++++++++++++++ src/test/modules/injection_points/meson.build | 1 + .../injection_points/specs/markers.spec | 45 ++++++++ 8 files changed, 163 insertions(+), 12 deletions(-) create mode 100644 src/test/modules/injection_points/expected/markers.out create mode 100644 src/test/modules/injection_points/specs/markers.spec diff --git a/src/test/isolation/README b/src/test/isolation/README index 5818ca50038..33f929b200f 100644 --- a/src/test/isolation/README +++ b/src/test/isolation/README @@ -183,9 +183,9 @@ A marker consisting solely of a step name indicates that this step may not be reported as completing until that other step has completed. This allows stabilizing cases where two queries might be seen to complete in either order. Note that this step can be *launched* before the other -step has completed. (If the other step is used more than once in the -current permutation, this step cannot complete while any of those -instances is active.) +step has completed or launched. (If the other step is used more than once +in the current permutation, this step cannot complete while any of those +instances are not yet complete.) A marker of the form " notices " (where is a positive integer) indicates that this step may not be reported as diff --git a/src/test/isolation/isolationtester.c b/src/test/isolation/isolationtester.c index e01c0c9de93..676563f959d 100644 --- a/src/test/isolation/isolationtester.c +++ b/src/test/isolation/isolationtester.c @@ -319,8 +319,8 @@ check_testspec(TestSpec *testspec) } pstep->step = *this; - /* Mark the step used, for check below */ - pstep->step->used = true; + /* Mark the step used by incrementing non-complete counter*/ + pstep->step->non_complete++; } /* @@ -378,7 +378,7 @@ check_testspec(TestSpec *testspec) { for (i = 0; i < nallsteps; i++) { - if (!allsteps[i]->used) + if (allsteps[i]->non_complete == 0) fprintf(stderr, "unused step name: %s\n", allsteps[i]->name); } } @@ -568,6 +568,11 @@ run_permutation(TestSpec *testspec, int nsteps, PermutationStep **steps) } } + for (i = 0; i < nsteps; i++) + steps[i]->step->non_complete = 0; + for (i = 0; i < nsteps; i++) + steps[i]->step->non_complete++; + /* Perform steps */ for (i = 0; i < nsteps; i++) { @@ -1002,6 +1007,7 @@ try_complete_step(TestSpec *testspec, PermutationStep *pstep, int flags) printf("step %s: <... completed>\n", step->name); else printf("step %s: %s\n", step->name, step->sql); + step->non_complete--; while ((res = PQgetResult(conn))) { @@ -1093,9 +1099,7 @@ step_has_blocker(PermutationStep *pstep) break; case PSB_OTHER_STEP: /* Block if referenced step is active */ - iconn = &conns[1 + blocker->step->session]; - if (iconn->active_step && - iconn->active_step->step == blocker->step) + if (blocker->step->non_complete > 0) return true; break; case PSB_NUM_NOTICES: diff --git a/src/test/isolation/isolationtester.h b/src/test/isolation/isolationtester.h index 1ef14dc9909..44b4c028a21 100644 --- a/src/test/isolation/isolationtester.h +++ b/src/test/isolation/isolationtester.h @@ -36,7 +36,7 @@ struct Step char *sql; /* These fields are filled by check_testspec(): */ int session; /* identifies owning session */ - bool used; /* has step been used in a permutation? */ + int non_complete; /* number of step execution in permutation which are not yet completed */ }; typedef enum diff --git a/src/test/isolation/specparse.y b/src/test/isolation/specparse.y index 98949a446e5..7705b34dc2e 100644 --- a/src/test/isolation/specparse.y +++ b/src/test/isolation/specparse.y @@ -156,7 +156,7 @@ step: $$->name = $2; $$->sql = $3; $$->session = -1; /* until filled */ - $$->used = false; + $$->non_complete = 0; } ; diff --git a/src/test/modules/injection_points/Makefile b/src/test/modules/injection_points/Makefile index 0753a9df58c..4f47f4a0e7e 100644 --- a/src/test/modules/injection_points/Makefile +++ b/src/test/modules/injection_points/Makefile @@ -13,7 +13,7 @@ PGFILEDESC = "injection_points - facility for injection points" REGRESS = injection_points reindex_conc REGRESS_OPTS = --dlpath=$(top_builddir)/src/test/regress -ISOLATION = basic inplace +ISOLATION = basic inplace markers TAP_TESTS = 1 diff --git a/src/test/modules/injection_points/expected/markers.out b/src/test/modules/injection_points/expected/markers.out new file mode 100644 index 00000000000..136bbae8338 --- /dev/null +++ b/src/test/modules/injection_points/expected/markers.out @@ -0,0 +1,101 @@ +Parsed test spec with 3 sessions + +starting permutation: after before detach1 wakeup1 detach2 wakeup2 +injection_points_attach +----------------------- + +(1 row) + +injection_points_attach +----------------------- + +(1 row) + +step after: SELECT injection_points_run('injection-points-wait-1'); +step before: SELECT injection_points_run('injection-points-wait-2'); +step detach1: SELECT injection_points_detach('injection-points-wait-1'); +injection_points_detach +----------------------- + +(1 row) + +step wakeup1: SELECT injection_points_wakeup('injection-points-wait-1'); +injection_points_wakeup +----------------------- + +(1 row) + +step detach2: SELECT injection_points_detach('injection-points-wait-2'); +injection_points_detach +----------------------- + +(1 row) + +step wakeup2: SELECT injection_points_wakeup('injection-points-wait-2'); +injection_points_wakeup +----------------------- + +(1 row) + +step before: <... completed> +injection_points_run +-------------------- + +(1 row) + +step after: <... completed> +injection_points_run +-------------------- + +(1 row) + + +starting permutation: after wakeup1 before detach1 detach2 wakeup2 +injection_points_attach +----------------------- + +(1 row) + +injection_points_attach +----------------------- + +(1 row) + +step after: SELECT injection_points_run('injection-points-wait-1'); +step wakeup1: SELECT injection_points_wakeup('injection-points-wait-1'); +injection_points_wakeup +----------------------- + +(1 row) + +step before: SELECT injection_points_run('injection-points-wait-2'); +step detach1: SELECT injection_points_detach('injection-points-wait-1'); +injection_points_detach +----------------------- + +(1 row) + +step detach2: SELECT injection_points_detach('injection-points-wait-2'); +injection_points_detach +----------------------- + +(1 row) + +step wakeup2: SELECT injection_points_wakeup('injection-points-wait-2'); +injection_points_wakeup +----------------------- + +(1 row) + +step before: <... completed> +injection_points_run +-------------------- + +(1 row) + +step after: <... completed> +injection_points_run +-------------------- + +(1 row) + diff --git a/src/test/modules/injection_points/meson.build b/src/test/modules/injection_points/meson.build index 989b4db226b..3998bac852f 100644 --- a/src/test/modules/injection_points/meson.build +++ b/src/test/modules/injection_points/meson.build @@ -44,6 +44,7 @@ tests += { 'specs': [ 'basic', 'inplace', + 'markers', ], }, 'tap': { diff --git a/src/test/modules/injection_points/specs/markers.spec b/src/test/modules/injection_points/specs/markers.spec new file mode 100644 index 00000000000..e9f56d7567b --- /dev/null +++ b/src/test/modules/injection_points/specs/markers.spec @@ -0,0 +1,45 @@ +setup +{ + CREATE EXTENSION injection_points; +} +teardown +{ + DROP EXTENSION injection_points; +} + +# Wait happens in the first session, wakeup in the second session. +session s1 +setup { + SELECT injection_points_set_local(); + SELECT injection_points_attach('injection-points-wait-1', 'wait'); +} +step after { SELECT injection_points_run('injection-points-wait-1'); } + +session s2 +setup { + SELECT injection_points_set_local(); + SELECT injection_points_attach('injection-points-wait-2', 'wait'); +} +step before { SELECT injection_points_run('injection-points-wait-2'); } + +session s3 +step wakeup1 { SELECT injection_points_wakeup('injection-points-wait-1'); } +step detach1 { SELECT injection_points_detach('injection-points-wait-1'); } +step wakeup2 { SELECT injection_points_wakeup('injection-points-wait-2'); } +step detach2 { SELECT injection_points_detach('injection-points-wait-2'); } + +permutation + after(before) + before + detach1 + wakeup1 + detach2 + wakeup2 + +permutation + after(before) + wakeup1 + before + detach1 + detach2 + wakeup2 \ No newline at end of file -- 2.43.0