Re: foreign key locks

Andres Freund wrote:
> On Friday, August 31, 2012 06:59:51 AM Alvaro Herrera wrote:
> > v21 is merged to latest master.
> Ok, I am starting to look at this.
>
> (working with a git merge of alvherre/fklocks into todays master)
>
> In a very first pass as somebody who hasn't followed the discussions in the
> past I took notice of the following things:
>
> * compiles and survives make check

Please verify src/test/isolation's make installcheck as well.

> * README.tuplock jumps in quite fast without any introduction

Hmm .. I'll give that a second look. Maybe some material from the pgcon
paper should be added as introduction.

> * "This is a bit of a performance regression, but since we expect FOR SHARE
> locks to be seldom used, we don’t feel this is a serious problem." makes me a
> bit uneasy, will look at performance separately

Thanks. Keep in mind though that the bits we really want good
performance for is FOR KEY SHARE, which is going to be used in foreign
keys. FOR SHARE is staying just for hypothetical backwards
compatibility. I just found that people is using it, see for example
http://stackoverflow.com/a/3243083

> * Should RelationGetIndexattrBitmap check whether a unique index is referenced
> from a pg_constraint row? Currently we pay part of the overhead independent
> from the existance of foreign keys.

Noah also suggested something more or less in the along the same lines.
My answer is that I don't want to do it currently; maybe we can improve
on what indexes we use later, but since this can be seen as a modularity
violation, I prefer to keep it as simple as possible.

> * mode == LockTupleKeyShare, == LockTupleShare, == LockTupleUpdate in
> heap_lock_tuple's BeingUpdated branch look like they should be an if/else if

Hm, will look at that.

> * I don't really like HEAP_UPDATE_KEY_REVOKED as a name, what about
> HEAP_KEYS_UPDATED (akin to HEAP_HOT_UPDATED)

Thanks.

> * how can we get infomask2 & HEAP_UPDATE_KEY_REVOKED && infomask &
> HEAP_XMAX_LOCK_ONLY?

SELECT FOR KEY UPDATE? This didn't exist initially, but previous
reviewers (Noah) felt that it made sense to have it for consistency. It
makes the lock conflict table make more sense, anyway.

> * heap_lock_tuple with mode == LockTupleKeyShare && nowait looks like it would
> wait anyway in heap_lock_updated_tuple_rec

Oops.

> * rename HeapSatisfiesHOTUpdate, adjust comments?

Yeah.

> * I wonder whether HeapSatisfiesHOTUpdate could be made to compute the result
> for key_attrs and hot_attrs at the same time, often enough they will do the
> same thing on the same column

Hmm, I will look at that.

> * you didn't start it but I find that all those l$i jump labels make the code
> harder to follow

You mean you suggest to have them renamed? That can be arranged.

> * shouldn't XmaxGetUpdateXid be rather called MultiXactIdGetUpdateXid or such?

Yes.

> /*
> * If the existing lock mode is identical to or weaker than the new
> * one, we can act as though there is no existing lock and have the
> * upper block handle this.
> */
> "block"?

Yeah, as in "the if {} block above". Since this is not clear maybe it
needs rewording.

> * I am not yet sure whether the (xmax == add_to_xmax) optimization in
> compute_new_xmax_infomask is actually safe

Will think about it and add a/some comment(s).

> * ConditionalMultiXactIdWait and MultiXactIdWait should probably rely on a
> common implementation

Will look at that.

> * I am not that convinced that moving the waiting functions away from
> multixact.c is a good idea, but I guess the required knowledge about lockmodes
> makes it hard not to do so

Yeah. The line between multixact.c and heapam.c is a zigzag currently,
I think. Maybe it needs to be more clear; I think the multixact modes
really belong into heapam.c, and they are just uint32 to multixact.c. I
wasn't brave enough to attempt this; maybe I should.

> * Haven't looked yet, but I have a slightly uneasy feeling about Hot Standby
> interactions. Did you look at that?
>
> * Hm?
> HeapTupleSatisfiesVacuum
> #if 0
> ResetMultiHintBit(tuple, buffer, xmax, true);
> #endif

Yeah. I had a couple of ResetMultiHintBit calls sprinkled in some of
the tqual routines, with the idea that if they saw multis that were no
longer live they could be removed, and replaced by just the remaining
updater. However, this doesn't really work because it's not safe to
change the Xmax when not holding exclusive lock, and the tqual routines
don't know that. So we're stuck with keeping the multi in there, even
when we know they are no longer interesting. This is a bit sad, because
it would be a performance benefit to remove them; but it's not strictly
necessary for correctness, so we can leave the optimization for a later
phase. I let those #ifdefed out calls in place so that it's easy to see
where the reset needs to happen.

> This obviously is not a real review, but just learning what the problem & patch
> actually try to do. This is quite a bit to take in ;). I will let it sink in
> ant try to have a look at the architecture and performance next.

Well, the patch is large and complex so any review is obviously going to
take a long time.

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