Re: pageinspect: Hash index support

On Sat, Jan 28, 2017 at 10:25 PM, Ashutosh Sharma <ashu(dot)coek88(at)gmail(dot)com> wrote:
> 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.
>

I think this was a very poor finding by me. If an overflow page is
freed, it is completely filled with zero values rather than marking
it's prev and next block number as invalid. Therefore, we won't be
able to read a free overflow page as it is a new page and hence, we
can't decide if an overflow page is free or not without reading the
corresponding bitmap page.

>> 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.