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

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?

> +     <function>heap_infomask_flags(infomask integer, infomask2
> integer, decode_combined bool) returns text[]</function>

> I think it is better to use one example for this function as we do for
> other functions in this doc.

Agreed.

> +     <para>
> +      If decode_combined is set, combination flags like
> +      <literal>HEAP_XMIN_FROZEN</literal> are output instead of raw
> +      flags, <literal>HEAP_XMIN_COMMITTED</literal> and
> +      <literal>HEAP_XMIN_INVALID</literal>. Default value is
> +      <literal>false</literal>.
> +     </para>
> 
> decode_combined should use parameter marker (like
> <parameter>decode_combined</parameter>).  Instead of saying
> "decode_combined" is set, you can use true/false terminology as that
> is what we use for this parameter.

Agreed.

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

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

> In any case, if you think that this function needs to behave
> differently w.r.t superuser privileges, then it is better to add some
> comments in the function header to explain the same.

Yeah.

> *
> +Datum
> +heap_infomask_flags(PG_FUNCTION_ARGS)
> {
> ..
> +
> + d = (Datum *) palloc0(sizeof(Datum) * bitcnt);
> 
> It seems we don't free this memory before leaving this function.  I
> think it shouldn't be a big problem as this will be normally allocated
> in ExprContext and shouldn't last for long, but if there is no strong
> reason, I think it is better to free it.

Agreed.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services