From: | Jesper Pedersen <jesper(dot)pedersen(at)redhat(dot)com> |
---|---|
To: | Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com> |
Cc: | Michael Paquier <michael(dot)paquier(at)gmail(dot)com>, Ashutosh Sharma <ashu(dot)coek88(at)gmail(dot)com>, Jeff Janes <jeff(dot)janes(at)gmail(dot)com>, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: pageinspect: Hash index support |
Date: | 2016-09-26 17:39:48 |
Message-ID: | 16d89e7e-cb1a-cb5d-e944-6f10c324a312@redhat.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Hi,
On 09/23/2016 12:10 AM, Peter Eisentraut 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.
>
Done.
> - 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.)
>
Left as is, since BuildTupleFromCStrings() vs. xyzGetDatum() are equally
readable in this case. But, I can change the patch if needed.
> - Documentation for hash_page_stats still has blkno argument.
>
Fixed.
> - In hash_metap, the magic field could be type bytea, so that it
> displays in hex. Or text like the brin functions.
>
Changed to 'text'.
> Some other comments:
>
> - hash_metap result fields spares and mapp should be arrays of integer.
>
B-tree and BRIN uses a 'text' field as output, so left as is.
> (Incidentally, the comment in hash.h talks about bitmaps[] but I think
> it means hashm_mapp[].)
>
> - As of very recently, we don't need to move pageinspect--1.5.sql to
> pageinspect--1.6.sql anymore. Just add pageinspect--1.5--1.6.sql.
>
We need to rename the pageinspect--1.5.sql file and add the new methods,
right ? Or ?
> - In the documentation for hash_page_stats(), the "+" sign is misaligned.
>
Fixed.
> - In hash_page_items, the fields itemlen, nulls, vars are always 16,
> false, false. So perhaps there is no need for them. Similarly, the
> hash_page_stats in hash_page_stats is always 16.
>
Fixed, by removing them. We can add them back later if needed.
> - The data field could be of type bytea.
>
Left as is, for same reasons as 'spares' and 'mapp'.
> - Add a pointer in the documentation to the relevant header file.
>
Done.
> Bonus:
>
> - Add subsections in the documentation (general functions, B-tree
> functions, etc.)
>
Done.
> - Add tests.
>
Thanks for the review !
Best regards,
Jesper
Attachment | Content-Type | Size |
---|---|---|
0001-pageinspect-Hash-index-support_v6.patch | text/x-patch | 35.1 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Jesper Pedersen | 2016-09-26 17:42:06 | Re: pageinspect: Hash index support |
Previous Message | Robert Haas | 2016-09-26 17:31:41 | Re: Speed up Clog Access by increasing CLOG buffers |