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. > > Best regards, > -- > Chao Li (Evan) > HighGo Software Co., Ltd. > https://www.highgo.com/ > > > > -- Regards, Ewan Young
v3-0001-Fix-right-with-the-most-negative-integer.patch
Description: Binary data
