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.
This thread has been committed, so CI has stopped here. Anything below is the last result it produced.
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 v12 (message #12), September 01, 2026 at 06:14 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_12 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_12 && git checkout t253539_12Patchset v12 (message #12) is on t253539_12
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/
On Wed, 26 Aug 2026 at 00:33, Daniel Gustafsson <daniel@yesql.se> wrote:
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.
I do see a few places where we do check for PG_INT32_MIN instead of
using pg_neg_s32_overflow(). The example in [1]https://godbolt.org/z/sfxY847E4 does end up with less
code as a result of using pg_neg_s32_overflow(), so it might be worth
removing all applicable examples that use the other method from master
as a follow-up. It'd be worth verifying it doesn't make anything worse
for build systems that don't have __builtin_sub_overflow().
David
Hi David,
Thanks for the review!
On Wed, Aug 26, 2026 at 8:55 PM David Rowley <dgrowleyml@gmail.com> wrote:
On Wed, 26 Aug 2026 at 00:33, Daniel Gustafsson <daniel@yesql.se> wrote:
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.
I do see a few places where we do check for PG_INT32_MIN instead of
using pg_neg_s32_overflow(). The example in [1] does end up with less
code as a result of using pg_neg_s32_overflow(), so it might be worth
removing all applicable examples that use the other method from master
as a follow-up. It'd be worth verifying it doesn't make anything worse
for build systems that don't have __builtin_sub_overflow().
I verified that converting them is safe on platforms without
__builtin_sub_overflow() -- there, pg_neg_s{16,32,64}_overflow() inline to
exactly the same "if (a == PG_INTnn_MIN) ...; *result = -a;" code that is
being replaced, so the generated code is unchanged (I confirmed the
fallback path is bit-for-bit equivalent to the manual form, including at
the INT_MIN boundary).
Rather than pile that cleanup onto this bug-fix thread, I'll send it as a
separate follow-up so this fix can be considered on its own. Will post
that shortly and CC you three.
David
--
Regards,
Ewan Young
On Thu, 27 Aug 2026 at 14:27, Ewan Young <kdbase.hack@gmail.com> wrote:
Rather than pile that cleanup onto this bug-fix thread, I'll send it as a
separate follow-up so this fix can be considered on its own. Will post
that shortly and CC you three.
Yes, another thread. Please also include a description of your method
for verifying the code matches for non-HAVE__BUILTIN_OP_OVERFLOW
builds.
Also, pg_neg_s32_overflow() was introduced in 2024 by 0a27c3d0f733,
which is v18+. Depending on Daniel's patience for backpatching two
different versions, it might be worth just using the == PG_INT32_MIN
method to fix the bug, then allowing the follow-up patch to switch
that in v20 only. That would mean v18 and v19 don't get the most
optimal fix, but it's hard to imagine that's going to be performance
critical.
David
On Thu, Aug 27, 2026 at 10:45 AM David Rowley <dgrowleyml@gmail.com> wrote:
On Thu, 27 Aug 2026 at 14:27, Ewan Young <kdbase.hack@gmail.com> wrote:
Rather than pile that cleanup onto this bug-fix thread, I'll send it as a
separate follow-up so this fix can be considered on its own. Will post
that shortly and CC you three.Yes, another thread. Please also include a description of your method
for verifying the code matches for non-HAVE__BUILTIN_OP_OVERFLOW
builds.Also, pg_neg_s32_overflow() was introduced in 2024 by 0a27c3d0f733,
which is v18+. Depending on Daniel's patience for backpatching two
different versions, it might be worth just using the == PG_INT32_MIN
method to fix the bug, then allowing the follow-up patch to switch
that in v20 only. That would mean v18 and v19 don't get the most
optimal fix, but it's hard to imagine that's going to be performance
critical.
Thanks, that plan makes sense to me -- let's keep the bug fix uniform
across all branches and do the pg_neg_s32_overflow() switch separately.
pg_neg_s32_overflow() only exists from v18 (0a27c3d0f733), while the bug
goes all the way back, so using it in the fix would mean carrying two
versions for the back-branches. Not worth it for a one-liner.
Attached v4 goes back to the explicit PG_INT32_MIN test:
if (unlikely(n == PG_INT32_MIN))
n = PG_INT32_MAX;
else
n = -n;
which applies cleanly to every supported branch (I checked REL_14 through
REL_19 and master). The fix and the regression case are otherwise
unchanged.
I'll take the pg_neg_s32_overflow() cleanup to its own thread, master
only, covering the other open-coded negation checks Ilmari pointed at
(int2/4/8 um/abs/div, lcm, cash, numericvar_to_int64); once this fix
lands I'll fold text_right() into that too. So the back-branches keep the
plain form and only master gets the helper.
David
--
Regards,
Ewan Young
Attachments:
t253539_12v4-0001-Fix-right-with-the-most-negative-integer.patchapplication/octet-stream; name=v4-0001-Fix-right-with-the-most-negative-integer.patchDownload+22-2
On 27 Aug 2026, at 07:17, Ewan Young <kdbase.hack@gmail.com> wrote:
On Thu, Aug 27, 2026 at 10:45 AM David Rowley <dgrowleyml@gmail.com> wrote:
Also, pg_neg_s32_overflow() was introduced in 2024 by 0a27c3d0f733,
which is v18+. Depending on Daniel's patience for backpatching two
different versions, it might be worth just using the == PG_INT32_MIN
method to fix the bug, then allowing the follow-up patch to switch
that in v20 only. That would mean v18 and v19 don't get the most
optimal fix, but it's hard to imagine that's going to be performance
critical.Thanks, that plan makes sense to me -- let's keep the bug fix uniform
across all branches and do the pg_neg_s32_overflow() switch separately.
FWIW, I agree with this plan.
Attached v4 goes back to the explicit PG_INT32_MIN test:
if (unlikely(n == PG_INT32_MIN))
n = PG_INT32_MAX;
else
n = -n;which applies cleanly to every supported branch (I checked REL_14 through
REL_19 and master). The fix and the regression case are otherwise
unchanged.
I will prepare a backpatch all the way of this, and will ping the new thread
when done. (There is a lof work being done for shipping v19 so have a little
patience.)
I'll take the pg_neg_s32_overflow() cleanup to its own thread, master
only, covering the other open-coded negation checks Ilmari pointed at
(int2/4/8 um/abs/div, lcm, cash, numericvar_to_int64); once this fix
lands I'll fold text_right() into that too. So the back-branches keep the
plain form and only master gets the helper.
+1
--
Daniel Gustafsson
On 27 Aug 2026, at 13:13, Daniel Gustafsson <daniel@yesql.se> wrote:
On 27 Aug 2026, at 07:17, Ewan Young <kdbase.hack@gmail.com> wrote:
On Thu, Aug 27, 2026 at 10:45 AM David Rowley <dgrowleyml@gmail.com> wrote:
Attached v4 goes back to the explicit PG_INT32_MIN test:
if (unlikely(n == PG_INT32_MIN))
n = PG_INT32_MAX;
else
n = -n;which applies cleanly to every supported branch (I checked REL_14 through
REL_19 and master). The fix and the regression case are otherwise
unchanged.I will prepare a backpatch all the way of this, and will ping the new thread
when done. (There is a lof work being done for shipping v19 so have a little
patience.)
Finally got around to backpatching this to all supported branches. Thanks for
the submission!
--
Daniel Gustafsson