Re: pageinspect: Hash index support

On Fri, Sep 23, 2016 at 9:40 AM, Peter Eisentraut
<peter(dot)eisentraut(at)2ndquadrant(dot)com> wrote:
> On 9/21/16 9:30 AM, Jesper Pedersen wrote:
>> Attached is v5, which add basic page verification.
>
> There are still some things that I found that appear to follow the old
> style (btree) conventions instead the new style (brin, gin) conventions.
>
> - Rename hash_metap to hash_metapage_info.
>
> - Don't use BuildTupleFromCStrings(). (There is nothing inherently
> wrong with it, but why convert numbers to strings and back again when it
> can be done more directly.)
>
> - Documentation for hash_page_stats still has blkno argument.
>
> - In hash_metap, the magic field could be type bytea, so that it
> displays in hex. Or text like the brin functions.
>
> Some other comments:
>
> - hash_metap result fields spares and mapp should be arrays of integer.
>

how would you like to see both those arrays in tuple, right now, I
think Jesper's code is showing it as string.

> (Incidentally, the comment in hash.h talks about bitmaps[] but I think
> it means hashm_mapp[].)
>

which comment are you referring here? hashm_mapp contains block
numbers of bitmap pages.

While looking at patch, I noticed below code which seems somewhat problematic:

+ stat->max_avail = BLCKSZ - (BLCKSZ - phdr->pd_special + SizeOfPageHeaderData);
+
+ /* page type (flags) */
+ if (opaque->hasho_flag & LH_META_PAGE)
+ stat->type = 'm';
+ else if (opaque->hasho_flag & LH_OVERFLOW_PAGE)
+ stat->type = 'v';
+ else if (opaque->hasho_flag & LH_BUCKET_PAGE)
+ stat->type = 'b';
+ else if (opaque->hasho_flag & LH_BITMAP_PAGE)
+ stat->type = 'i';

In the above code, it appears that you are trying to calculate
max_avail space for all pages in same way. Don't you need to
calculate it differently for bitmap page or meta page as they don't
share the same format as other type of pages?

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com