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

On Sun, Sep 8, 2019 at 1:06 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> 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.
> >
>

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.

*
+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);
+ int cnt = 0;
+ ArrayType *a;
+ int bitcnt;
+ Datum *d;
+
+ bitcnt = pg_popcount((const char *) &t_infomask, sizeof(uint16)) +
+ pg_popcount((const char *) &t_infomask2, sizeof(uint16));

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?

I think one possible explanation could be that here we are not passing
raw page on which this function will operate on, but isn't the same
true for tuple_data_split?

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.

*
+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. You can find the examples in
code both where we free after such usage and where we don't. I prefer
to free it.

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