Date: Wed, 09 Jul 2008 15:43:59 +0200
From: Zdenek Kotala <Zdenek.Kotala@Sun.COM>
Subject: Re: page macros cleanup (ver 04)
In-reply-to: <4874A6E0.9070909@enterprisedb.com>
To: Heikki Linnakangas <heikki@enterprisedb.com>
Cc: Pavan Deolasee <pavan.deolasee@gmail.com>,
	PostgreSQL-patches <pgsql-patches@postgresql.org>
Message-id: <4874C09F.4000302@sun.com>
MIME-version: 1.0
Content-type: multipart/mixed; boundary="Boundary_(ID_97uW378B4ucs5jEEcKYPyA)"
References: <48529B83.9010907@sun.com>
	<2e78013d0807030450m4cab60d0jd99d1acce59fae56@mail.gmail.com>
	<486DD1D1.50604@sun.com>
	<2e78013d0807040150w393ad72codaf06a6ac12a83ce@mail.gmail.com>
	<486DF644.4070604@enterprisedb.com>
	<2e78013d0807040350m3b2913cfkceaa70d8f9d697c2@mail.gmail.com>
	<486E01B0.9050100@enterprisedb.com>
	<2e78013d0807040404q5ff7f7faq9f87a39e0f0552fa@mail.gmail.com>
	<486E138F.4060900@sun.com> <4874A6E0.9070909@enterprisedb.com>
User-Agent: Thunderbird 2.0.0.14 (X11/20080507)

This is a multi-part message in MIME format.

--Boundary_(ID_97uW378B4ucs5jEEcKYPyA)
Content-type: text/plain; format=flowed; charset=UTF-8
Content-transfer-encoding: 7BIT

Heikki Linnakangas napsal(a):
> Zdenek Kotala wrote:
>> Pavan Deolasee napsal(a):

> There's another academical discrepancy between these two hunks:

<snip>

> I admit I don't understand what that bitmap is, it looks like we're 
> assuming it can take up all the space between page header and the 
> special portion, without any line pointers, but in HashPageGetBitmap, 
> we're reserving space for one pointer. It looks like the actual size of 
> the bitmap is only the largest power of 2 below the maximum size, so 
> that won't be an issue in practice. That macro is actually doing the 
> same thing as PageGetContents, so I switched to using that. As that 
> moves the data sligthly on those bitmap pages, I guess we'll need a 
> catversion bump.

Good catch. if we have to bump catalog version then I have there small patch
which introduce following macro and remove PageHeaderData stucture from 
HashMetaPageData:

#define HashPageGetMeta(page) ((HashMetaPage) (PageGetContents(page)))

It also needs bump a catalog version and if we do it now I think it is better to
connect both these patches and do bump only once.

> Attached is an updated patch. I also fixed some whitespace and comments 
> that were no longer valid. How does it look to you now?

Perfect. Thanks

		Zdenek


--Boundary_(ID_97uW378B4ucs5jEEcKYPyA)
Content-type: text/x-patch; name=hash.patch
Content-transfer-encoding: 8BIT
Content-disposition: inline; filename=hash.patch

diff -rc pgsql_hash.851ed84fb6aa/src/backend/access/hash/hash.c pgsql_hash/src/backend/access/hash/hash.c
*** pgsql_hash.851ed84fb6aa/src/backend/access/hash/hash.c	út črc  8 21:31:38 2008
--- pgsql_hash/src/backend/access/hash/hash.c	út črc  8 21:31:38 2008
***************
*** 527,533 ****
  	 * each bucket.
  	 */
  	metabuf = _hash_getbuf(rel, HASH_METAPAGE, HASH_READ, LH_META_PAGE);
! 	metap = (HashMetaPage) BufferGetPage(metabuf);
  	orig_maxbucket = metap->hashm_maxbucket;
  	orig_ntuples = metap->hashm_ntuples;
  	memcpy(&local_metapage, metap, sizeof(local_metapage));
--- 527,533 ----
  	 * each bucket.
  	 */
  	metabuf = _hash_getbuf(rel, HASH_METAPAGE, HASH_READ, LH_META_PAGE);
! 	metap =  HashPageGetMeta(BufferGetPage(metabuf));
  	orig_maxbucket = metap->hashm_maxbucket;
  	orig_ntuples = metap->hashm_ntuples;
  	memcpy(&local_metapage, metap, sizeof(local_metapage));
