> On Aug 26, 2026, at 09:52, Ewan Young <[email protected]> wrote:
>
> On Tue, Aug 25, 2026 at 8:18 PM Dagfinn Ilmari Mannsåker
> <[email protected]> wrote:
>>
>> Ewan Young <[email protected]> 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?
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
