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 

* how can we get infomask2 & HEAP_UPDATE_KEY_REVOKED && infomask & 

* 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 
                 * one, we can act as though there is no existing lock and have 
                 * upper block handle this.

* 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?
#if 0
                                ResetMultiHintBit(tuple, buffer, xmax, true);

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.



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

Reply via email to