Re: TopoSort() fix - Mailing list pgsql-hackers

[ removing <ruihaij@gmail.com>, as that mailing address seems to be MIA ]

Robert Haas <robertmhaas@gmail.com> writes:
> On Mon, Jul 29, 2019 at 10:56 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> If Robert doesn't weigh in pretty soon, I'll take responsibility for it.

> That's fine, or if you prefer that I commit it, I will.

FYI, I just got done inventing a way to reach that code, and I have
to suspect that it's impossible to do so in production, because under
ordinary circumstances no parallel worker will take any exclusive lock
that isn't already held by its leader.  (If you happen to know an
easy counterexample, let's see it.)

The attached heavily-hacked version of deadlock-soft.spec makes it go by
forcing duplicate advisory locks to be taken in worker processes, which
of course first requires disabling PreventAdvisoryLocksInParallelMode().
I kind of wonder if we should provide some debug-only, here-be-dragons
way to disable that restriction so that we could make this an official
regression test, because I'm now pretty suspicious that none of this code
has ever executed before.

Anyway, armed with this, I was able to prove that HEAD just hangs up
on this test case; apparently the deadlock checker never detects that
the additional holders of the advisory lock need to be rearranged.
And removing that "break" fixes it.

So I'll go commit the break-ectomy, but what do people think about
testing this better?

            regards, tom lane

