MIME-Version: 1.0
References: 
 <AI6AnABgKW93Qbx1jVzi84r9.8.1781322625756.Hmail.3020001251@tju.edu.cn>
 <CAJTYsWUurV6dBhnyd6R7W1w7Fy6FiG9-F_Swk_GDXQQpHoiciA@mail.gmail.com>
 <a4a639c9-01bc-4d58-b942-32ada123feae@iki.fi>
In-Reply-To: <a4a639c9-01bc-4d58-b942-32ada123feae@iki.fi>
From: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Date: Mon, 15 Jun 2026 20:54:56 +0530
Message-ID: 
 <CAJTYsWXhViez1AjDHeTUux-XoE2fUwJyt=Vzm4T2=BDnxvKo6w@mail.gmail.com>
Subject: Re: Fw:Re: Fw: ltree_compare in contrib/ltree/ltree_op.c overflows
 int32 on deep ltree comparisons, returning the wrong sign
To: Heikki Linnakangas <hlinnaka@iki.fi>
Cc: =?UTF-8?B?546L6LeD5p6X?= <violin0613@tju.edu.cn>,
	pgsql-bugs@postgresql.org
Content-Type: multipart/alternative; boundary="0000000000007b93f306544c6ec3"
Archived-At: 
 <https://www.postgresql.org/message-id/CAJTYsWXhViez1AjDHeTUux-XoE2fUwJyt%3DVzm4T2%3DBDnxvKo6w%40mail.gmail.com>
Precedence: bulk

--0000000000007b93f306544c6ec3
Content-Type: text/plain; charset="UTF-8"

Hi,

On Mon, 15 Jun 2026 at 20:38, Heikki Linnakangas <hlinnaka@iki.fi> wrote:

> On 13/06/2026 09:12, Ayush Tiwari wrote:
> > This looks like a classic case of integer overflow that's
> > happening in ltree_compare function in ltree_op.c.
> >
> > return (al->len - bl->len) * 10 * (an + 1);
> > return res * 10 * (an + 1);
> > return (a->numlevel - b->numlevel) * 10 * (an + 1);
> >
> > I think the calculation should be done as int64, something of this sort:
>
> Yeah, that works. However, I note that the multiplication is only really
> needed by the ltree_penalty() caller. All the other callers just check
> if the return value is less than, equal, or greater than zero. It feels
> a little silly to do all that work of multiplication and clamping for
> those callers. And for ltree_penalty(), the caller actually converts the
> return value to a float, so clamping it to int32 range feels a little
> silly for that too. So I propose the attached, which splits the
> ltree_compare() function into two variants: one for ltree_penalty() that
> returns a float, and one for others that don't care about the
> "magnitude". It duplicates a little code, but I think it's easier to
> reason about. What do you think?
>

I had thought initially of using the method you have added,
(not with float return-type though, I thought of planning to create
a duplicate function with int64 type just for ltree_penalty(), but float
type
is better) noting the same thing that rest callers just care about
comparison with 0.

But thought backpatching it would be harder, hence I used the int64
method. However, your patch looks much better than the ugly
behaviour and I agree it did not make sense to do those
multiplications at all other comparison functions.

Regards,
Ayush

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

<div dir=3D"ltr"><div dir=3D"ltr">Hi,</div><br><div class=3D"gmail_quote gm=
ail_quote_container"><div dir=3D"ltr" class=3D"gmail_attr">On Mon, 15 Jun 2=
026 at 20:38, Heikki Linnakangas &lt;<a href=3D"mailto:hlinnaka@iki.fi">hli=
nnaka@iki.fi</a>&gt; wrote:<br></div><blockquote class=3D"gmail_quote" styl=
e=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);paddin=
g-left:1ex">On 13/06/2026 09:12, Ayush Tiwari wrote:<br>
&gt; This looks like a classic case of integer overflow that&#39;s<br>
&gt; happening in ltree_compare function in ltree_op.c.<br>
&gt; <br>
&gt; return (al-&gt;len - bl-&gt;len) * 10 * (an + 1);<br>
&gt; return res * 10 * (an + 1);<br>
&gt; return (a-&gt;numlevel - b-&gt;numlevel) * 10 * (an + 1);<br>
&gt; <br>
&gt; I think the calculation should be done as int64, something of this sor=
t:<br>
<br>
Yeah, that works. However, I note that the multiplication is only really <b=
r>
needed by the ltree_penalty() caller. All the other callers just check <br>
if the return value is less than, equal, or greater than zero. It feels <br=
>
a little silly to do all that work of multiplication and clamping for <br>
those callers. And for ltree_penalty(), the caller actually converts the <b=
r>
return value to a float, so clamping it to int32 range feels a little <br>
silly for that too. So I propose the attached, which splits the <br>
ltree_compare() function into two variants: one for ltree_penalty() that <b=
r>
returns a float, and one for others that don&#39;t care about the <br>
&quot;magnitude&quot;. It duplicates a little code, but I think it&#39;s ea=
sier to <br>
reason about. What do you think?<br></blockquote><div><br>I had thought ini=
tially of using the method you have added,<br>(not with float return-type t=
hough, I thought of planning to create<br>a duplicate function with int64 t=
ype just for ltree_penalty(), but float type<br>is better) noting the same =
thing that rest callers just care about <br>comparison with 0.<br><br>But t=
hought backpatching it would be harder, hence I used the int64 <br>method. =
However, your patch looks much better than the ugly <br>behaviour and I agr=
ee it did not make sense to do those<br>multiplications at all other compar=
ison functions.</div><div><br>Regards,<br>Ayush</div></div></div>

--0000000000007b93f306544c6ec3--