Hi Tomas,

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

Reply via email to