From: Andres Freund <andres@2ndquadrant.com>
To: pgsql-hackers@postgresql.org
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>
Subject: Re: foreign key locks
Date: Thu, 11 Oct 2012 17:36:01 +0200
Message-ID: <201210111736.01406.andres@2ndquadrant.com> (raw)
In-Reply-To: <1346389151-sup-8379@alvh.no-ip.org>
References: <4FEAB90A0200002500048B7D@gw.wicourts.gov>
<1346095617-sup-567@alvh.no-ip.org>
<1346389151-sup-8379@alvh.no-ip.org>
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: andres@2ndquadrant.com, alvherre@2ndquadrant.com
Subject: Re: foreign key locks
In-Reply-To: <201210111736.01406.andres@2ndquadrant.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox