Date: Fri, 19 Oct 2012 11:40:29 -0400
From: Noah Misch <noah@leadboat.com>
To: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: Pg Hackers <pgsql-hackers@postgresql.org>
Subject: Re: foreign key locks
Message-ID: <20121019154029.GA27306@tornado.leadboat.com>
References: <4FEAB90A0200002500048B7D@gw.wicourts.gov>
	<1341011531-sup-1642@alvh.no-ip.org>
	<1341528201-sup-720@alvh.no-ip.org>
	<1345234806-sup-3685@alvh.no-ip.org>
	<1345670473-sup-4222@alvh.no-ip.org>
	<1346095617-sup-567@alvh.no-ip.org>
	<1346389151-sup-8379@alvh.no-ip.org>
	<20121018195820.GO3763@alvh.no-ip.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20121018195820.GO3763@alvh.no-ip.org>
User-Agent: Mutt/1.5.18 (2008-05-17)

On Thu, Oct 18, 2012 at 04:58:20PM -0300, Alvaro Herrera wrote:
> Here is version 22 of this patch.  This version contains fixes to issues
> reported by Andres, as well as a rebase to latest master.

I scanned this for obvious signs of work left to do.  It contains numerous XXX
and FIXME comments.  Many highlight micro-optimization opportunities and the
like; those can stay.  Others preclude commit, either highlighting an unsolved
problem or wrongly highlighting a non-problem:

> + 	/*
> + 	 * XXX we do not lock this tuple here; the theory is that it's sufficient
> + 	 * with the buffer lock we're about to grab.  Any other code must be able
> + 	 * to cope with tuple lock specifics changing while they don't hold buffer
> + 	 * lock anyway.
> + 	 */

>    * so they will be uninteresting by the time our next transaction starts.
>    * (XXX not clear that this is correct --- other members of the MultiXact
>    * could hang around longer than we did.  However, it's not clear what a
> !  * better policy for flushing old cache entries would be.)  FIXME actually
> !  * this is plain wrong now that multixact's may contain update Xids.

> ! 	nmembers = GetMultiXactIdMembers(multi, &members, true);
> ! 	/*
> ! 	 * XXX we don't have the infomask to run the required consistency check
> ! 	 * here; the required notational overhead seems excessive.
> ! 	 */

>   		/* We assume the cache entries are sorted */
> ! 		/* XXX we assume the unused bits in "status" are zeroed */
> ! 		if (memcmp(members, entry->members, nmembers * sizeof(MultiXactMember)) == 0)

> !  * XXX do we have any issues with needing to checkpoint here?
>    */
> ! static void
> ! TruncateMultiXact(void)
>   {

> + 	/* FIXME what should we initialize this to? */
> + 	newFrozenMulti = ReadNextMultiXactId();

> + 	 * FIXME -- the XMAX_IS_MULTI test is a bit wrong .. it's possible to
> + 	 * have tuples with that bit set that are dead.  However, if that's
> + 	 * changed, the RawXmax() call below should probably be researched as well.
>   	 */
>   	if (tuple->t_infomask &
> ! 		(HEAP_XMAX_INVALID | HEAP_XMAX_LOCK_ONLY | HEAP_XMAX_IS_MULTI))
>   		return false;