PG17beta2: SMGR: inconsistent type for nblocks
Hackorum builds and tests every patch posted to the lists, not only commitfest submissions. This is Hackorum's own CI rather than the PostgreSQL project's, and it is still under testing - please report anything that looks wrong.
You can run a PostgreSQL built from this patch straight from Docker, with no checkout and no build:
docker run --rm -p 5432:5432 ghcr.io/hackorum-dev/postgres-patch:t50031psql -h localhost -U postgresBuilt from patchset v3 (message #3), July 27, 2026 at 11:53 AM.
Every patchset is also pushed to a branch of our PostgreSQL fork, so you can check out the same tree CI built. Without a PostgreSQL checkout:
git clone --branch t50031_3 https://github.com/hackorum-dev/postgres.gitIn a checkout you already have, add the fork once:
git remote add hackorum https://github.com/hackorum-dev/postgres.gitthen, for this patchset and every later one:
git fetch hackorum t50031_3 && git checkout t50031_3Patchset v3 (message #3) is on t50031_3
Hi,
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.
Kind regards,
Matthias van de Meent
Neon (https://neon.tech)
On Tue, Jul 30, 2024 at 11:24 PM Matthias van de Meent
<boekewurm@gmail.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.
Hi Matthias,
Yeah, right, I noticed that once myself[1]/messages/by-id/CA+hUKGLx5bLwezZKAYB2O_qHj=ov10RpgRVY7e8TSJVE74oVjg@mail.gmail.com. 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?
[1]: /messages/by-id/CA+hUKGLx5bLwezZKAYB2O_qHj=ov10RpgRVY7e8TSJVE74oVjg@mail.gmail.com
On Tue, 30 Jul 2024 at 14:32, Thomas Munro <thomas.munro@gmail.com> wrote:
On Tue, Jul 30, 2024 at 11:24 PM Matthias van de Meent
<boekewurm@gmail.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.
Kind regards,
Matthias van de Meent
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.munro@gmail.com> wrote:
On Tue, Jul 30, 2024 at 11:24 PM Matthias van de Meent
<boekewurm@gmail.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
Hi,
On Thu, 1 Aug 2024 at 18:44, Andres Freund <andres@anarazel.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