MIME-Version: 1.0
References: 
 <AH*AvQCYKhQGVvPWi1GiU4oY.8.1781609375063.Hmail.3020001251@tju.edu.cn>
 <CAJTYsWWSx+5-kVaivB_znaQCk3TmHrO4QNNM-CD3AN-oOSmG8w@mail.gmail.com>
 <AGoA0AD5KvwQ8G7e6ofMyap6.3.1781760245047.Hmail.3020001251@tju.edu.cn>
 <3748108.1783003096@sss.pgh.pa.us>
 <CAJTYsWX1C5JLj92QdqEcArWs4XtXPog+yvyb6=X=_cghjXnkYQ@mail.gmail.com>
 <3756776.1783011257@sss.pgh.pa.us>
 <CAJTYsWVus+7hn846A0gMJYk+9uUGZxizk5Herbxb4CmhF3vndQ@mail.gmail.com>
 <3793780.1783033337@sss.pgh.pa.us>
In-Reply-To: <3793780.1783033337@sss.pgh.pa.us>
From: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Date: Fri, 3 Jul 2026 14:10:44 +0530
Message-ID: 
 <CAJTYsWUg+ZH=W5EduH1AEDfJU0waO4uU4hakaiOF+KKUKFbR9g@mail.gmail.com>
Subject: 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
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: =?UTF-8?B?546L6LeD5p6X?= <violin0613@tju.edu.cn>,
	pgsql-bugs <pgsql-bugs@postgresql.org>, 3764353996 <3764353996@qq.com>
Content-Type: multipart/alternative; boundary="0000000000000fd1d00655b0e2ef"
Archived-At: 
 <https://www.postgresql.org/message-id/CAJTYsWUg%2BZH%3DW5EduH1AEDfJU0waO4uU4hakaiOF%2BKKUKFbR9g%40mail.gmail.com>
Precedence: bulk

--0000000000000fd1d00655b0e2ef
Content-Type: text/plain; charset="UTF-8"

Hi,

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

> 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

--0000000000000fd1d00655b0e2ef
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

