On 2020-11-25 20:04, Alvaro Herrera wrote:
> On 2020-Nov-25, Peter Eisentraut wrote:
>
>> bt_page_stats(PG_FUNCTION_ARGS)
>> {
>> text *relname = PG_GETARG_TEXT_PP(0);
>> - uint32 blkno = PG_GETARG_UINT32(1);
>> + int64 blkno = PG_GETARG_INT64(1);
>
> As a matter of style, I think it'd be better to have an int64 variable
> that gets the value from PG_GETARG_INT64(), then you cast that to
> another variable that's a BlockNumber and use that throughout the rest
> of the code. So you'd avoid changes like this:
>
>> static bytea *get_raw_page_internal(text *relname, ForkNumber forknum,
>> - BlockNumber blkno);
>> + int64 blkno);
>
> where the previous coding was correct, and the new one is dubious and it
> forces you to add unnecessary range checks in that function:
>
>> @@ -144,11 +144,16 @@ get_raw_page_internal(text *relname, ForkNumber forknum, BlockNumber blkno)
>> (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
>> errmsg("cannot access temporary tables of other sessions")));
>>
>> + if (blkno < 0 || blkno > MaxBlockNumber)
>> + ereport(ERROR,
>> + (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
>> + errmsg("invalid block number")));
>> +
The point of the patch is to have the range check somewhere. If you
just cast it, then you won't notice out of range arguments. Note that
other contrib modules that take block numbers work the same way.