Re: BUG #17946: LC_MONETARY & DO LANGUAGE plperl - BUG

From: "Tristan Partin" <tristan(at)neon(dot)tech>
To: <joe(at)cd>, "Heikki Linnakangas" <hlinnaka(at)iki(dot)fi>, "Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us>, <gdo(at)leader(dot)it>
Cc: "PostgreSQL-development" <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: BUG #17946: LC_MONETARY & DO LANGUAGE plperl - BUG
Date: 2023-07-03 14:42:58
Message-ID: CTSM7EZZMY12.1F7GV7Y2KPBBD@gonk
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-bugs pgsql-hackers

On Fri Jun 30, 2023 at 7:13 AM CDT, Joe Conway wrote:
> On 6/29/23 22:13, Tristan Partin wrote:
> > On Mon Jun 5, 2023 at 11:00 AM CDT, Heikki Linnakangas wrote:
> >> I think the uselocale() call renders ineffective the setlocale() calls
> >> that we make later. Maybe we should replace our setlocale() calls with
> >> uselocale(), too.
> >
> > For what it's worth to everyone else in the thread (especially Joe), I
> > have a patch locally that fixes the mentioned bug using uselocale(). I
> > am not sure that it is worth committing for v16 given how _large_ (the
> > patch is actually quite small, +216 -235) of a change it is. I am going
> > to spend tomorrow combing over it a bit more and evaluating other
> > setlocale uses in the codebase.
>
> (moving thread to hackers)
>
> I don't see a patch attached -- how is it different than what I posted a
> week ago and added to the commitfest here?
>
> https://commitfest.postgresql.org/43/4413/
>
> FWIW, if you are proposing replacing all uses of setlocale() with
> uselocale() as Heikki suggested:
>
> 1/ I don't think that is pg16 material, and almost certainly not
> back-patchable to earlier.

I am in agreement.

> 2/ It probably does not solve all of the identified issues caused by the
> newer perl libraries by itself, i.e. I believe the patch posted to the
> CF is still needed.

Perhaps. I do think your patch is still valuable regardless. Works for
backpatching and is just good defensive programming. I have added myself
as a reviewer.

> 3/ I believe it is probably the right way to go for pg17+, but I would
> love to hear opinions from Jeff Davis, Peter Eisentraut, and/or Thomas
> Munroe (the locale code "usual suspects" ;-)), and others, about that.

Thanks for your patience. Attached is a patch that should cover all the
problematic use cases of setlocale(). There are some setlocale() calls in
tests, initdb, and ecpg left. I plan to get to ecpglib before the final
version of this patch after I abstract over Windows not having
uselocale(). I think leaving initdb and tests as is would be fine, but I
am also happy to just permanently purge setlocale() from the codebase
if people see value in that. We could also poison[0] setlocale() at that
point.

[0]: https://gcc.gnu.org/onlinedocs/cpp/Pragmas.html

--
Tristan Partin
Neon (https://neon.tech)

Attachment Content-Type Size
v1-0001-Skip-checking-for-uselocale-on-Windows.patch text/x-patch 677 bytes
v1-0002-Add-locale_is_c-function.patch text/x-patch 4.5 KB
v1-0003-Use-uselocale-instead-of-setlocale.patch text/x-patch 26.7 KB

In response to

Responses

Browse pgsql-bugs by date

  From Date Subject
Next Message Pantelis Theodosiou 2023-07-03 14:57:52 Re: group by can use alias from select list but not the having clause
Previous Message Tom Lane 2023-07-03 14:12:11 Re: group by can use alias from select list but not the having clause

Browse pgsql-hackers by date

  From Date Subject
Next Message Tristan Partin 2023-07-03 14:49:21 Re: Make uselocale protection more consistent
Previous Message Peter Eisentraut 2023-07-03 14:26:24 Re: Optionally using a better backtrace library?