<div dir=3D"ltr"><div dir=3D"ltr"><div dir=3D"ltr">Hi,</div><br><div class=
=3D"gmail_quote"><div dir=3D"ltr" class=3D"gmail_attr">On Fri, 3 Jul 2026 a=
t 04:32, Tom Lane &lt;<a href=3D"mailto:tgl@sss.pgh.pa.us" target=3D"_blank=
">tgl@sss.pgh.pa.us</a>&gt; wrote:<br></div><blockquote class=3D"gmail_quot=
e" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204)=
;padding-left:1ex">Ayush Tiwari &lt;<a href=3D"mailto:ayushtiwari.slg01@gma=
il.com" target=3D"_blank">ayushtiwari.slg01@gmail.com</a>&gt; writes:<br>
&gt; On Thu, 2 Jul 2026 at 22:24, Tom Lane &lt;<a href=3D"mailto:tgl@sss.pg=
h.pa.us" target=3D"_blank">tgl@sss.pgh.pa.us</a>&gt; wrote:<br>
&gt;&gt; Anyway, I think part of our work here has got to be to raise the<b=
r>
&gt;&gt; level of documentation of this code.=C2=A0 I&#39;ve had it with ha=
ving<br>
&gt;&gt; to reverse-engineer details as fundamental as these.<br>
<br>
&gt; I&#39;ve added some documentation in code, patch attached.<br>
<br>
I had something considerably more ambitious in mind, so I had a<br>
go at that --- and was glad that I did so, because the review<br>
turned up an additional bug as well as some other opportunities<br>
for improvement.<br></blockquote><div><br></div><div>Thanks for working on =
the patch!=C2=A0<br><br></div><blockquote class=3D"gmail_quote" style=3D"ma=
rgin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:=
1ex">
v3-0001 is a documentation-improvement patch.=C2=A0 I only touched<br>
the files associated with btree_utils_var.c.=C2=A0 Maybe it&#39;s worth<br>
doing something similar for the btree_utils_num.c group, but<br>
I have the impression that those are way simpler.<br></blockquote><div><br>=
0001 LGTM<br>=C2=A0</div><blockquote class=3D"gmail_quote" style=3D"margin:=
0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
v3-0002 is your patch, editorialized a bit.=C2=A0 I left out the<br>
test case because it seemed fairly expensive for something<br>
that won&#39;t expose a bug in normal builds.=C2=A0 (BTW, I wonder<br>
if rather than documenting the comparisons as being reversed,<br>
we should just flip them all.)<br></blockquote><div><br></div>I&#39;d lean =
towards flipping them for readability, though I&#39;m not sure<br><div><spa=
n style=3D"background-color:transparent">it&#39;s worth the backpatch churn=
? I guess we could start a new=C2=A0</span></div><div>thread for master on =
this.<br><br></div><blockquote class=3D"gmail_quote" style=3D"margin:0px 0p=
x 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
v3-0003 fixes a bug I identified: index builds that use the<br>
gbt_bit_ssup_cmp infrastructure will sort the entries in a<br>
way that doesn&#39;t match the bit types&#39; actual ordering,<br>
leading to what&#39;s probably a very inefficient index.</blockquote><div><=
br></div><div>It looks right to me, leaf entries are real bit/varbit values=
, so bitcmp</div><div><span style=3D"background-color:transparent">is the r=
ight comparator there.=C2=A0</span></div><div>=C2=A0</div><blockquote class=
=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rg=
b(204,204,204);padding-left:1ex">
v3-0004 and v3-0005 clean up some things that seem like<br>
dead code to me.<br></blockquote><div><br>One small thing on 0005: once gbt=
_var_node_cp_len stops using<br>pg_mblen_range, the &quot;#include &quot;mb=
/pg_wchar.h&quot;&quot; in btree_utils_var.c<br>looks like it becomes unuse=
d.=C2=A0 Might be worth dropping it too.<br>=C2=A0</div><blockquote class=
=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rg=
b(204,204,204);padding-left:1ex">
Also, it occurs to me after another look at 0002 that what was<br>
perhaps intended was something like this for the non-leaf case:<br>
<br>
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (is_leaf)<br>
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 retval =3D !(tinfo-=
&gt;f_eq(query, key-&gt;lower, collation, flinfo));<br>
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 else<br>
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 retval =3D tinfo-&g=
t;trnc ||<br>
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 !(tin=
fo-&gt;f_cmp(query, key-&gt;lower, collation, flinfo) =3D=3D 0 &amp;&amp;<b=
r>
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 tinfo-&gt;f_cmp(query, key-&gt;upper, collation, flinfo) =3D=3D 0);<br>
<br>
That is, if we&#39;re looking at non-truncated keys and lower =3D=3D upper,=
<br>
then we know that all the keys below this node are exactly that<br>
value, so we can avoid descending if that value is equal to the query.<br>
Most of the time this would not pay off in any savings, but if you<br>
had an index on a column with only a few values, maybe there would be<br>
leaf pages like that?=C2=A0 I would think that the planner would avoid<br>
choosing to implement a &lt;&gt; query with an indexscan unless there was<b=
r>
a pretty large fraction of the table that &lt;&gt; would reject, so maybe<b=
r>
this actually is worth doing.<br></blockquote><div><br></div>I think that o=
ptimization is correct.=C2=A0 For the non-truncatable types<br>the internal=
 lower/upper are exact min/max (built via f_cmp in<br>gbt_var_bin_union wit=
h no truncation), so if both equal the query<br><div><span style=3D"backgro=
und-color:transparent">then everything below does too, and pruning can&#39;=
t drop a match.</span><br>Are you planning on including it in the backpatch=
 or to keep the=C2=A0</div><div>optimization just part of master, and the b=
ug fix backpatched?<br><br>Regards,<br>Ayush=C2=A0</div><div><br>=C2=A0</di=
v></div></div>
</div>

--0000000000000fd1d00655b0e2ef--