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

On Thu, Sep 12, 2019 at 4:48 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Thu, Sep 12, 2019 at 05:34:08PM +0800, Masahiko Sawada wrote:
> > On Thu, Sep 12, 2019 at 1:56 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >> 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.
>
> Well, those functions are all able to work only from data of a raw
> page, so the existing message was actually fine by me.  If you want to
> change it this way, I don't see either any arguments against.
>
> > 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.
>
> Same interpretation here.
>

Hmm, I thought when decode_combined flag is set to false means we will
display the raw flags set on the tuple without any further
interpretation.  I think that is what is most people in thread
advocated about.

> >> 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.
>
> For HEAP_LOCKED_UPGRADED, my interpretation was that the current code
> is correct to understand it as a decomposition of HEAP_XMAX_IS_MULTI
> and HEAP_XMAX_LOCK_ONLY, still...
>
> It seems to me that the way we define combined flags is subject to
> a lot of interpretation.
>

Right.

>  Honestly, if we cannot come up with a clear
> definition of what should be combined or not, I would be of the
> opinion to just wipe out the option, and just return in the text array
> the bits which are set.  It has been discussed on the thread that it
> would be confusing to not show combined flags to some users as some
> bits set have rather contrary meaning when set with others.
>

Yes, I think we could have more discussion on this point.  It is not
100% clear how we should interpret this flag and or where to draw a
line.  It might be that whatever we have done is alright, but still,
it is worth more discussion and opinion from a few more people.

> We tell
> the user that all the flag details are defined in htup_details.h in
> the code and the documentation so the information is not in the
> returned data, but in the code.  And I would like to think that users
> of pageinspect are knowledgeable enough about Postgres that they would
> likely never use decode_combined = true.  Likely I am outnumbered
> regarding this point, so I won't push hard on it, still I get that the
> confusion does not come from this module, but in the way the code
> combines and names all the bits for the infomasks :)
>
> And there would be the argument to not use HEAP_XMAX_IS_LOCKED_ONLY()
> in the code.
>
> >> 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.
>
> decode_combined sounds like a good compromise to me.  If there is a
> better consensus, well, let's use it, but I don't find those
> suggestions to be improvements.
>

I think it depends on the meaning of that flag.

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