Re: foreign key locks

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