Re: TopoSort() fix

On Tue, Jul 30, 2019 at 10:27 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I also looked into whether one could use SELECT FOR UPDATE/SHARE to get
> stronger locks at a tuple level, but that's been blocked off as well.
> You guys really did a pretty good job of locking that down.

Thanks.  We learned from the master.

> After thinking about this for awhile, though, I believe it might be
> reasonable to just remove PreventAdvisoryLocksInParallelMode()
> altogether.  The "parallel unsafe" markings on the advisory-lock
> functions seem like adequate protection against somebody running
> them in a parallel worker.  If you defeat that by calling them from
> a mislabeled-parallel-safe wrapper (as the proposed test case does),
> then any negative consequences are on your own head.  AFAICT the
> only actual negative consequence is that the locks disappear the
> moment the parallel worker exits, so we'd not be opening any large
> holes even to people who rip off the safety cover.
>
> (BTW, why aren't these functions just "parallel restricted"?)

I don't exactly remember why we installed all of these restrictions
any more.  You might be able to find some discussion of it by
searching the archives.  I believe we may have been concerned about
the fact that group locking would cause advisory locks taken in one
process not to conflict with the same advisory lock taken in some
cooperating process, and maybe that would be unwelcome behavior for
someone.  For example, suppose the user defines a function that takes
an advisory lock on the number 1, does a bunch of stuff that should
never happen multiply at the same time, and then releases the lock.
Without parallel query, that will work.  With parallel query, it
won't, because several workers running the same query might run the
same function simultaneously and their locks won't conflict.

But it is really pretty arguable whether we should feel responsible
for that problem.  We could just decide that if you're doing that, and
you don't want the scenario described above to happen, you oughta mark
the function that contains this logic at least PARALLEL RESTRICTED,
and if you don't, then it's your fault for doing a dumb thing.  I
believe when we were early on in the development of this we wanted to
be very conservative lest, ah, someone accuse us of not locking things
down well enough, but maybe at this point parallel query is a
sufficiently well-established concept that we should lighten up on
some cases where we took an overly-stringent line.  If we take that
view, then I'm not sure why these functions couldn't be just marked
PARALLEL SAFE.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company