Обсуждение: Possible PANIC in PostPrepare_Locks

While looking at the lock code the other day, I noticed this comment in
PostPrepare_Locks():
           /*            * We cannot simply modify proclock->tag.myProc to reassign            * ownership of the lock,
becausethat's part of the hash key and            * the proclock would then be in the wrong hash chain.  So, unlink
      * and delete the old proclock; create a new one with the right            * contents; and link it into place.  We
doit in this order to be            * certain we won't run out of shared memory (the way dynahash.c            * works,
thedeleted object is certain to be available for            * reallocation).            */
 

I believe this comment was correct when it was written, since at that
time the routine would have held an exclusive lock on the entire shared
lock table.  However, the subsequent changes to partition the lock
hashtables broke it, because a partitioned hashtable's freelist is not
partitioned.  This means that another process working in a different
partition could grab the freelist entry before we can recycle it.  In
principle, therefore, it's possible that the HASH_ENTER operation a few
lines below this could find itself out of shared memory and be unable to
make a new proclock table entry for the lock.  If that happens, it
PANICs.

I don't see any very good way to avoid a PANIC on failure there, because
(1) we have already committed the prepared transaction; it's too late to
back out and throw a regular error.  And (2) without any PROCLOCK, the
shared-memory representation of the lock would be corrupt anyhow;
there's no owner.  So what we have to do is ensure it can't fail.

The only moderately clean solution that comes to mind is to create a new
dynahash function with the semantics of "atomically update this entry's
hash key".  It would have the same behavior as the existing
remove-and-reenter code sequence, but it would never put the entry on
the freelist so there's no risk.

BTW, the only reason this is safe at all is that the new entry must
belong in the same partition as the old one, cf proclock_hash().
Possibly this comment should point that out, because I spent a few
minutes wondering if that was another bug.

Also, it looks like we'll need two code paths in PostPrepare_Locks to
deal with the possibility that a conflicting entry already exists?
I'm not sure this is possible, but I'm not sure it's not, either.

In principle this is an ancient bug that we ought to back-patch, but
since we've never heard of this PANIC happening in the field, it
probably is sufficient to fix it in HEAD.  The odds of it biting someone
seem really small.

Thoughts?
        regards, tom lane

On 11.01.2013 04:16, Tom Lane wrote:
> [explanation of a race condition]

Good catch.

> Also, it looks like we'll need two code paths in PostPrepare_Locks to
> deal with the possibility that a conflicting entry already exists?
> I'm not sure this is possible, but I'm not sure it's not, either.

If I understand this correctly, that would mean that someone else is 
holding a lock that conflicts with the lock the 
transaction-being-prepared holds. That shouldn't happen.

- Heikki

Heikki Linnakangas <hlinnakangas@vmware.com> writes:
> On 11.01.2013 04:16, Tom Lane wrote:
>> Also, it looks like we'll need two code paths in PostPrepare_Locks to
>> deal with the possibility that a conflicting entry already exists?
>> I'm not sure this is possible, but I'm not sure it's not, either.

> If I understand this correctly, that would mean that someone else is 
> holding a lock that conflicts with the lock the 
> transaction-being-prepared holds. That shouldn't happen.

After looking at it again I decided the case was impossible because
there can be at most one PROCLOCK for any given lock among those held
by our own PGPROC (else there's already duplicate keys in the proclock
table); so we will create at most one PROCLOCK per lock for the new
dummy PGPROC.  Any collision would imply that the new PGPROC already
held some of those locks, which it surely should not.

So I've committed a patch in which the occurrence of any such collision
will result in a PANIC, but out-of-memory failures are not possible.
        regards, tom lane