On Wed, Jul 15, 2026 at 7:06 AM Tomas Vondra <[email protected]> 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
