From: Alvaro Herrera from 2ndQuadrant <alvherre@alvh.no-ip.org>
To: Amit Kapila <amit.kapila16@gmail.com>
Cc: Masahiko Sawada <sawada.mshk@gmail.com>
Cc: Michael Paquier <michael@paquier.xyz>
Cc: Craig Ringer <craig@2ndquadrant.com>
Cc: Ashutosh Sharma <ashu.coek88@gmail.com>
Cc: Julien Rouhaud <rjuju123@gmail.com>
Cc: Peter Geoghegan <pg@bowt.ie>
Cc: PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Cc: Abhijit Menon-Sen <ams@2ndquadrant.com>
Cc: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
Cc: Pavan Deolasee <pavan.deolasee@2ndquadrant.com>
Subject: Re: [HACKERS] [PATCH] pageinspect function to decode infomasks
Date: Mon, 9 Sep 2019 09:52:51 -0300
Message-ID: <20190909125251.GA21347@alvherre.pgsql> (raw)
In-Reply-To: <CAA4eK1LmpXd6d+getjPGxa74M_DZ+BD2PA8-LU-cOXfDEMJB5A@mail.gmail.com> <CAA4eK1LPvgUS3ZJ9vsLQ7qsoEWpZsD5fwyzCsgMHkBcu+6-XAQ@mail.gmail.com>
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: alvherre@alvh.no-ip.org, amit.kapila16@gmail.com, sawada.mshk@gmail.com, michael@paquier.xyz, craig@2ndquadrant.com, ashu.coek88@gmail.com, rjuju123@gmail.com, pg@bowt.ie, ams@2ndquadrant.com, peter.eisentraut@2ndquadrant.com, pavan.deolasee@2ndquadrant.com
Subject: Re: [HACKERS] [PATCH] pageinspect function to decode infomasks
In-Reply-To: <20190909125251.GA21347@alvherre.pgsql>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox