Re: Re: [COMMITTERS] pgsql: Introduce group locking to prevent parallel processes from deadl

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Sun, Feb 14, 2016 at 5:26 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Also, after fixing that it would be good to add a comment explaining why
>> it's not fundamentally unsafe for BecomeLockGroupMember() to examine
>> leader->pgprocno without having any relevant lock. AFAICS, that's only
>> okay because the pgprocno fields are never changed after InitProcGlobal,
>> even when a PGPROC is recycled.

> I am sort of disinclined to think that this needs a comment.

I might not either, except that the entire point of that piece of code is
to protect against the possibility that the leader PGPROC vanishes before
we can get this lock. Since you've got extensive comments addressing that
point, it seems appropriate to explain why this one line doesn't invalidate
the whole design ... because it's right on the hairy edge of doing so.
If we did something like memset a PGPROC to all zeroes when we free it,
which in general would seem like a perfectly reasonable thing to do, this
code would be broken (and not in an easy to detect way; it would indeed
be indistinguishable from the way in which it's broken right now).

> Do we
> really have a comment every other place that pgprocno is referenced
> without a lock?

I suspect strongly that there is no other place that attempts to touch any
part of an invalid (freed) PGPROC. If there is, more than likely it's
unsafe.

I don't have time right now to read the patch in detail, but will look
tomorrow. In the meantime, thanks for addressing my concerns.

regards, tom lane