Re: PATCH: pageinspect / add page_checksum and bt_page_items(bytea) - Mailing list pgsql-hackers

Hi Tomas,

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