Re: TopoSort() fix

Tom Lane <tgl@sss.pgh.pa.us>

From: Tom Lane <tgl@sss.pgh.pa.us>
To: Robert Haas <robertmhaas@gmail.com>
Cc: Michael Paquier <michael@paquier.xyz>, Andres Freund <andres@anarazel.de>, Alvaro Herrera <alvherre@2ndquadrant.com>, PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Date: 2019-07-30T18:10:13Z
Lists: pgsql-hackers
Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Jul 30, 2019 at 1:44 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Well, there'd be an actual isolation test that they work ;-), if you
>> override the marking.  Admittedly, one test case does not prove that
>> there's no way to crash the system, but that can be said of most
>> parts of Postgres.

> True.  I'm just talking about what we can foresee.

Sure.  But I think what we can foresee is that if there are any bugs
reachable this way, they'd be reachable and need fixing regardless.
We've already established that parallel workers can take and release locks
that their leader isn't holding.  Apparently, they won't take anything
stronger than RowExclusiveLock; but even AccessShare is enough to let a
process participate in all interesting behaviors of the lock manager,
including blocking, being blocked, and being released early by deadlock
resolution.  And the advisory-lock functions are pretty darn thin wrappers
around the lock manager.  So I'm finding it hard to see where there's
incremental risk, even if a user does intentionally bypass the parallel
safety markings.  And what we get in return is an easier way to add tests
for this area.

			regards, tom lane



Commits

  1. Add an isolation test to exercise parallel-worker deadlock resolution.

  2. Mark advisory-lock functions as parallel restricted, not parallel unsafe.

  3. Fix busted logic for parallel lock grouping in TopoSort().