Re: BTMaxItemSize seems to be subtly incorrect

On Wed, Jun 8, 2022 at 4:18 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> I wasn't originally setting out to modify BTPageOpaqueData at all,
> just borrow some special space. See the "storing an explicit nonce"
> discussion and patch set. But when this regression failure turned up I
> said to myself, hmm, I think this is an unrelated bug.

FWIW I don't see much difference between borrowing special space and
adding something to BTPageOpaqueData. If anything I'd prefer the
latter.

> > I think that we should fix this on HEAD, on general principle. There
> > is no reason to believe that this is a live bug, so a backpatch seems
> > unnecessary.
>
> Yeah, I agree with not back-patching the fix, unless it turns out that
> there is some platform where the same issue occurs without any
> cabbage. I assume that if it happened on any common system someone
> would have complained about it by now, so probably it doesn't.

I don't think it's possible on 32-bit platforms, which is the only
further variation I can think of. But let's assume that the same
problem was in fact possible "without cabbage", just for the sake of
argument. The worst problem that could result would be a failure to
split a page that had maximally large keys, without TOAST compression
-- which is what you demonstrated. There wouldn't be any downstream
consequences.

Here's why: BTMaxItemSizeNoHeapTid() is actually what BTMaxItemSize()
looked like prior to Postgres 12. So the limit on internal pages never
changed, even in Postgres 12. There was no separate leaf page limit
prior to 12. Only the rules on the leaf level ever really changed.

Note also that amcheck has tests for this stuff. Though that probably
doesn't matter at all.

--
Peter Geoghegan