From: Andres Freund <andres@2ndquadrant.com>
Organization: 2ndquadrant.com
To: pgsql-hackers@postgresql.org
Subject: Re: foreign key locks
Date: Thu, 11 Oct 2012 17:36:01 +0200
User-Agent: KMail/1.13.7 (Linux/3.6.0-rc7-andres-00118-g66266e1-dirty;
	KDE/4.8.4; x86_64; ; )
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>
References: <4FEAB90A0200002500048B7D@gw.wicourts.gov>
	<1346095617-sup-567@alvh.no-ip.org>
	<1346389151-sup-8379@alvh.no-ip.org>
In-Reply-To: <1346389151-sup-8379@alvh.no-ip.org>
MIME-Version: 1.0
Content-Type: Text/Plain;
  charset="utf-8"
Content-Transfer-Encoding: quoted-printable
Message-Id: <201210111736.01406.andres@2ndquadrant.com>

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=
=20
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=20
the - very nice - pgcon paper.

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

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

* mode =3D=3D LockTupleKeyShare, =3D=3D LockTupleShare, =3D=3D LockTupleUpd=
ate in=20
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=20
HEAP_KEYS_UPDATED (akin to HEAP_HOT_UPDATED)

* how can we get infomask2 & HEAP_UPDATE_KEY_REVOKED && infomask &=20
HEAP_XMAX_LOCK_ONLY?

* heap_lock_tuple with mode =3D=3D LockTupleKeyShare && nowait looks like i=
t would=20
wait anyway in heap_lock_updated_tuple_rec

* rename HeapSatisfiesHOTUpdate, adjust comments?

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

* you didn't start it but I find that all those l$i jump labels make the co=
de=20
harder to follow

* shouldn't XmaxGetUpdateXid be rather called MultiXactIdGetUpdateXid or su=
ch?=20

*
		/*
		 * 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 =3D=3D add_to_xmax) optimization in=20
compute_new_xmax_infomask is actually safe

* ConditionalMultiXactIdWait and MultiXactIdWait should probably rely on a=
=20
common implementation

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

* Haven't looked yet, but I have a slightly uneasy feeling about Hot Standb=
y=20
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 & p=
atch=20
actually try to do. This is quite a bit to take in ;). I will let it sink i=
n=20
ant try to have a look at the architecture and performance next.


Greetings,

Andres

=2EoO(and people call catalog timetravel complicated)

=2D-=20
 Andres Freund	                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services