diff --git a/src/backend/utils/adt/lockfuncs.c b/src/backend/utils/adt/lockfuncs.c
index ffd1970..1e815a2 100644
--- a/src/backend/utils/adt/lockfuncs.c
+++ b/src/backend/utils/adt/lockfuncs.c
@@ -658,10 +658,6 @@ pg_isolation_test_session_is_blocked(PG_FUNCTION_ARGS)
 static void
 PreventAdvisoryLocksInParallelMode(void)
 {
-    if (IsInParallelMode())
-        ereport(ERROR,
-                (errcode(ERRCODE_INVALID_TRANSACTION_STATE),
-                 errmsg("cannot use advisory locks during a parallel operation")));
 }

 /*
diff --git a/src/test/isolation/expected/deadlock-soft.out b/src/test/isolation/expected/deadlock-soft.out
index 24a35da..7abbd19 100644
--- a/src/test/isolation/expected/deadlock-soft.out
+++ b/src/test/isolation/expected/deadlock-soft.out
@@ -1,17 +1,47 @@
 Parsed test spec with 4 sessions

 starting permutation: d1a1 d2a2 e1l e2l d1a2 d2a1 d1c e1c d2c e2c
-step d1a1: LOCK TABLE a1 IN ACCESS SHARE MODE;
-step d2a2: LOCK TABLE a2 IN ACCESS SHARE MODE;
-step e1l: LOCK TABLE a1 IN ACCESS EXCLUSIVE MODE; <waiting ...>
-step e2l: LOCK TABLE a2 IN ACCESS EXCLUSIVE MODE; <waiting ...>
-step d1a2: LOCK TABLE a2 IN ACCESS SHARE MODE; <waiting ...>
-step d2a1: LOCK TABLE a1 IN ACCESS SHARE MODE; <waiting ...>
+step d1a1: select lock_share(1,x) from bigt limit 1;
+lock_share
+
+1
+step d2a2: select lock_share(2,x) from bigt limit 1;
+lock_share
+
+1
+step e1l: select lock_excl(1,x) from bigt limit 1; <waiting ...>
+step e2l: select lock_excl(2,x) from bigt limit 1; <waiting ...>
+step d1a2: SET force_parallel_mode = on;
+set parallel_setup_cost=0;
+set parallel_tuple_cost=0;
+set min_parallel_table_scan_size=0;
+set parallel_leader_participation=off;
+set max_parallel_workers_per_gather=4;
+ select sum(lock_share(2,x)) from bigt; <waiting ...>
+step d2a1: SET force_parallel_mode = on;
+set parallel_setup_cost=0;
+set parallel_tuple_cost=0;
+set min_parallel_table_scan_size=0;
+set parallel_leader_participation=off;
+set max_parallel_workers_per_gather=4;
+ select sum(lock_share(1,x)) from bigt; <waiting ...>
 step d1a2: <... completed>
+sum
+
+10000
 step d1c: COMMIT;
 step e1l: <... completed>
-step e1c: COMMIT;
+lock_excl
+
+1
 step d2a1: <... completed>
+sum
+
+10000
+step e1c: COMMIT;
 step d2c: COMMIT;
 step e2l: <... completed>
+lock_excl
+
+1
 step e2c: COMMIT;
diff --git a/src/test/isolation/specs/deadlock-soft.spec b/src/test/isolation/specs/deadlock-soft.spec
index 49d16e0..b03f0c7 100644
--- a/src/test/isolation/specs/deadlock-soft.spec
+++ b/src/test/isolation/specs/deadlock-soft.spec
@@ -1,3 +1,12 @@
+# Test deadlock resolution with parallel process groups.
+
+# It's fairly hard to get parallel worker processes to block on locks,
+# since generally they don't want any locks their leader didn't already
+# take.  We cheat like mad here by making a function that takes a lock,
+# and is incorrectly marked immutable so that it can execute in a worker.
+# (This also requires disabling the lockfuncs.c code that prevents that.)
+
+# Otherwise, this is morally equivalent to standard deadlock-soft.spec:
 # Four-process deadlock with two hard edges and two soft edges.
 # d2 waits for e1 (soft edge), e1 waits for d1 (hard edge),
 # d1 waits for e2 (soft edge), e2 waits for d2 (hard edge).
@@ -6,35 +15,67 @@

 setup
 {
-  CREATE TABLE a1 ();
-  CREATE TABLE a2 ();
+  create function lock_share(int,int) returns int language sql as
+  'select pg_advisory_xact_lock_shared($1); select 1;' immutable parallel safe;
+
+  create function lock_excl(int,int) returns int language sql as
+  'select pg_advisory_xact_lock($1); select 1;' immutable parallel safe;
+
+  create table bigt as select x, x % 3 as x3 from generate_series(1, 10000) x;
+  analyze bigt;
 }

 teardown
 {
-  DROP TABLE a1, a2;
+  drop function lock_share(int,int);
+  drop function lock_excl(int,int);
+  drop table bigt;
 }

 session "d1"
-setup        { BEGIN; SET deadlock_timeout = '10s'; }
-step "d1a1"    { LOCK TABLE a1 IN ACCESS SHARE MODE; }
-step "d1a2"    { LOCK TABLE a2 IN ACCESS SHARE MODE; }
+setup        { BEGIN isolation level repeatable read;
+  SET force_parallel_mode = off;
+  SET deadlock_timeout = '10s';
+}
+step "d1a1"    { select lock_share(1,x) from bigt limit 1; }
+step "d1a2"    {  SET force_parallel_mode = on;
+set parallel_setup_cost=0;
+set parallel_tuple_cost=0;
+set min_parallel_table_scan_size=0;
+set parallel_leader_participation=off;
+set max_parallel_workers_per_gather=4;
+ select sum(lock_share(2,x)) from bigt; }
 step "d1c"    { COMMIT; }

 session "d2"
-setup        { BEGIN; SET deadlock_timeout = '10ms'; }
-step "d2a2"    { LOCK TABLE a2 IN ACCESS SHARE MODE; }
-step "d2a1"    { LOCK TABLE a1 IN ACCESS SHARE MODE; }
+setup        { BEGIN isolation level repeatable read;
+  SET force_parallel_mode = off;
+  SET deadlock_timeout = '1s';
+}
+step "d2a2"    { select lock_share(2,x) from bigt limit 1; }
+step "d2a1"    {  SET force_parallel_mode = on;
+set parallel_setup_cost=0;
+set parallel_tuple_cost=0;
+set min_parallel_table_scan_size=0;
+set parallel_leader_participation=off;
+set max_parallel_workers_per_gather=4;
+ select sum(lock_share(1,x)) from bigt; }
 step "d2c"    { COMMIT; }

 session "e1"
-setup        { BEGIN; SET deadlock_timeout = '10s'; }
-step "e1l"    { LOCK TABLE a1 IN ACCESS EXCLUSIVE MODE; }
+setup        { BEGIN isolation level repeatable read;
+  SET force_parallel_mode = on;
+  SET deadlock_timeout = '10s';
+}
+step "e1l"    { select lock_excl(1,x) from bigt limit 1; }
 step "e1c"    { COMMIT; }

 session "e2"
-setup        { BEGIN; SET deadlock_timeout = '10s'; }
-step "e2l"    { LOCK TABLE a2 IN ACCESS EXCLUSIVE MODE; }
+setup        { BEGIN isolation level repeatable read;
+  SET force_parallel_mode = on;
+  SET deadlock_timeout = '10s';
+}
+step "e2l"    { select lock_excl(2,x) from bigt limit 1; }
 step "e2c"    { COMMIT; }

 permutation "d1a1" "d2a2" "e1l" "e2l" "d1a2" "d2a1" "d1c" "e1c" "d2c" "e2c"