Re: [BUGS] BUG #4186: set lc_messages does not work

Hiroshi Inoue wrote:
> Hiroshi Inoue wrote:
>> Hi,
>>
>> I posted a patch 18 days ago but have got no responce.
>> Anyway I've simplified the patch by using an appropriate
>> gettext module.
>>
>> Hiroshi Inoue wrote:
>>> Bruce Momjian wrote:
>>>> Tom Lane wrote:
>>>>> Magnus Hagander <magnus(at)hagander(dot)net> writes:
>>>>>> Thomas H. wrote:
>>>>>>> so at least that explains the "changed" behaviour. nevertheless,
>>>>>>> LC_MESSAGES seems to be defunct - with the "locale" folder present,
>>>>>>> pg always picks the os' language and ignores the lc_message value.
>>>>>> This looks like I can reproduce though, at least on cvs head. Did
>>>>>> this
>>>>>> work for you in previous versions?
>>>>> Maybe we were using a different build of gettext in the previous
>>>>> releases, one that didn't look at the same info as the current code?
>>>>>
>>>>
>>>> Where are we on this?
>>
>> AFAICS there are 2 causes.
>>
>> 1. MSVC version of postgres is using a bad gettext module.
>> 2. getenv() in mingw cannot see the result of putenv() in MSVC8.0.
>>
>> As for 1, we have to use another gettext module. I can provide it
>> if requested.

Yes, I think that'll be needed. Exactly what is wrong and needs to be
changed? (Copying DAve in on this since he builds the MSI)

Is it possible to build this one with the same version of MSVC? If it
is, then that should remove the need for #2, right?

>> As for 2, pg_putenv() or pg_unsetenv() in the attachced patch calls
>> corresponding putenv() whose result can be referenced by getenv() in
>> mingw.

Oh, yuck. This must be because msvcrt.dll (used by mingw) caches the
values in the environment.

That's a rather ugly solution, but I guess if we have no other choice..
Does it make a difference if you try to set the value using
SetEnvironmentVariable()?

It would definitely work if the gettext stuff uses
GetEnvironmentVariable(). I doubt it does though, but it might work
anyway...

>> In addtion the patch provides a functionality to Windows locale
>> name to ISO formatted locale name.
>>
>> Comments ?

I wonder if it's cleaner to use this "load msvcrt version of setenv()"
*always*. Or to cover all bases, perhaps we should always do *both* -
that is, both set in current runtime and msvcrt.dll... We don't do this
in a lot of places today, but we might use more in the future. And as
long as it's not in a performance critical path, doing it twice is
almost for free...

There is already src/port/unsetenv.c, we should probably be building on
top of that one.

As for the code itself, there are some very non-postgresql-standard
constructs in the patch, like:
+ if (result = IsoLocaleName(locale), NULL == result)
+ result = locale;

And things like:
+ return NULL;
break;

is just wrong.

And we don't normally write if (NULL == <something>).

But these are fairly simple to fix, so I can do those at commit-time.

//Magnus