On Thu, Dec 18, 2025 at 3:18 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>
> The smgr interface isn't really an extension point, so we don't need to
> worry about that. (I wish it was, but that's a different story [1])
Thanks for the additional context.
> I don't think mdtruncate() should modify smgr_cached_nblocks, we should
> keep that in smgrtruncate(). smgr.c is responsible for all other updates
> of smgr_cached_nblocks, so doing it in mdtruncate() would be a layering
> violation.
>
> I'm thinking that we should do the attached. Untested, and we should
> also add a comment to smgrtruncate() and mdtruncate() to explain how
> they behave if nblocks > curnblk.
This appears to work. The cached size is correctly updated and my
script doesn't trigger the bug anymore. I've updated the patch with
this approach.
> I wonder if we should move the whole "if (nblocks > curnblk)" check and
> ereport() from mdtruncate() to smgrtruncate(). That logic doesn't really
> depend on anything specific to md.c. If you'd imagine a different smgr
> implementation, it'd need to just copy-paste that check. It's the
> caller's mistake if it passes nblocks > curnblk, when not in recovery.
> Then again, we do have other places in md.c too that behave differently
> when InRecovery.
>
> What do you think?
I imagine we will still want some sanity checks in mdtruncate(), or at
least an Assert to make sure the provided block values are correct.
This also assumes that we will only have one caller to mdtruncate(),
another caller will have to duplicate the check.
And in the context of a backpatch, the current approach has the
benefit of minimising the amount of change, so I am slightly partial
to keeping the check in mdtruncate().
Regards,
Anthonin Bonnefoy
