On Wed, Jul 15, 2026 at 7:06 AM Tomas Vondra <tomas@vondra.me> wrote:
> Thanks. I took a look at the v2 fix, and I think it's correct / fine. It
> does surprise me a bit we've never seen any reports of failures, but I
> guess the problematic opclasses are not used very often. Or not with an
> index that would trigger it.
Yeah, it's quite a narrow issue.
Pushed a slightly improved version of this fix just now, backpatching
all the way. I improved the comments in line with your feedback.
> I think moving the logic to StoreIndexTuple is a clear improvement.
Yes, it's definitely better this way. We're literally doing exactly
the same thing when we handle the xs_itup and xs_hitup cases, so it
makes no sense to handle the xs_hitup directly while only having
xs_itup-related logic in the separate StoreIndexTuple helper function.
> A couple comment nitpicks:
>
> * In either case we must deform the tuple using the tupdesc the AM
> * formed it with (xs_hitupdesc or xs_itupdesc), not the slot's tupdesc.
> * An AM builds that descriptor from its opclass, so a column's type
> * there can differ ...
>
> It's not entirely clear which descriptior "that descriptor" refers to.
I fixed that. I also cut down the comments quite a bit in the committed patch.
> Maybe this should say "may not match"?
Agreed.
> * anyrange forms its tuples with that type's alignment, which need not
> * match the alignment of the actual type.
>
> I wonder if we want to mention particular opclasses in comments,
> references like this are easy to go stale / obsolete. But it's probably
> worth it, not sure.
The committed version has comments mentioning rare cases involving
nonmatching column alignment. We no longer mention any opclass by name
(except for btree's name_ops, which StoreIndexTuple must handle as a
special case).
I concluded that discussing the specific issue in detail wasn't
necessary. There's likely to be nbtree opclasses that we'd have subtly
broken if we somehow made the same mistake with nbtree/xs_itup (which
never happened, likely because doing so would break name_ops in an
obvious way). It would make about as much sense to mention these
never-affected nbtree opclasses as it would to mention the historical
issue with gist/anyrange. Let's mention neither.
Thanks for the review!
--
Peter Geoghegan
