Date: Tue, 20 Nov 2012 22:15:33 +0100
From: Andres Freund <andres@2ndquadrant.com>
To: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: pgsql-hackers@postgresql.org
Subject: Re: foreign key locks
Message-ID: <20121120211533.GA4171@awork2.anarazel.de>
References: <4FEAB90A0200002500048B7D@gw.wicourts.gov>
	<1346389151-sup-8379@alvh.no-ip.org>
	<20121018195820.GO3763@alvh.no-ip.org>
	<201210270006.54728.andres@2ndquadrant.com>
	<20121114152347.GA5161@alvh.no-ip.org>
	<20121114162726.GE5161@alvh.no-ip.org>
	<20121119115804.GA28067@awork2.anarazel.de>
	<20121120203614.GA26623@alvh.no-ip.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20121120203614.GA26623@alvh.no-ip.org>

On 2012-11-20 17:36:14 -0300, Alvaro Herrera wrote:
> Andres Freund wrote:
>
>
> > - I find using a default: clause in switches with enum types where everything
> >   is expected to be handled like the following a bad idea, this way the
> >   compiler won't warn you if youve missed case's which makes changing/extending code harder:
> > 		switch (rc->strength)
> > 		{
>
> I eliminated some of these default clauses, but the compiler is not
> happy about not having some of them, so they remain.

You have removed the ones that looked removable to me...

>
> > -
> > #if 0
> > 			/*
> > 			 * The idea here is to remove the IS_MULTI bit, and replace the
> > 			 * xmax with the updater's Xid.  However, we can't really do it:
> > 			 * modifying the Xmax is not allowed under our buffer locking
> > 			 * rules, unless we have an exclusive lock; but we don't know that
> > 			 * we have it.  So the multi needs to remain in place :-(
> > 			 */
> > 			ResetMultiHintBit(tuple, buffer, xmax, true);
> > #endif
> >
> >  Three things:
> >     - HeapTupleSatisfiesUpdate is actually always called exclusively locked ;)
> >     - Extending something like LWLockHeldByMe to also return the current
> >       lockmode doesn't sound hard
> >     - we seem to be using #ifdef NOT_YET for such cases
>
> After spending some time trying to make this work usefully, I observed
> that it's pointless, at least if we apply it only in
> HeapTupleSatisfiesUpdate, because the IS_MULTI bit is going to be
> removed by compute_new_xmax_infomask if the multi is gone.  Something
> like this would be useful if we could do it in other places; say when
> we're merely scanning page without the specific intention of modifying
> that particular tuple.  Maybe in heap_prune_page, for example.  ISTM we
> can see that as an optimization we can add later.

heap_prune_page sounds like a good fit, yes. One other place would be
when following hot chains, but the locking would be far more critical in
that path, so its less likely to be beneficial.
Pushing it off sounds good to me.

> Some of your other observations have been fixed by commits that I have
> pushed to my github tree.

A short repetition of the previous pgbench run of many SELECT FOR
SHARE's:

10s test:
master:  22673 24637 23874 25527
fklocks: 24835 24601 24606 24868

60s test:
master:  32609 33087
fklocks: 33350 33359

Very nice!

Greetings,

Andres Freund

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