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

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

From
Joe Conway
Date:
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.

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.

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.

-- 
Joe Conway
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com


Attachment

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

From
"Tristan Partin"
Date:
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

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

From
"Tristan Partin"
Date:
Joe,

The Reply-To header in your email is pointing at joe@cd, fyi. Pretty
strange.

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



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

From
Joe Conway
Date:
On 7/3/23 12:17, Tristan Partin wrote:
> The Reply-To header in your email is pointing at joe@cd, fyi. Pretty
> strange.


I noticed that -- it happened only the one time, and I am not sure why. 
Seems fine now though.

-- 
Joe Conway
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com




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

From
"Tristan Partin"
Date:
On Mon Jul 3, 2023 at 9:42 AM CDT, Tristan Partin wrote:
> 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

Here is a v2 with best effort Windows support. My patch currently
assumes that you either have uselocale() or are Windows. I dropped the
environment variable hacks, but could bring them back if we didn't like
this requirement.

I tried to add an email[0] to discuss this with hackers, but failed to add
the CC. Let's discuss here instead given my complete inability to manage
mailing lists :).

[0]: https://www.postgresql.org/message-id/CTUJ604ZWHI1.3PFZK152XCWLX%40gonk

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



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

From
"Tristan Partin"
Date:
Someday I will learn...

Attached is the v2.

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

Attachment

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

From
"Tristan Partin"
Date:
Here is an up to date patch given some churn on the master branch.

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

Attachment