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


With Regards,
Ashutosh Sharma

Attachment: 0001-Add-support-for-hash-index-in-pageinspect-contrib-mo_v10.patch
Description: invalid/octet-stream

Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to