Re: [HACKERS] PATCH: pageinspect / add page_checksum andbt_page_items(bytea)

On 03/06/2017 10:13 PM, Peter Eisentraut wrote:
> On 3/3/17 09:03, Tomas Vondra wrote:
>> Attached is v2, fixing both issues.
>
> I wonder if
>
> +   bytea      *raw_page = PG_GETARG_BYTEA_P(0);
>
> +       uargs->page = VARDATA(raw_page);
>
> is expected to work reliably, without copying the argument to a
> different memory context.
>
> I think it would be better not to maintain so much duplicate code
> between bt_page_items(text, int) and bt_page_items(bytea).  How about
> just redefining bt_page_items(text, int) as an SQL-language function
> calling bt_page_items(get_raw_page($1, $2))?
>

Maybe. We can also probably share the code at the C level, so I'll look 
into that.
>
> For page_checksum(), the checksums are internally unsigned, so maybe
> return type int on the SQL level, so that there is no confusion about
> the sign?
>

That ship already sailed, I'm afraid. We already have checksum in the 
page_header() output, and it's defined as smallint there. So using int 
here would be inconsistent.

A similar issue is that get_raw_page() uses int for block number, while 
internally it's uint32. That often requires a fair amount of gymnastics 
on large tables.

I'm not against changing either of those bits - using int for checksums 
and bigint for block numbers, but I think it should be a separate patch.

regards

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