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 * README.tuplock jumps in quite fast without any introduction * the reasons for the redesign aren't really included in the patch but just in the - very nice - pgcon paper. * "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 * 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. * mode == LockTupleKeyShare, == LockTupleShare, == LockTupleUpdate in heap_lock_tuple's BeingUpdated branch look like they should be an if/else if * I don't really like HEAP_UPDATE_KEY_REVOKED as a name, what about HEAP_KEYS_UPDATED (akin to HEAP_HOT_UPDATED) * how can we get infomask2 & HEAP_UPDATE_KEY_REVOKED && infomask & HEAP_XMAX_LOCK_ONLY? * heap_lock_tuple with mode == LockTupleKeyShare && nowait looks like it would wait anyway in heap_lock_updated_tuple_rec * rename HeapSatisfiesHOTUpdate, adjust comments? * 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 * you didn't start it but I find that all those l$i jump labels make the code harder to follow * shouldn't XmaxGetUpdateXid be rather called MultiXactIdGetUpdateXid or such? * /* * 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"? * I am not yet sure whether the (xmax == add_to_xmax) optimization in compute_new_xmax_infomask is actually safe * ConditionalMultiXactIdWait and MultiXactIdWait should probably rely on a common implementation * 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 * 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 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. Greetings, Andres .oO(and people call catalog timetravel complicated) -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (email@example.com) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers