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

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(dot)kapila16(at)gmail(dot)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.

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