Thread: Re: BUG #17946: LC_MONETARY & DO LANGUAGE plperl - BUG
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
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
Joe, The Reply-To header in your email is pointing at joe@cd, fyi. Pretty strange. -- Tristan Partin Neon (https://neon.tech)
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
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)
Someday I will learn... Attached is the v2. -- Tristan Partin Neon (https://neon.tech)
Attachment
Here is an up to date patch given some churn on the master branch. -- Tristan Partin Neon (https://neon.tech)