Re: PG17beta2: SMGR: inconsistent type for nblocks

Hi,

On Thu, 1 Aug 2024 at 18:44, Andres Freund <andres(at)anarazel(dot)de> wrote:
> On 2024-08-01 12:45:16 +0200, Matthias van de Meent wrote:
> > 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".

IIf BlockNumber doesn't do it for you, then between plain uint32 and
int64, which would you prefer? int itself doesn't allow syncing of all
blocks of a relation's fork, so that's out for me.

> > 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?

To prevent underflows in the loop below, if any would happen to exist.
Could've been BlockNumber too, but I went with a slightly more
defensive approach.

Kind regards,

Matthias van de Meent