Re: foreign key locks - Mailing list pgsql-hackers

Here's version 23 of this patch, with fixes for the below comments and
a merge to master.

Andres Freund wrote:
> 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.

Okay, in subsequent discussion everyone agreed that this is necessary;
thanks for noticing the problem.  Instead of your suggestion, however, I
used Noah's; luckily I just noticed that it was identical to yours.  So
apparently there is also agreement on this point.  I have updated the
code, comments and docs (and renamed some other stuff to match).

> 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 don't really know where that comes from, but it's a statement I
grabbed from the docs somewhere.

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

Done.  If that's not enough, I would welcome suggestions or specific
question needing clarification.

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

For pg_upgrade.  HEAP_XMAX_LOCK_ONLY is the value previously used by
HEAP_XMAX_SHARED_LOCK, so a database containing such values that is
upgraded will contain that bit pattern.

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

Sorry about that.

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

I haven't changed this yet.  There are bits available in infomask2 that
we could use, if we agree that it is necessary; or, as you say, we could
use two bits to distinguish three different states, but we need to
ensure that pg_upgraded databases will work sanely.

> * In heap_lock_tuple's  XMAX_IS_MULTI case
>
> [snip]
>
> why is it membermode > mode and not membermode >= mode?

Uh, that's a bug.  Fixed.  As noticed in the comment above that snippet,
there was a deadlock possible here.  Maybe I should add a test to ensure
this doesn't happen.

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

I wasn't able to figure out what you think is the problem.

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

Basically heap_lock_tuple can be told by the caller to follow the update
chain to lock subsequent tuples, or not.  Mainly, EvalPlanQual does not
want this to happen, because it does its own update-chain walking.

In all honesty, it is not clear to me that this is the proper solution
to the problem.  Maybe some hacking around ExecLockRows is necessary.

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

Right, plugged.

> We might also leak in the members == 0 case there, not sure yet.

Hm, I don't think GetMultiXactIdMembers really considers the case when 0
members are to be returned.  Maybe that needs some more tweaking.

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

Many thanks.

--
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services