right() returns the whole string for the most negative n
Hackorum builds and tests every patch posted to the lists, not only commitfest submissions. This is Hackorum's own CI rather than the PostgreSQL project's, and it is still under testing - please report anything that looks wrong.
You can run a PostgreSQL built from this patch straight from Docker, with no checkout and no build:
docker run --rm -p 5432:5432 ghcr.io/hackorum-dev/postgres-patch:t253539psql -h localhost -U postgresBuilt from patchset v7 (message #7), August 26, 2026 at 06:51 AM.
Every patchset is also pushed to a branch of our PostgreSQL fork, so you can check out the same tree CI built. Without a PostgreSQL checkout:
git clone --branch t253539_7 https://github.com/hackorum-dev/postgres.gitIn a checkout you already have, add the fork once:
git remote add hackorum https://github.com/hackorum-dev/postgres.gitthen, for this patchset and every later one:
git fetch hackorum t253539_7 && git checkout t253539_7Patchset v7 (message #7) is on t253539_7
Hi Hackers,
right(text, int) gets one input wrong:
SELECT right('abcdef', (-2147483648)::int4); -- 'abcdef'
SELECT right('abcdef', -2147483647); -- '' (correct)
SELECT right('abcdef', -3); -- 'def' (correct)
A negative n means "return all but the first |n| characters", so |n| that
big has to give an empty string. Only this one value is affected, and
left() is fine.
text_right() negates n to get the number of characters to skip:
if (n < 0)
n = -n;
else
n = pg_mbstrlen_with_len(p, len) - n;
off = pg_mbcharcliplen(p, len, n);
Negating PG_INT32_MIN overflows; under -fwrapv it comes back as
PG_INT32_MIN, still negative, and pg_mbcharcliplen() returns 0 for any
negative limit, so off is 0 and the whole string is returned.
The attached patch clamps to PG_INT32_MAX rather than negating. Any n
whose absolute value reaches the string's length skips all of it, and a
text value can't be longer than PG_INT32_MAX, so the answer is unchanged
for every other input. I did not make it an error, unlike
text_format_string_conversion() a few hundred lines down, which rejects a
width of INT_MIN (73e7025bd8e, complete with a "-INT_MIN is undefined"
comment): a format width has no sensible clamp, whereas an oversized skip
count does.
left() is not affected because its negative case adds n to the character
length instead of negating it, and since the length is non-negative and
bounded by the varlena size limit, that sum cannot overflow.
This dates to 49b27ab5514, which added left()/right() in 2010, and the
line has not been touched since; I could not find a previous report. It's
the same shape as b4dfae2ffac (money, INT64_MIN / -1) from a few weeks
ago.
The patch adds the case to the existing left()/right() test in text.sql,
which currently covers only generate_series(-5, 5). make check passes;
before adding the expected output I ran the suite deliberately and the
only difference was the new line.
--
Regards,
Ewan Young
On 25 Aug 2026, at 08:57, Ewan Young <kdbase.hack@gmail.com> wrote:
Negating PG_INT32_MIN overflows; under -fwrapv it comes back as
PG_INT32_MIN, still negative, and pg_mbcharcliplen() returns 0 for any
negative limit, so off is 0 and the whole string is returned.
Thanks for the report and patch, the fix seems correct to me.
This dates to 49b27ab5514, which added left()/right() in 2010, and the
line has not been touched since; I could not find a previous report.
Understandably so, it's not an issue likely to cause issues in production.
It's the same shape as b4dfae2ffac (money, INT64_MIN / -1) from a few weeks
ago.
Agreed, so backpatching all the way for this one as well makes sense. I'll
have another look and will leave it open to input from others before applying.
--
Daniel Gustafsson
Ewan Young <kdbase.hack@gmail.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;
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.
- ilmari
On 25 Aug 2026, at 14:18, Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> wrote:
Instead of open-coding this, how about about using pg_neg_s32_overflow?
if (pg_neg_s32_overflow(n, &n))
n = PG_INT32_MAX;
Yes, that's indeed a good idea.
--
Daniel Gustafsson
On Tue, Aug 25, 2026 at 8:18 PM Dagfinn Ilmari Mannsåker
<ilmari@ilmari.org> wrote:
Ewan Young <kdbase.hack@gmail.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
On Aug 26, 2026, at 09:52, Ewan Young <kdbase.hack@gmail.com> wrote:
On Tue, Aug 25, 2026 at 8:18 PM Dagfinn Ilmari Mannsåker
<ilmari@ilmari.org> wrote:Ewan Young <kdbase.hack@gmail.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?
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
Hi
On Wed, Aug 26, 2026 at 1:58 PM Chao Li <li.evan.chao@gmail.com> wrote:
On Aug 26, 2026, at 09:52, Ewan Young <kdbase.hack@gmail.com> wrote:
On Tue, Aug 25, 2026 at 8:18 PM Dagfinn Ilmari Mannsåker
<ilmari@ilmari.org> wrote:Ewan Young <kdbase.hack@gmail.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
On Aug 26, 2026, at 14:29, Ewan Young <kdbase.hack@gmail.com> wrote:
Hi
On Wed, Aug 26, 2026 at 1:58 PM Chao Li <li.evan.chao@gmail.com> wrote:
On Aug 26, 2026, at 09:52, Ewan Young <kdbase.hack@gmail.com> wrote:
On Tue, Aug 25, 2026 at 8:18 PM Dagfinn Ilmari Mannsåker
<ilmari@ilmari.org> wrote:Ewan Young <kdbase.hack@gmail.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.
--
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/