Re: PATCH: pageinspect / add page_checksum and bt_page_items(bytea)

Hi Tomas,

On Fri, Mar 31, 2017 at 11:05 PM, Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com>
wrote:
> On 03/31/2017 06:01 PM, Ashutosh Sharma wrote:
>>
>>
>> It seems like the latest patch(v4) shared by Tomas upthread is an
>> empty patch. If I am not wrong, please share the correct patch.
>> Thanks.
>>
>
> OMG, this is the second time I managed to generate an empty patch. I
really
> need to learn not to do that ..
>
> Attached is v5 of the patch, hopefully correct this time ;-)

Yes, it was correct. I have spent some time on reviewing your patch today
and the review comments are as follows.

1. It would be good to have a blank line in between following set of lines
describing bt_page_print_tuples.

+/*
+ * bt_page_print_tuples
+ * form a tuple describing index tuple at a given offset
+ */

Something like this,

+/* ---------------------------------------------------------------------
+ * bt_page_print_tuples()
+ *
+ * Form a tuple describing index tuple at a given offset
+ * ---------------------------------------------------------------------
+ */

Please note that, if you do not agree with my suggestion, you may ignore it
:)

2. I think the following comments for bt_page_items needs to be moved to
the right place in the code.

/*-------------------------------------------------------------------------
* bt_page_items()
*
* Get IndexTupleData set in a btree page
*
* Usage: SELECT * FROM bt_page_items('t1_pkey', 1);
*-------------------------------------------------------------------------
*/

/*
* cross-call data structure for SRF
*/

*struct user_args{ Page page; OffsetNumber offset;};*

With your patch, above structure definition is followed by the definition
for bt_page_print_tuples() not bt_page_items(), hence you may need to put
the comments for bt_page_items just above it's definition.

3. I am seeing a server crash when running the sql statement 'SELECT * FROM
bt_page_items('bt_i4_index', 1);'. Here is the backtrace.

I think this crash is happening because in bt_page_items, without reading
opaque region of a page, you have added this if statement,

*if (P_ISMETA(opaque)) ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE), errmsg("block
is a meta page")));*

Please pull back above if statement below the following line to get rid of
this crash.

opaque = (BTPageOpaque) PageGetSpecialPointer(uargs->page);

OR, the best thing to do would be to retain the earlier if statement for
checking meta page because that way you can avoid reading a
buffer unnecessarily.

#0 0x00007f592975030c in bt_page_items (fcinfo=0x7fffe5fdb860) at
btreefuncs.c:352
352 if (P_ISMETA(opaque))
Missing separate debuginfos, use: debuginfo-install glibc-2.17-78.el7.x86_64
(gdb) bt
#0 0x00007f592975030c in bt_page_items (fcinfo=0x7fffe5fdb860) at
btreefuncs.c:352
#1 0x00000000006797e0 in ExecMakeTableFunctionResult (setexpr=0x21269f8,
econtext=0x2126738, argContext=0x2136c98, expectedDesc=0x2155178,
randomAccess=0 '\000') at execSRF.c:230
#2 0x0000000000689dda in FunctionNext (node=0x2126628) at
nodeFunctionscan.c:94
#3 0x0000000000678f0e in ExecScanFetch (node=0x2126628, accessMtd=0x689d23
<FunctionNext>, recheckMtd=0x68a108 <FunctionRecheck>) at execScan.c:95
#4 0x0000000000678f7d in ExecScan (node=0x2126628, accessMtd=0x689d23
<FunctionNext>, recheckMtd=0x68a108 <FunctionRecheck>) at execScan.c:143

4. If you agree with the reason mentioned by me in comment #3, please
remove the following if statement from bt_page_items(),

*if (P_ISMETA(opaque)) ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE), errmsg("block
is a meta page")));*

Apart from above comments, your patch looks good to me. I have also marked
this patch as 'Waiting for Author' in the commitfest. Thanks.

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