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

From: Alvaro Herrera from 2ndQuadrant <alvherre(at)alvh(dot)no-ip(dot)org>
To: Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
Cc: Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>, Michael Paquier <michael(at)paquier(dot)xyz>, Craig Ringer <craig(at)2ndquadrant(dot)com>, Ashutosh Sharma <ashu(dot)coek88(at)gmail(dot)com>, Julien Rouhaud <rjuju123(at)gmail(dot)com>, Peter Geoghegan <pg(at)bowt(dot)ie>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>, Abhijit Menon-Sen <ams(at)2ndquadrant(dot)com>, Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com>, Pavan Deolasee <pavan(dot)deolasee(at)2ndquadrant(dot)com>
Subject: Re: [HACKERS] [PATCH] pageinspect function to decode infomasks
Date: 2019-09-09 12:52:51
Message-ID: 20190909125251.GA21347@alvherre.pgsql
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On 2019-Sep-08, Amit Kapila wrote:

> On Thu, Sep 5, 2019 at 2:17 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Alvaro Herrera from 2ndQuadrant 2019-09-09 13:37:35 Re: Commit fest 2019-09
Previous Message Andrew Dunstan 2019-09-09 12:39:20 Re: msys2 vs pg_upgrade/test.sh