Received: from magus.postgresql.org (magus.postgresql.org [87.238.57.229])
	by mail.postgresql.org (Postfix) with ESMTP id 4B547EA3550
	for <pgsql-hackers@postgresql.org>;
	Fri, 13 Jul 2012 06:34:09 -0300 (ADT)
Received: from shaykin.bluegap.ch ([78.46.81.73])
	by magus.postgresql.org with esmtp (Exim 4.72)
	(envelope-from <markus@bluegap.ch>) id 1SpcGD-0002GW-LK
	for pgsql-hackers@postgresql.org; Fri, 13 Jul 2012 09:34:09 +0000
Received: from [10.80.16.200] (adsl-84-227-247-21.adslplus.ch
	[::ffff:84.227.247.21])
	(AUTH: LOGIN markus@bluegap.ch, TLS: TLSv1/SSLv3,256bits,AES256-SHA)
	by shaykin.bluegap.ch with esmtp; Fri, 13 Jul 2012 11:45:04 +0200
	id 00000000015880E6.000000004FFFEE21.00004A4A
Message-ID: <4FFFEB7F.9090702@bluegap.ch>
Date: Fri, 13 Jul 2012 11:33:51 +0200
From: Markus Wanner <markus@bluegap.ch>
User-Agent: Mozilla/5.0 (X11; Linux x86_64;
	rv:10.0.4) Gecko/20120510 Icedove/10.0.4
Mime-Version: 1.0
Content-Type: multipart/mixed;
	boundary="=_shaykin.bluegap.ch-19018-1342172705-0001-2"
To: Tom Lane <tgl@sss.pgh.pa.us>
CC: Teodor Sigaev <teodor@sigaev.ru>, Oleg Bartunov <oleg@sai.msu.su>,
	pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: BlockNumber initialized to InvalidBuffer?
References: <4FFC292C.20703@bluegap.ch> <13604.1341978315@sss.pgh.pa.us>
In-Reply-To: <13604.1341978315@sss.pgh.pa.us>
X-Enigmail-Version: 1.4.1
X-Pg-Spam-Score: -1.9 (-)
X-Archive-Number: 201207/597
X-Sequence-Number: 211280

This is a MIME-formatted message.  If you see this text it means that your
E-mail software does not support MIME-formatted messages.

--=_shaykin.bluegap.ch-19018-1342172705-0001-2
Content-Type: text/plain; charset=iso-8859-1
Content-Transfer-Encoding: 7bit

On 07/11/2012 05:45 AM, Tom Lane wrote:
> I'm also inclined to think that the "while (stack)" coding of the rest
> of it is wrong, misleading, or both, on precisely the same grounds: if
> that loop ever did fall out at the test, the function would have failed
> to honor its contract.  The only correct exit points are the "return"s
> in the middle.

I came to the same conclusion, yes. Looks like the additional asserts in
the attached patch all hold true.

As another minor improvement, it doesn't seem necessary to repeatedly
set the rootBlkno.

Regards

Markus Wanner

--=_shaykin.bluegap.ch-19018-1342172705-0001-2
Content-Type: text/x-patch; name="pg-gin-strange-initialization2.diff";
	charset=iso-8859-1
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename="pg-gin-strange-initialization2.diff"

#
# old_revision [d90761edf47c6543d4686a76baa0b4e2a7ed113b]
#
# patch "src/backend/access/gin/ginbtree.c"
#  from [2d3e63387737b4034fc25ca3cb128d9ac57f4f01]
#    to [62efe71b8e3429fc1bf2d2742f8d2fa69f2f4de6]
#
============================================================
*** src/backend/access/gin/ginbtree.c	2d3e63387737b4034fc25ca3cb128d9ac57f4f01
--- src/backend/access/gin/ginbtree.c	62efe71b8e3429fc1bf2d2742f8d2fa69f2f4de6
*************** ginInsertValue(GinBtree btree, GinBtreeS
*** 276,294 ****
  ginInsertValue(GinBtree btree, GinBtreeStack *stack, GinStatsData *buildStats)
  {
  	GinBtreeStack *parent = stack;
! 	BlockNumber rootBlkno = InvalidBuffer;
  	Page		page,
  				rpage,
  				lpage;
  
  	/* remember root BlockNumber */
! 	while (parent)
  	{
  		rootBlkno = parent->blkno;
  		parent = parent->parent;
! 	}
  
! 	while (stack)
  	{
  		XLogRecData *rdata;
  		BlockNumber savedRightLink;
--- 276,298 ----
  ginInsertValue(GinBtree btree, GinBtreeStack *stack, GinStatsData *buildStats)
  {
  	GinBtreeStack *parent = stack;
! 	BlockNumber rootBlkno;
  	Page		page,
  				rpage,
  				lpage;
  
  	/* remember root BlockNumber */
! 	Assert(stack != NULL);
! 	parent = stack;
! 	do
  	{
  		rootBlkno = parent->blkno;
  		parent = parent->parent;
! 	} while (parent);
  
! 	Assert(BlockNumberIsValid(rootBlkno));
! 
! 	for (;;)
  	{
  		XLogRecData *rdata;
  		BlockNumber savedRightLink;
*************** ginInsertValue(GinBtree btree, GinBtreeS
*** 469,474 ****
--- 473,479 ----
  
  		UnlockReleaseBuffer(stack->buffer);
  		pfree(stack);
+ 		Assert(parent != NULL);		/* parent == NULL case is handled above */
  		stack = parent;
  	}
  }

--=_shaykin.bluegap.ch-19018-1342172705-0001-2--