On Mon, Feb 15, 2016 at 7:55 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:
> Robert Haas <robertmh...@gmail.com> writes:
>> On Sun, Feb 14, 2016 at 5:26 PM, Tom Lane <t...@sss.pgh.pa.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).

OK.  Well, I'm happy to have you add a comment in a patch of your own,
or suggest something to include in mine.  I'm less sure I can write
something independently that you'll like.

>> 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.

Sure thing.  I appreciate the review.

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

Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to