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: http://www.postgresql.org/mailpref/pgsql-hackers
