Re: pageinspect: Hash index support

Hi,

Please find my reply inline.

> In hash_bimap_info(), we go to the trouble of creating a raw page but
> never do anything with it. I guess the idea here is just to error out
> if the supplied page number is not an overflow page, but there are no
> comments explaining that. Anyway, I'm not sure it's a very good idea,
> because it means that if you try to write a query to interrogate the
> status of all the bitmap pages, it's going to read all of the overflow
> pages to which they point, which makes the operation a LOT more
> expensive. I think it would be better to leave this part out; if the
> user wants to know which pages are actually overflow pages, they can
> use hash_page_type() to figure it out.

Yes, the intention was to ensure that user only passes overflow page
as an input to this function. I think if we wan't to avoid creating a
raw page then we may need to find some other way to verify if it is an
overflow page or not, may be we can make use of hash_check_page().

Let's make it the job of this
> function just to check the available/free status of the page in the
> bitmap, without reading the bitmap itself.
>

okay, In that case I think we can check the previous block number that
the overflow page is pointing to in order to identify if it is free or
in-use. AFAIU, if an overflow page is free it's prev block number will
be Invalid. This way, we may not have to read bitmap page. Now the
question here is, we also have bucket pages where previous block
number is always Invalid but before checking this we ensure that we
are only dealing with an overflow page.Please let me know if you feel
we do have some better option than this to identify the status of
overflow page without reading bitmap.

> In doing that, I think it should either return (bitmapblkno,
> bitmapbit, bit) or just bit. Returning bitmapblkno but not bitmapbit
> seems strange. Also, I think it should return bit as a bool, not
> int4.

It would be good to return bitmap bit number as well along with the
bitmap block number. Also, returning bit as bool would look good. I
will do that.

I am also working on other review comments and will share the updated
patch asap. Thanks.