Re: pageinspect: Hash index support

On Wed, Feb 8, 2017 at 9:25 AM, Ashutosh Sharma <ashu(dot)coek88(at)gmail(dot)com> wrote:
>>> 1) Check if an overflow page is a new page. If so, read a bitmap page
>>> to confirm if a bit corresponding to this overflow page is clear or
>>> not. For this, I would first add Assert statement to ensure that the
>>> bit is clear and if it is, then set the statusbit as false indicating
>>> that the page is free.
>>>
>>> 2) In case if an overflow page is not new, first verify if it is
>>> really an overflow page and if so, check if the bit corresponding to
>>> it in the bitmap page is SET. If so, set the statusbit as true; If
>>> not, we would see an assertion failure happening.
>>
>> I think this is complicated and not what anybody wants. I think you
>> should do exactly what I said above: return true if the bit is set in
>> the bitmap, and false if it isn't. Full stop. Don't read or do
>> anything with the underlying page. Only read the bitmap page.
>
> Okay, As per the inputs from you, I have modified hash_bitmap_info()
> and have tried to keep the things simple. Attached is the patch that
> has this changes. Please have a look and let me know if you feel it is
> not yet in the right shape. Thanks.

Well, this changes the function signature and I don't see any
advantage in doing that. Also, it still doesn't read the code that
reads the overflow page. Any correct patch for this problem needs to
include the following hunk:

- buf = ReadBufferExtended(indexRel, MAIN_FORKNUM, (BlockNumber) ovflblkno,
- RBM_NORMAL, NULL);
- LockBuffer(buf, BUFFER_LOCK_SHARE);
- _hash_checkpage(indexRel, buf, LH_PAGE_TYPE);
- page = BufferGetPage(buf);
- opaque = (HashPageOpaque) PageGetSpecialPointer(page);
-
- if (opaque->hasho_flag != LH_OVERFLOW_PAGE)
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("page is not an overflow page"),
- errdetail("Expected %08x, got %08x.",
- LH_OVERFLOW_PAGE, opaque->hasho_flag)));
-
- if (BlockNumberIsValid(opaque->hasho_prevblkno))
- bit = true;
-
- UnlockReleaseBuffer(buf);

And then, instead, you need to add some code to set bit based on the
bitmap page, like what you have:

+ mapbuf = _hash_getbuf(indexRel, bitmapblkno, HASH_READ, LH_BITMAP_PAGE);
+ mappage = BufferGetPage(mapbuf);
+ freep = HashPageGetBitmap(mappage);
+
+ if (ISSET(freep, bitmapbit))
+ bit = 1;

Except I would write that last line as...

bit = ISSET(freep, bitmapbit) != 0

...instead of using an if statement.

I'm sort of confused as to why the idea of not reading the underlying
page is so hard to understand.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company