| From: | Ewan Young <kdbase(dot)hack(at)gmail(dot)com> |
|---|---|
| To: | David Rowley <dgrowleyml(at)gmail(dot)com> |
| Cc: | Daniel Gustafsson <daniel(at)yesql(dot)se>, Dagfinn Ilmari Mannsåker <ilmari(at)ilmari(dot)org>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Andrey Rachitskiy <pl0h0yp1(at)gmail(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
| Subject: | Re: right() returns the whole string for the most negative n |
| Date: | 2026-08-27 05:17:12 |
| Message-ID: | CAON2xHNFX75fd01CwwSpFqP_o4ULNz-NzWaH4VXhLVb9=+3AQQ@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
On Thu, Aug 27, 2026 at 10:45 AM David Rowley <dgrowleyml(at)gmail(dot)com> wrote:
>
> On Thu, 27 Aug 2026 at 14:27, Ewan Young <kdbase(dot)hack(at)gmail(dot)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
| Attachment | Content-Type | Size |
|---|---|---|
| v4-0001-Fix-right-with-the-most-negative-integer.patch | application/octet-stream | 3.5 KB |
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Bertrand Drouvot | 2026-08-27 05:21:16 | Re: pgstat: Flush some statistics within running transactions, take 2 |
| Previous Message | vignesh C | 2026-08-27 05:11:26 | Re: Proposal: Conflict log history table for Logical Replication |