***************
*** 629,635 ****
  
  	/* Write-lock metapage and check for split since we started */
  	metabuf = _hash_getbuf(rel, HASH_METAPAGE, HASH_WRITE, LH_META_PAGE);
! 	metap = (HashMetaPage) BufferGetPage(metabuf);
  
  	if (cur_maxbucket != metap->hashm_maxbucket)
  	{
--- 629,635 ----
  
  	/* Write-lock metapage and check for split since we started */
  	metabuf = _hash_getbuf(rel, HASH_METAPAGE, HASH_WRITE, LH_META_PAGE);
! 	metap = HashPageGetMeta(BufferGetPage(metabuf));
  
  	if (cur_maxbucket != metap->hashm_maxbucket)
  	{
diff -rc pgsql_hash.851ed84fb6aa/src/backend/access/hash/hashinsert.c pgsql_hash/src/backend/access/hash/hashinsert.c
*** pgsql_hash.851ed84fb6aa/src/backend/access/hash/hashinsert.c	út črc  8 21:31:38 2008
--- pgsql_hash/src/backend/access/hash/hashinsert.c	út črc  8 21:31:38 2008
***************
*** 69,75 ****
  
  	/* Read the metapage */
  	metabuf = _hash_getbuf(rel, HASH_METAPAGE, HASH_READ, LH_META_PAGE);
! 	metap = (HashMetaPage) BufferGetPage(metabuf);
  
  	/*
  	 * Check whether the item can fit on a hash page at all. (Eventually, we
--- 69,75 ----
  
  	/* Read the metapage */
  	metabuf = _hash_getbuf(rel, HASH_METAPAGE, HASH_READ, LH_META_PAGE);
! 	metap = HashPageGetMeta(BufferGetPage(metabuf));
  
  	/*
  	 * Check whether the item can fit on a hash page at all. (Eventually, we
diff -rc pgsql_hash.851ed84fb6aa/src/backend/access/hash/hashovfl.c pgsql_hash/src/backend/access/hash/hashovfl.c
*** pgsql_hash.851ed84fb6aa/src/backend/access/hash/hashovfl.c	út črc  8 21:31:38 2008
--- pgsql_hash/src/backend/access/hash/hashovfl.c	út črc  8 21:31:38 2008
***************
*** 187,193 ****
  	_hash_chgbufaccess(rel, metabuf, HASH_NOLOCK, HASH_WRITE);
  
  	_hash_checkpage(rel, metabuf, LH_META_PAGE);
! 	metap = (HashMetaPage) BufferGetPage(metabuf);
  
  	/* start search at hashm_firstfree */
  	orig_firstfree = metap->hashm_firstfree;
--- 187,193 ----
  	_hash_chgbufaccess(rel, metabuf, HASH_NOLOCK, HASH_WRITE);
  
  	_hash_checkpage(rel, metabuf, LH_META_PAGE);
! 	metap = HashPageGetMeta(BufferGetPage(metabuf));
  
  	/* start search at hashm_firstfree */
  	orig_firstfree = metap->hashm_firstfree;
***************
*** 450,456 ****
  
  	/* Read the metapage so we can determine which bitmap page to use */
  	metabuf = _hash_getbuf(rel, HASH_METAPAGE, HASH_READ, LH_META_PAGE);
! 	metap = (HashMetaPage) BufferGetPage(metabuf);
  
  	/* Identify which bit to set */
  	ovflbitno = blkno_to_bitno(metap, ovflblkno);
--- 450,456 ----
  
  	/* Read the metapage so we can determine which bitmap page to use */
  	metabuf = _hash_getbuf(rel, HASH_METAPAGE, HASH_READ, LH_META_PAGE);
! 	metap = HashPageGetMeta(BufferGetPage(metabuf));
  
  	/* Identify which bit to set */
  	ovflbitno = blkno_to_bitno(metap, ovflblkno);
diff -rc pgsql_hash.851ed84fb6aa/src/backend/access/hash/hashpage.c pgsql_hash/src/backend/access/hash/hashpage.c
*** pgsql_hash.851ed84fb6aa/src/backend/access/hash/hashpage.c	út črc  8 21:31:38 2008
--- pgsql_hash/src/backend/access/hash/hashpage.c	út črc  8 21:31:38 2008
***************
*** 395,401 ****
  	pageopaque->hasho_flag = LH_META_PAGE;
  	pageopaque->hasho_page_id = HASHO_PAGE_ID;
  
! 	metap = (HashMetaPage) pg;
  
  	metap->hashm_magic = HASH_MAGIC;
  	metap->hashm_version = HASH_VERSION;
--- 395,401 ----
  	pageopaque->hasho_flag = LH_META_PAGE;
  	pageopaque->hasho_page_id = HASHO_PAGE_ID;
  
! 	metap = HashPageGetMeta(pg);
  
  	metap->hashm_magic = HASH_MAGIC;
  	metap->hashm_version = HASH_VERSION;
***************
*** 532,538 ****
  	_hash_chgbufaccess(rel, metabuf, HASH_NOLOCK, HASH_WRITE);
  
  	_hash_checkpage(rel, metabuf, LH_META_PAGE);
! 	metap = (HashMetaPage) BufferGetPage(metabuf);
  
  	/*
  	 * Check to see if split is still needed; someone else might have already
--- 532,538 ----
  	_hash_chgbufaccess(rel, metabuf, HASH_NOLOCK, HASH_WRITE);
  
  	_hash_checkpage(rel, metabuf, LH_META_PAGE);
! 	metap = HashPageGetMeta(BufferGetPage(metabuf));
  
  	/*
  	 * Check to see if split is still needed; someone else might have already
diff -rc pgsql_hash.851ed84fb6aa/src/backend/access/hash/hashsearch.c pgsql_hash/src/backend/access/hash/hashsearch.c
*** pgsql_hash.851ed84fb6aa/src/backend/access/hash/hashsearch.c	út črc  8 21:31:38 2008
--- pgsql_hash/src/backend/access/hash/hashsearch.c	út črc  8 21:31:38 2008
***************
*** 186,192 ****
  
  	/* Read the metapage */
  	metabuf = _hash_getbuf(rel, HASH_METAPAGE, HASH_READ, LH_META_PAGE);
! 	metap = (HashMetaPage) BufferGetPage(metabuf);
  
  	/*
  	 * Compute the target bucket number, and convert to block number.
--- 186,192 ----
  
  	/* Read the metapage */
  	metabuf = _hash_getbuf(rel, HASH_METAPAGE, HASH_READ, LH_META_PAGE);
! 	metap = HashPageGetMeta(BufferGetPage(metabuf));
  
  	/*
  	 * Compute the target bucket number, and convert to block number.
diff -rc pgsql_hash.851ed84fb6aa/src/backend/access/hash/hashutil.c pgsql_hash/src/backend/access/hash/hashutil.c
*** pgsql_hash.851ed84fb6aa/src/backend/access/hash/hashutil.c	út črc  8 21:31:38 2008
--- pgsql_hash/src/backend/access/hash/hashutil.c	út črc  8 21:31:38 2008
***************
*** 191,197 ****
  	 */
  	if (flags == LH_META_PAGE)
  	{
! 		HashMetaPage metap = (HashMetaPage) page;
  
  		if (metap->hashm_magic != HASH_MAGIC)
  			ereport(ERROR,
--- 191,197 ----
  	 */
  	if (flags == LH_META_PAGE)
  	{
! 		HashMetaPage metap = HashPageGetMeta(page);
  
  		if (metap->hashm_magic != HASH_MAGIC)
  			ereport(ERROR,
diff -rc pgsql_hash.851ed84fb6aa/src/include/access/hash.h pgsql_hash/src/include/access/hash.h
*** pgsql_hash.851ed84fb6aa/src/include/access/hash.h	út črc  8 21:31:38 2008
--- pgsql_hash/src/include/access/hash.h	út črc  8 21:31:38 2008
***************
*** 138,144 ****
  
  typedef struct HashMetaPageData
  {
- 	PageHeaderData hashm_phdr;	/* pad for page header (do not use) */
  	uint32		hashm_magic;	/* magic no. for hash tables */
  	uint32		hashm_version;	/* version ID */
  	double		hashm_ntuples;	/* number of tuples stored in the table */
--- 138,143 ----
***************
*** 162,167 ****
--- 161,168 ----
  
  typedef HashMetaPageData *HashMetaPage;
  
+ #define HashPageGetMeta(page) ((HashMetaPage) (PageGetContents(page)))
+ 
  /*
   * Maximum size of a hash index item (it's okay to have only one per page)
   */

--Boundary_(ID_97uW378B4ucs5jEEcKYPyA)--