Hi Tomas,
On Fri, Mar 31, 2017 at 11:05 PM, Tomas Vondra <tomas.von...@2ndquadrant.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
