Re: PG17beta2: SMGR: inconsistent type for nblocks

Hi,

On 2024-08-01 12:45:16 +0200, Matthias van de Meent wrote:
> On Tue, 30 Jul 2024 at 14:32, Thomas Munro <thomas(dot)munro(at)gmail(dot)com> wrote:
> >
> > On Tue, Jul 30, 2024 at 11:24 PM Matthias van de Meent
> > <boekewurm(at)gmail(dot)com> wrote:
> > > While working on rebasing the patches of Neon's fork onto the
> > > REL_17_STABLE branch, I noticed that the nblocks arguments of various
> > > smgr functions have inconsistent types: smgrzeroextend accepts
> > > `nblocks` as signed integer, as does the new signature for
> > > smgrprefetch, but the new vectorized operations of *readv and *writev,
> > > and the older *writeback all use an unsigned BlockNumber as indicator
> > > for number of blocks.
> > >
> > > Can we update the definition to be consistent across this (new, or
> > > also older) API? As far as I can see, in none of these cases are
> > > negative numbers allowed or expected, so updating this all to be
> > > consistently BlockNumber across the API seems like a straigthforward
> > > patch.
> > >
> > > cc-ed Thomas as committer of the PG17 smgr API changes.
> >
> > Yeah, right, I noticed that once myself[1]. For the cases from my
> > keyboard, I guess I was trying to be consistent with nearby existing
> > stuff in each case, which was already inconsistent... Do you have a
> > patch?
>
> Here's one that covers both master and the v17 backbranch.

FWIW, I find it quite ugly to use BlockNumber to indicate the number of blocks
to be written. It's just further increasing the type confusion by conflating
"the first block to be targeted" and "number of blocks".

> diff --git a/src/backend/storage/smgr/md.c b/src/backend/storage/smgr/md.c
> index 6796756358..1d02766978 100644
> --- a/src/backend/storage/smgr/md.c
> +++ b/src/backend/storage/smgr/md.c
> @@ -523,11 +523,11 @@ mdextend(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum,
> */
> void
> mdzeroextend(SMgrRelation reln, ForkNumber forknum,
> - BlockNumber blocknum, int nblocks, bool skipFsync)
> + BlockNumber blocknum, BlockNumber nblocks, bool skipFsync)
> {
> MdfdVec *v;
> BlockNumber curblocknum = blocknum;
> - int remblocks = nblocks;
> + int64 remblocks = nblocks;
>
> Assert(nblocks > 0);

Isn't this particularly bogus? What's the point of using a 64bit remblocks
here?

Greetings,

Andres Freund