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

From: Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
To: Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>
Cc: Michael Paquier <michael(at)paquier(dot)xyz>, Alvaro Herrera from 2ndQuadrant <alvherre(at)alvh(dot)no-ip(dot)org>, Craig Ringer <craig(at)2ndquadrant(dot)com>, Ashutosh Sharma <ashu(dot)coek88(at)gmail(dot)com>, Julien Rouhaud <rjuju123(at)gmail(dot)com>, Peter Geoghegan <pg(at)bowt(dot)ie>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>, Abhijit Menon-Sen <ams(at)2ndquadrant(dot)com>, Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com>, Pavan Deolasee <pavan(dot)deolasee(at)2ndquadrant(dot)com>
Subject: Re: [HACKERS] [PATCH] pageinspect function to decode infomasks
Date: 2019-09-12 05:56:47
Message-ID: CAA4eK1LMJMSKMKsmO-v0SAXD4WaApDBUSyGopY-BZKCT5a3bKg@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Wed, Sep 11, 2019 at 8:53 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Michael Paquier 2019-09-12 06:12:57 Re: [HACKERS] [PATCH] pageinspect function to decode infomasks
Previous Message Thomas Munro 2019-09-12 05:56:00 Parallel Full Hash Join