Re: foreign key locks - Mailing list pgsql-hackers

On Thursday, October 18, 2012 09:58:20 PM Alvaro Herrera wrote:
> Here is version 22 of this patch.  This version contains fixes to issues
> reported by Andres, as well as a rebase to latest master.

Ok, I now that pgconf.eu has ended I am starting to do a real review:

* Is it ok to make FOR UPDATE somewhat weaker than before? In 9.2 and earlier 
you could be sure that if you FOR UPDATE'ed a row you could delete it. Unless 
I miss something now this will not block somebody else acquiring a FOR KEY 
SHARE lock, so this guarantee is gone.
This seems likely to introduce subtle problems in user applications.

I propose renaming FOR UPDATE => FOR NO KEY UPDATE, FOR KEY UPDATE => FOR 
UPDATE or similar (or PREVENT KEY UPDATE?). That keeps the old meaning of FOR 
UPDATE.

You write "SELECT FOR UPDATE is a standards-compliant exclusive lock". I 
didn't really find all that much about the semantics of FOR UPDATE on cursors 
in the standard other than "The operations of update and delete are permitted 
for updatable cursors, subject to constraining Access Rules.". 

* I would welcome adding some explanatory comments about the point of 
TupleLockExtraInfo and MultiXactStatusLock at the respective definition.

* Why do we have the HEAP_XMAX_IS_MULTI && HEAP_XMAX_LOCK_ONLY case?

* I think some of the longer comments could use the attention of a native 
speaker, unfortunately thats not me.

* I am still uncomfortable with the FOR SHARE deoptimization. I have seen 
people lock larger parts of their table to make some READ COMMITTED things 
actually safe.
Is there any problem retaining the non XMAX_IS_MULTI behaviour except space in 
infomask? That seems solveable by something like

#define HEAP_XMAX_SHR_LOCK     0x0010
#define HEAP_XMAX_EXCL_LOCK        0x0040
#define HEAP_XMAX_KEYSHR_LOCK   (HEAP_XMAX_SHR_LOCK  | HEAP_XMAX_EXCL_LOCK)

and somewhat more complex expressions when testing the locks ((infomask & 
HEAP_XMAX_KEYSHR_LOCK ) == HEAP_XMAX_KEYSHR_LOCK, etc).

* In heap_lock_tuple's  XMAX_IS_MULTI case
        for (i = 0; i < nmembers; i++)        {            if (TransactionIdIsCurrentTransactionId(members[i].xid))
      {                LockTupleMode    membermode;
 
                membermode = 
TUPLOCK_from_mxstatus(members[i].status);
                if (membermode > mode)                {                    if (have_tuple_lock)
UnlockTupleTuplock(relation,tid, mode);
 
                    pfree(members);                    return HeapTupleMayBeUpdated;                }            }
 }
 

why is it membermode > mode and not membermode >= mode?

* Is the case of a a non-key-update followed by a key-update actually handled 
when doing a heap_lock_tuple with mode = LockTupleKeyShare and follow_updates 
= false? I don't really see how, so it seems to deserve at least a comment.

But then I don't yet understand why follow_update makes sense.

* In heap_lock_tuple with mode == LockTupleUpdate && infomask & 
HEAP_XMAX_IS_MULTI, were leaking members when doing goto l3. Probably not 
relevant, but given the code tries to be careful everywhere else...
We might also leak in the members == 0 case there, not sure yet.

Ok, this is at about 35% of the diff in my second pass, but I just arrived back 
in Berlin, and this seems useful enough on its own...

Andres
-- 
Andres Freund        http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services