> On Aug 26, 2026, at 14:29, Ewan Young <[email protected]> wrote:
>
> Hi
>
> On Wed, Aug 26, 2026 at 1:58 PM Chao Li <[email protected]> wrote:
>>
>>
>>
>>> 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?
>
> 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.
>
> --
> Regards,
> Ewan Young
> <v3-0001-Fix-right-with-the-most-negative-integer.patch>
Thanks for updating the patch. V3 LGTM.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
