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 (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
