Re: Fw:Re: Fw: gbt_var_consistent in contrib/btree_gist/btree_utils_var.c has internal-node type confusion on the <> strategy, bypassing exclusion constraints

Hi,

On Fri, 3 Jul 2026 at 04:32, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Ayush Tiwari <ayushtiwari(dot)slg01(at)gmail(dot)com> writes:
> > On Thu, 2 Jul 2026 at 22:24, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >> Anyway, I think part of our work here has got to be to raise the
> >> level of documentation of this code. I've had it with having
> >> to reverse-engineer details as fundamental as these.
>
> > I've added some documentation in code, patch attached.
>
> I had something considerably more ambitious in mind, so I had a
> go at that --- and was glad that I did so, because the review
> turned up an additional bug as well as some other opportunities
> for improvement.
>

Thanks for working on the patch!

v3-0001 is a documentation-improvement patch. I only touched
> the files associated with btree_utils_var.c. Maybe it's worth
> doing something similar for the btree_utils_num.c group, but
> I have the impression that those are way simpler.
>

0001 LGTM

> v3-0002 is your patch, editorialized a bit. I left out the
> test case because it seemed fairly expensive for something
> that won't expose a bug in normal builds. (BTW, I wonder
> if rather than documenting the comparisons as being reversed,
> we should just flip them all.)
>

I'd lean towards flipping them for readability, though I'm not sure
it's worth the backpatch churn? I guess we could start a new
thread for master on this.

v3-0003 fixes a bug I identified: index builds that use the
> gbt_bit_ssup_cmp infrastructure will sort the entries in a
> way that doesn't match the bit types' actual ordering,
> leading to what's probably a very inefficient index.

It looks right to me, leaf entries are real bit/varbit values, so bitcmp
is the right comparator there.

> v3-0004 and v3-0005 clean up some things that seem like
> dead code to me.
>

One small thing on 0005: once gbt_var_node_cp_len stops using
pg_mblen_range, the "#include "mb/pg_wchar.h"" in btree_utils_var.c
looks like it becomes unused. Might be worth dropping it too.

> Also, it occurs to me after another look at 0002 that what was
> perhaps intended was something like this for the non-leaf case:
>
> if (is_leaf)
> retval = !(tinfo->f_eq(query, key->lower, collation,
> flinfo));
> else
> retval = tinfo->trnc ||
> !(tinfo->f_cmp(query, key->lower, collation, flinfo)
> == 0 &&
> tinfo->f_cmp(query, key->upper, collation, flinfo)
> == 0);
>
> That is, if we're looking at non-truncated keys and lower == upper,
> then we know that all the keys below this node are exactly that
> value, so we can avoid descending if that value is equal to the query.
> Most of the time this would not pay off in any savings, but if you
> had an index on a column with only a few values, maybe there would be
> leaf pages like that? I would think that the planner would avoid
> choosing to implement a <> query with an indexscan unless there was
> a pretty large fraction of the table that <> would reject, so maybe
> this actually is worth doing.
>

I think that optimization is correct. For the non-truncatable types
the internal lower/upper are exact min/max (built via f_cmp in
gbt_var_bin_union with no truncation), so if both equal the query
then everything below does too, and pruning can't drop a match.
Are you planning on including it in the backpatch or to keep the
optimization just part of master, and the bug fix backpatched?

Regards,
Ayush