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.m...@gmail.com> wrote: > On Thu, Jul 20, 2017 at 3:13 PM, Julien Rouhaud <rjuju...@gmail.com> wrote: >> On Thu, Jul 20, 2017 at 5:44 AM, Peter Geoghegan <p...@bowt.ie> wrote: >>> On Wed, Jul 19, 2017 at 8:33 PM, Craig Ringer <cr...@2ndquadrant.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@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers