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

On Thu, Sep 12, 2019 at 1:56 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> 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:

Thank you for your 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.

Agreed. Will fix.

>
> *
> 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.

Hmm my understanding of 'decode_combined' is to decode the flags that
we represent by using multiple flags. HEAP_XMAX_IS_LOCKED_ONLY is true
either if HEAP_XMAX_LOCK_ONLY is set or not a multi and the EXCL_LOCK
bit is set. That is it requires only one flag. So I thought that it's
not a combined flag.

>
> *
> + 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.
>

I thought it would be better to interpret it as much as possible,
especially for diagnostic use cases. I'm concerned that user might not
be able to get enough information for investigation if we
intentionally filtered particular flags.

> *
> +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?
>

raw_flags might be more straightforward. Or perhaps the third idea
could be show_raw_flags? If other hackers agree to change the flag
name I'll fix it.

I'll submit the patch to fix the commit after we got a consensus on
the above changes.



Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center