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

On 03/13/2017 06:49 AM, Ashutosh Sharma wrote:
> Hi,
>
> I had a look into this patch and would like to share some of my review
> comments that requires author's attention.
>
> 1) The comment for page_checksum() needs to be corrected. It seems
> like it has been copied from page_header and not edited it further.
>
> /*
> * page_header
> *
> * Allows inspection of page header fields of a raw page
> */
>
> PG_FUNCTION_INFO_V1(page_checksum);
>
> Datum
> page_checksum(PG_FUNCTION_ARGS)
>

True, will fix.

> 2) It seems like you have choosen wrong datatype for page_checksum. I
> am getting negative checksum value when trying to run below query. I
> think the return type for the SQL function page_checksum should be
> 'integer' instead of 'smallint'.
>
> postgres=# SELECT page_checksum(get_raw_page('pg_class', 0), 100);
> page_checksum
> ---------------
> -19532
> (1 row)
>
> Above problem also exist in case of page_header. I am facing similar
> problem with page_header as well.
>
> postgres=# SELECT page_header(get_raw_page('pg_class', 0));
> page_header
> ---------------------------------------------
> (0/2891538,-28949,1,220,7216,8192,8192,4,0)
> (1 row)
>

No. This is consistent with page_header() which is also using smallint
for the checksum value.

>
> 3) Any reason for not marking bt_page_items or page_checksum as PARALLEL_SAFE.
>

No, not really. It's an oversight.

> 4) Error messages in new bt_page_items are not consistent with old
> bt_page_items. For eg. if i pass meta page blocknumber as input to
> bt_page_items the error message thrown by new and old bt_page_items
> are different.
>
> postgres=# SELECT * FROM bt_page_items('btree_index', 0) LIMIT 1;
> ERROR: block 0 is a meta page
>
> postgres=# SELECT * FROM bt_page_items(get_raw_page('btree_index', 0)) LIMIT 1;
> ERROR: block is a meta page
>

Well, the new function does not actually know the block number, so how
could it include it in the error message? You only get the page itself,
and it might be read from anywhere. Granted, the meta page should only
be stored in block 0, but when the storage mixes up the pages somehow,
that's not reliable.

>
> postgres=# SELECT * FROM bt_page_items(get_raw_page('btree_index',
> 1024)) LIMIT 1;
> ERROR: block number 1024 is out of range for relation "btree_index"
>
>
> postgres=# SELECT * FROM bt_page_items('btree_index', 1024) LIMIT 1;
> ERROR: block number out of range
>

Well, that error message does not come from the new function, it comes
from get_raw_page(), so I'm not sure what am I supposed to do with that?
Similarly to the previous case, the new function does not actually know
the block number at all.

> 5) Code duplication in bt_page_items() and bt_page_items_bytea() needs
> to be handled.
>

Yes. If we adopt the approach proposed by Peter Eisentraut (redirecting
the old bt_page_items using a SQL function calling the new one), it will
also make the error messages consistent.

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services