On 2019-Sep-08, Amit Kapila wrote:
> On Thu, Sep 5, 2019 at 2:17 PM Masahiko Sawada <sawada.m...@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
