Re: [PATCH] pageinspect function to decode infomasks

I had a quick look into this patch and also tested it and following
are my observations.

1) I am seeing a server crash when passing any non meaningful value
for t_infomask2 to heap_infomask_flags().

postgres=# SELECT heap_infomask_flags(2816, 3);
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.
!> \q

Following is the backtrace,

(gdb) bt
#0 0x0000000000d9c55b in pg_detoast_datum (datum=0x0) at fmgr.c:1833
#1 0x0000000000b87374 in construct_md_array (elems=0x2ad74c0,
nulls=0x0, ndims=1, dims=0x7ffc0b0bbcd0, lbs=0x7ffc0b0bbcc0,
elmtype=25, elmlen=-1,
elmbyval=0 '\000', elmalign=105 'i') at arrayfuncs.c:3382
#2 0x0000000000b8709f in construct_array (elems=0x2ad74c0, nelems=10,
elmtype=25, elmlen=-1, elmbyval=0 '\000', elmalign=105 'i') at
arrayfuncs.c:3316
#3 0x00007fb8001603a5 in heap_infomask_flags (fcinfo=0x2ad3b88) at
heapfuncs.c:597
#4 0x000000000082f4cd in ExecInterpExpr (state=0x2ad3aa0,
econtext=0x2ad3750, isnull=0x7ffc0b0bbf67 "") at execExprInterp.c:672
#5 0x000000000088b832 in ExecEvalExprSwitchContext (state=0x2ad3aa0,
econtext=0x2ad3750, isNull=0x7ffc0b0bbf67 "")
at ../../../src/include/executor/executor.h:290
#6 0x000000000088b8e3 in ExecProject (projInfo=0x2ad3a98) at
../../../src/include/executor/executor.h:324
#7 0x000000000088bb89 in ExecResult (node=0x2ad36b8) at nodeResult.c:132
#8 0x00000000008494fe in ExecProcNode (node=0x2ad36b8) at execProcnode.c:416
#9 0x000000000084125d in ExecutePlan (estate=0x2ad34a0,
planstate=0x2ad36b8, use_parallel_mode=0 '\000', operation=CMD_SELECT,
sendTuples=1 '\001',
numberTuples=0, direction=ForwardScanDirection, dest=0x2ac0ae0,
execute_once=1 '\001') at execMain.c:1693
#10 0x000000000083d54b in standard_ExecutorRun (queryDesc=0x2a42880,
direction=ForwardScanDirection, count=0, execute_once=1 '\001') at
execMain.c:362
#11 0x000000000083d253 in ExecutorRun (queryDesc=0x2a42880,
direction=ForwardScanDirection, count=0, execute_once=1 '\001') at
execMain.c:305
#12 0x0000000000b3dd8f in PortalRunSelect (portal=0x2ad1490, forward=1
'\001', count=0, dest=0x2ac0ae0) at pquery.c:932
#13 0x0000000000b3d7e7 in PortalRun (portal=0x2ad1490,
count=9223372036854775807, isTopLevel=1 '\001', run_once=1 '\001',
dest=0x2ac0ae0, altdest=0x2ac0ae0,
completionTag=0x7ffc0b0bc2c0 "") at pquery.c:773
#14 0x0000000000b31fe4 in exec_simple_query (query_string=0x2a9d9a0
"SELECT heap_infomask_flags(11008, 1111, true);") at postgres.c:1099
#15 0x0000000000b3a727 in PostgresMain (argc=1, argv=0x2a49eb0,
dbname=0x2a1d480 "postgres", username=0x2a49d18 "ashu") at
postgres.c:4090
#16 0x0000000000a2cb3f in BackendRun (port=0x2a3e700) at postmaster.c:4357
#17 0x0000000000a2bc63 in BackendStartup (port=0x2a3e700) at postmaster.c:4029
#18 0x0000000000a248ab in ServerLoop () at postmaster.c:1753
#19 0x0000000000a236a9 in PostmasterMain (argc=3, argv=0x2a1b2b0) at
postmaster.c:1361
#20 0x00000000008d8054 in main (argc=3, argv=0x2a1b2b0) at main.c:228

2) I can see the documentation for heap_infomask(). But, I do not see
it being defined or used anywhere in the patch.

+ <para>
+ The <function>heap_infomask</function> function can be used to unpack the
+ recognised bits of the infomasks of heap tuples.
+ </para>

3) If show_combined flag is set to it's default value and a tuple is
frozen then may i know the reason for not showing it as frozen tuple
when t_infomask2
is passed as zero.

postgres=# SELECT heap_infomask_flags(2816, 0);
heap_infomask_flags
-----------------------------------------------------------
{HEAP_XMIN_COMMITTED,HEAP_XMIN_INVALID,HEAP_XMAX_INVALID}
(1 row)

postgres=# SELECT heap_infomask_flags(2816, 1);
heap_infomask_flags
----------------------------------------------------------------------------
{HEAP_XMIN_COMMITTED,HEAP_XMIN_INVALID,HEAP_XMAX_INVALID,HEAP_XMIN_FROZEN}
(1 row)

4) I think, it would be better to use the same argument name for the
newly added function i.e heap_infomask_flags() in both documentation
and sql file. I am basically refering to 'include_combined' argument.
IF you see the function definition, the argument name used is
'include_combined' whereas in documentation you have mentioned
'show_combined'.

--
With Regards,
Ashutosh Sharma
EnterpriseDB:http://www.enterprisedb.com

On Thu, Jul 20, 2017 at 11:56 AM, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> On Thu, Jul 20, 2017 at 3:13 PM, Julien Rouhaud <rjuju123(at)gmail(dot)com> wrote:
>> On Thu, Jul 20, 2017 at 5:44 AM, Peter Geoghegan <pg(at)bowt(dot)ie> wrote:
>>> On Wed, Jul 19, 2017 at 8:33 PM, Craig Ringer <craig(at)2ndquadrant(dot)com> wrote:
>>>> That's silly, so here's a patch to teach pageinspect how to decode infomasks
>>>> to a human readable array of flag names.
>>>>
>>>> Example:
>>>>
>>>> SELECT t_infomask, t_infomask2, flags
>>>> FROM heap_page_items(get_raw_page('test1', 0)),
>>>> LATERAL heap_infomask_flags(t_infomask, t_infomask2, true) m(flags);
>>>> t_infomask | t_infomask2 | flags
>>>> ------------+-------------+----------------------------------------------------------------------------
>>>> 2816 | 2 |
>>>> {HEAP_XMIN_COMMITTED,HEAP_XMIN_INVALID,HEAP_XMAX_INVALID,HEAP_XMIN_FROZEN}
>>>> (1 row)
>>>
>>> Seems like a good idea to me.
>>>
>>
>> +1, it'll be really helpful.
>>
>
> +1.
> When I investigated data corruption incident I also wrote a plpgsql
> function for the same purpose, and it was very useful. I think we can
> have the similar thing for lp_flags as well.
>
> Regards,
>
> --
> Masahiko Sawada
> NIPPON TELEGRAPH AND TELEPHONE CORPORATION
> NTT Open Source Software Center
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers