Re: page macros cleanup (ver 04)

Zdenek Kotala wrote:
> Pavan Deolasee napsal(a):
>> On Fri, Jul 4, 2008 at 4:25 PM, Heikki Linnakangas
>> <heikki(at)enterprisedb(dot)com> wrote:
>>>
>>> No, there's a itemsz = MAXALIGN(itemsz) call before the check against
>>> HashMaxItemSize.
>>>
>>
>> Ah, right. Still should we just not MAXALIGN_DOWN the Max size to
>> reflect the practical limit on the itemsz ? It's more academical
>> though, so not a big deal.
>
> Finally I use following formula:
>
> #define HashMaxItemSize(page) \
> MAXALIGN_DOWN(PageGetPageSize(page) - \
> ( SizeOfPageHeaderData + sizeof(ItemIdData) ) - \
> MAXALIGN(sizeof(HashPageOpaqueData)) )
>
>
> I did not replace PageGetPageSize(page), because other *MaxItemSize has
> same interface.

Ok, fair enough.

There's another academical discrepancy between these two hunks:

> *** src/backend/access/hash/hashpage.c 12 May 2008 00:00:44 -0000 1.75
> --- src/backend/access/hash/hashpage.c 9 Jul 2008 11:30:09 -0000
> ***************
> *** 407,413 ****
> for (i = _hash_log2(metap->hashm_bsize); i > 0; --i)
> {
> if ((1 << i) <= (metap->hashm_bsize -
> ! (MAXALIGN(sizeof(PageHeaderData)) +
> MAXALIGN(sizeof(HashPageOpaqueData)))))
> break;
> }
> --- 407,413 ----
> for (i = _hash_log2(metap->hashm_bsize); i > 0; --i)
> {
> if ((1 << i) <= (metap->hashm_bsize -
> ! (MAXALIGN(SizeOfPageHeaderData) +
> MAXALIGN(sizeof(HashPageOpaqueData)))))
> break;
> }

and

> *** src/include/access/hash.h 19 Jun 2008 00:46:05 -0000 1.88
> --- src/include/access/hash.h 9 Jul 2008 11:30:10 -0000
> ***************
> *** 192,198 ****
> #define BMPG_SHIFT(metap) ((metap)->hashm_bmshift)
> #define BMPG_MASK(metap) (BMPGSZ_BIT(metap) - 1)
> #define HashPageGetBitmap(pg) \
> ! ((uint32 *) (((char *) (pg)) + MAXALIGN(sizeof(PageHeaderData))))
>
> /*
> * The number of bits in an ovflpage bitmap word.
> --- 191,197 ----
> #define BMPG_SHIFT(metap) ((metap)->hashm_bmshift)
> #define BMPG_MASK(metap) (BMPGSZ_BIT(metap) - 1)
> #define HashPageGetBitmap(pg) \
> ! ((uint32 *) (((char *) (pg)) + MAXALIGN(SizeOfPageHeaderData + sizeof(ItemIdData))))
>
> /*
> * The number of bits in an ovflpage bitmap word.

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.

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

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com