Re: GIN pageinspect functions

On 11/20/2014 05:52 AM, Michael Paquier wrote:
> On Wed, Nov 19, 2014 at 7:01 AM, Peter Geoghegan <pg@heroku.com> wrote:
>> On Tue, Nov 4, 2014 at 7:26 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
>> 1. Documentation seems to be missing, other API's exposed
>> via pageinspect are documented at:
>> http://www.postgresql.org/docs/devel/static/pageinspect.html
> Done.
>
>> 2.
>> +CREATE FUNCTION gin_metapage(IN page bytea,
>> +    OUT pending_head bigint,
>> +    OUT pending_tail bigint,
>> +    OUT version int4)
>> +AS 'MODULE_PATHNAME', 'gin_metapage'
>> +LANGUAGE C STRICT;
>> a. Isn't it better to name the function as gin_metap(..) similar to
>> existing function bt_metap(..)?
> I actually liked more gin_metapage_info, a name similar to the
> newly-introduced brin indexes.
>
>> b. Can this function have a similar signature as bt_metap() which means
>> it should take input as relname?
> That's mostly a matter of taste but I think we should definitely pass
> a raw page to it as it is now. This has the advantage to add an extra
> check if the page passed is really a meta page of not, something
> useful for development.
>
>> 3. Can gin_dataleafpage() API have similar name and signature as
>> API bt_page_items() exposed for btree?
> What about gin_leafpage_items then?

The signature of bt_page_items() isn't a good example to follow. It 
existed before the get_raw_page() function, and the other functions that 
are designed to work with that, was added. gin_leafpage_items() name 
seems fine to me.

>> 4. Can we have any better name for gin_pageopaq (other API name's
>> in this module are self explanatory)?
> gin_page_opaque_info? Because we get back information about the opaque
> portion of the page. Feel free if you have any better idea.
>
> Updated patch, with some more things improved and cleaned up (addition
> of header of ginfuncs.c, addition of array of decoded item pointers
> for compressed data leaf pages), is attached.

This is why I love open source - I post something half-baked, and others 
pop up and finish the work ;-). Committed with minor fixes, many thanks!

> One last thing not only interesting for this patch: it may be good to
> expose DatumGetItemPointer and ItemPointerGetDatum in for extensions
> analyzing content of pages. I am not sure where though, a place like
> utils/*.h may be useful. Thoughts?

Yeah, maybe. I'll leave that to the next patch that needs it, as long as 
there's only one user of it, it doesn't seem worth it.

- Heikki