Ayush Tiwari <ayushtiwari.slg01@gmail.com> writes:
> On Thu, 2 Jul 2026 at 22:24, Tom Lane <tgl@sss.pgh.pa.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
