Re: pageinspect: Hash index support

>
> + ptr = (char *) itup + IndexInfoFindDataOffset(itup->t_info);
> + Assert(ptr <= uargs->page + BLCKSZ);
>
> I think this should be promoted to an ereport(); these functions can
> accept an arbitrary bytea.

I think we had added 'ptr' variable initially just to dump hash code
in hexadecimal format but now since we have removed that logic from
current code, I think it is no more required and I have therefore
removed it from the current patch. Below is the code that used it
initially. It got changed as per your comment - [1]

>
> + if (opaque->hasho_flag & LH_BUCKET_PAGE)
> + stat->hasho_prevblkno = InvalidBlockNumber;
> + else
> + stat->hasho_prevblkno = opaque->hasho_prevblkno;
>
> I think we should return the raw value here. Mithun's patch to cache
> the metapage hasn't gone in yet, but even if it does, let's assume
> anyone using contrib/pageinspect wants to see the data that's
> physically present, not our gloss on it.

okay, agreed. I have corrected this in the attached v10 patch.

>
> Other than that, I don't think I have any other comments on this. The
> tests that got added look a little verbose to me (do we really need to
> check pages 1-4 separately in each case when they're all hash pages?
> if hash_bitmap_info rejects one, surely it will reject the others) but
> I'm not going to fight too hard if Peter wants it that way.
>

I think it's OK to check hash_bitmap_info() or any other functions
with different page types at least once.

[1]- https://www.postgresql.org/message-id/CA%2BTgmoZUjrVy52TUU3b_Q5L6jcrt7w5R4qFwMXdeCuKQBmYWqQ%40mail.gmail.com

--
With Regards,
Ashutosh Sharma
EnterpriseDB:http://www.enterprisedb.com