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")));
> +
