Re: [HACKERS] [PATCH] pageinspect function to decode infomasks

On Mon, Sep 9, 2019 at 6:22 PM Alvaro Herrera from 2ndQuadrant
<alvherre@alvh.no-ip.org> wrote:
>
> On 2019-Sep-08, Amit Kapila wrote:
>
> > On Thu, Sep 5, 2019 at 2:17 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > >
> > > Thanks. I hope the attached new patch fixes this issue.
> > *
> > +-- decode infomask flags as human readable flag names
> > +CREATE FUNCTION heap_infomask_flags(
> > +       infomask integer,
> > +       infomask2 integer,
> > +       decode_combined boolean DEFAULT false)
> > +RETURNS text[]
> > +AS 'MODULE_PATHNAME', 'heap_infomask_flags'
> >
> > Isn't it better to name this function as tuple_infomask_flags or
> > something like that?  The other functions that start their name with
> > heap take page as input.  Also, I think the index-related functions
> > that start with index name take blk/page as input.
>
> I think that other table AMs are not necessarily going to use the same
> infomask flags, so I think we should keep a name that is somehow
> heapam-specific.  Maybe "heapam_infomask_flags" would be okay?
>

It will look bit strange to use heapam as a prefix for this function
when all others use heap.  I guess if we want to keep it AM specific,
then the proposed name (heap_infomask_flags) is better or
alternatively we can consider heap_tuple_infomask_flags?

>
> > Some more comments:
> > *
> > +SELECT t_infomask, t_infomask2, flags
> > +FROM heap_page_items
> > (get_raw_page('test1', 0)),
> > +     LATERAL heap_infomask_flags(t_infomask,
> > t_infomask2, true) m(flags);
> >
> > Do we really need a LATERAL clause in the above kind of queries?
> > AFAIU, the functions can reference the columns that exist in the FROM
> > list, but I might be missing some point.
>
> I think the spec allows the LATERAL keyword to be implicit in the case
> of functions, so this seems a matter of style.  Putting LATERAL
> explicitly seems slightly clearer, to me.
>

No problem.

> > +Datum
> > +heap_infomask_flags(PG_FUNCTION_ARGS)
> > +{
> > + uint16 t_infomask = PG_GETARG_INT16(0);
> > + uint16 t_infomask2 = PG_GETARG_INT16(1);
> > + bool decode_combined = PG_GETARG_BOOL(2);
>
> > All the functions in this file are allowed only for superusers and
> > there is an explanation for the same as mentioned in the file header
> > comments.  Is there a reason for this function to be different?
>
> The other functions can crash if fed arbitrary input.  I don't think
> this one can crash, so it seems okay for it not to be superuser-only.
>

At the beginning of pageinspect documentation page, we have a line
"All of these functions may be used only by superusers.".  We need to
change that and then maybe give some explanation of why this
particular function will be allowed to non-superusers.  BTW, do you
have any use case in mind for the same because anyway we need
superuser privileges to get the page contents and I think this
function can't be used independently?

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