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

On Wed, Sep 11, 2019 at 8:53 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> I've attached the updated patch that incorporated all comments. I kept
> the function as superuser-restricted.
>

Thanks for the updated patch.

Few more comments:

*
+ if (!superuser())
+ ereport(ERROR,
+ (errcode
(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("must be superuser to use raw
page functions")));

I think it is better to use a message like "must be superuser to use
pageinspect functions" as this function doesn't take raw page as
input.  If you see other functions like bt_page_items which doesn't
take raw page as input has the message which I am suggesting.  I can
see that tuple_data_split also has a similar message as you are
proposing, but I think that is also not very appropriate.

*
else
+ {
+ if (HEAP_XMAX_IS_LOCKED_ONLY(t_infomask))
+ d[cnt++] = CStringGetTextDatum
("HEAP_XMAX_LOCK_ONLY");

If the idea is that whenever decode_combined flag is false, we will
display the raw flags set on the tuple, then why to try to interpret
flags on a tuple in the above case.

*
+ if (decode_combined &&
+ HEAP_LOCKED_UPGRADED(t_infomask))
+ d[cnt++] = CStringGetTextDatum("HEAP_LOCKED_UPGRADED");
+ else
+ {
+ if (HEAP_XMAX_IS_LOCKED_ONLY(t_infomask))
+ d[cnt++] = CStringGetTextDatum
("HEAP_XMAX_LOCK_ONLY");
+ if ((t_infomask & HEAP_XMAX_IS_MULTI) != 0)
+ d[cnt++] = CStringGetTextDatum
("HEAP_XMAX_IS_MULTI");
+ }

I am not completely sure whether we want to cover HEAP_LOCKED_UPGRADED
case even when decode_combined flag is set.  It seems this is a bit
more interpretation of flags than we are doing in other cases.  For
example, other cases like HEAP_XMAX_SHR_LOCK or HEAP_XMIN_FROZEN are
the flags that are explicitly set on the tuple so displaying them
makes sense, but the same is not true for HEAP_LOCKED_UPGRADED.

*
+CREATE FUNCTION heap_tuple_infomask_flags(
+       t_infomask integer,
+       t_infomask2 integer,
+       decode_combined boolean DEFAULT false)

I am not very happy with the parameter name 'decode_combined'.  It is
not clear from the name what it means and I think it doesn't even
match with what we are actually doing here.  How about raw_flags,
raw_tuple_flags or something like that?

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