Re: [PATCH] pageinspect function to decode infomasks

On 20 July 2017 at 19:09, Ashutosh Sharma <ashu.coek88@gmail.com> wrote:

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


Fixed.


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

Fixed. Renamed the function, missed a spot.


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


It was a consequence of (1). Fixed.


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

Fixed, thanks.

I want to find time to expand the tests on this some more and look more
closely, but here it is for now.

-- 
 Craig Ringer                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services