Re: [HACKERS] pageinspect: Hash index support

On Wed, Feb 8, 2017 at 9:25 AM, Ashutosh Sharma <ashu.coek88@gmail.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