Re: right() returns the whole string for the most negative n

Hi

On Wed, Aug 26, 2026 at 1:58 PM Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> wrote:
>
>
>
> > On Aug 26, 2026, at 09:52, Ewan Young <kdbase(dot)hack(at)gmail(dot)com> wrote:
> >
> > On Tue, Aug 25, 2026 at 8:18 PM Dagfinn Ilmari Mannsåker
> > <ilmari(at)ilmari(dot)org> wrote:
> >>
> >> Ewan Young <kdbase(dot)hack(at)gmail(dot)com> writes:
> >>
> >>> diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.c
> >>> index a09a9e5d5bb..3117069cf1a 100644
> >>> --- a/src/backend/utils/adt/varlena.c
> >>> +++ b/src/backend/utils/adt/varlena.c
> >>> @@ -4714,7 +4714,17 @@ text_right(PG_FUNCTION_ARGS)
> >>> int off;
> >>>
> >>> if (n < 0)
> >>> - n = -n;
> >>> + {
> >>> + /*
> >>> + * Negating PG_INT32_MIN would overflow, so clamp instead. Any n whose
> >>> + * absolute value is at least the string's length skips the whole
> >>> + * string, and len can't exceed PG_INT32_MAX, so this is equivalent.
> >>> + */
> >>> + if (unlikely(n == PG_INT32_MIN))
> >>> + n = PG_INT32_MAX;
> >>> + else
> >>> + n = -n;
> >>> + }
> >>
> >> Instead of open-coding this, how about about using pg_neg_s32_overflow?
> >>
> >> if (pg_neg_s32_overflow(n, &n))
> >> n = PG_INT32_MAX;
> >>
> >
> > Much nicer, thanks - done in v2. varlena.c already includes common/int.h,
> > so no new header was needed.
> >
> >> This made me think we might want saturating versions of the
> >> pg_*_overflow functions, but some quick grepping doesn't reveal any
> >> other places using pg_*_overflow do it manually, so that feels like
> >> premature generalisation.
> >
> > Agreed, I left it as the two-liner.
> >
> > Behaviour and tests are unchanged from v1: right('abcdef', INT32_MIN) now
> > returns '', the adjacent values and left() are untouched, and make check
> > passes.
> >
> >>
> >> - ilmari
> >
> >
> >
> > --
> > Regards,
> > Ewan Young
> > <v2-0001-Fix-right-with-the-most-negative-integer.patch>
>
> ```
> + /*
> + * Negating PG_INT32_MIN would overflow, so clamp instead. Any n whose
> + * absolute value is at least the string's length skips the whole
> + * string, and len can't exceed PG_INT32_MAX, so this is equivalent.
> + */
> + if (pg_neg_s32_overflow(n, &n))
> + n = PG_INT32_MAX;
> ```
>
> I think using pg_neg_s32_overflow() is clearer. Shall we also update the comment, since PG_INT32_MIN is no longer explicitly referenced in this code?

Good point — done. Reworded the comment in v3 to describe the overflow
case generically; no other changes from v2. Patch attached.

Thanks for the review.

>
> Best regards,
> --
> Chao Li (Evan)
> HighGo Software Co., Ltd.
> https://www.highgo.com/
>
>
>
>

--
Regards,
Ewan Young