Обсуждение: PG compilation error with Visual Studio 2015/2017/2019

On Mon, Apr 6, 2020 at 4:38 PM davinder singh
<davindersingh2692@gmail.com> wrote:
>
> Hi All,
>
> I am working on “pg_locale compilation error with Visual Studio 2017”, Related threads[1],[2].
> We are getting compilation error in static char *IsoLocaleName(const char *winlocname) function in pg_locale.c file.
Thisfunction is trying to convert the locale name into the Unix style. For example, it will change the locale name
"en-US"into "en_US". 
> It is creating a locale using _locale_t _create_locale( int category, const char *locale) and then trying to access
thename of that locale by pointing to internal elements of the structure loct->locinfo->locale_name[LC_CTYPE] but it
hasbeen missing from the _locale_t since VS2015 which is causing the compilation error. I found a few useful APIs that
canbe used here. 
>
> ResolveLocaleName and GetLocaleInfoEx both can take locale in the following format.
> <language>-<REGION>
> <language>-<Script>-<REGION>
>
> ResolveLocaleName will try to find the closest matching locale name to the input locale name even if the input locale
nameis invalid given that the <language> is correct. 
>
> en-something-YZ     => en-US
> ex-US                      => error
> Aa-aaaa-aa             => aa-ET   represents (Afar,Ethiopia)
> Aa-aaa-aa               => aa-ET
>
> Refer [4] for more details
>
> But in the case of GetLocaleInfoEx, it will only check the format of the input locale, as mentioned before, if
correctit will return the name of the locale otherwise it will return an error. 
> For example.
>
> en-something-YZ     => error
> ex-US                       => ex-US
> aa-aaaa-aa              => aa-Aaaa-AA
> aa-aaa-aa                => error.
>
> Refer [5] for more details.
>
> Currently, it is using _create_locale it behaves similarly to GetLocaleInfoEx i.e. it also only checks the format
onlydifference is for a bigger set. 
> I thought of using GetLocaleInfoEx for the fix because it behaves similarly to the already existing and a similar
issuewas resolved earlier using the same. 
>

It seems the right direction to use GetLocaleInfoEx as we have already
used it to handle a similar problem (lc_codepage is missing in
_locale_t in higher versions of MSVC (cf commit 0fb54de9aa)) in
chklocale.c.  However, I see that we have added a manual parsing there
if GetLocaleInfoEx doesn't parse it.  I think that addresses your
concern for _create_locale handling bigger sets.  Don't we need
something equivalent here for the cases which GetLocaleInfoEx doesn't
support?

How have you ensured the testing of this code?  I see that we have
src\test\locale in our test directory.  Can we test using that?

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Tue, Apr 7, 2020 at 8:30 PM Juan José Santamaría Flecha
<juanjo.santamaria@gmail.com> wrote:
>
> * I think you could save a couple of code lines, and make it clearer, by merging both tests on  _MSC_VER  into a
single#if... #else and leaving as common code: 
> + }
> + else
> + return NULL;
> +#endif /*_MSC_VER && _MSC_VER < 1900*/
>
> * The logic on "defined(_MSC_VER) && (_MSC_VER >= 1900)" is defined as "_WIN32_WINNT >= 0x0600" on other parts of the
code.I would recommend using the later. 
>

I see that we have used _MSC_VER form of checks in win32_langinfo
(chklocale.c) for a similar kind of handling.  So, isn't it better to
be consistent with that?  Which exact part of the code you are
referring to?

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Fri, Apr 10, 2020 at 5:35 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, Apr 7, 2020 at 8:30 PM Juan José Santamaría Flecha
> <juanjo.santamaria@gmail.com> wrote:
> >
> > * I think you could save a couple of code lines, and make it clearer, by merging both tests on  _MSC_VER  into a
single#if... #else and leaving as common code: 
> > + }
> > + else
> > + return NULL;
> > +#endif /*_MSC_VER && _MSC_VER < 1900*/
> >
> > * The logic on "defined(_MSC_VER) && (_MSC_VER >= 1900)" is defined as "_WIN32_WINNT >= 0x0600" on other parts of
thecode. I would recommend using the later. 
> >
>
> I see that we have used _MSC_VER form of checks in win32_langinfo
> (chklocale.c) for a similar kind of handling.  So, isn't it better to
> be consistent with that?  Which exact part of the code you are
> referring to?
>

I see that the kind of check you are talking is recently added by
commit 352f6f2d.  I think it is better to be consistent in all places.
Let's pick one and use that if possible.

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Wed, Apr 15, 2020 at 11:58 PM Juan José Santamaría Flecha
<juanjo.santamaria@gmail.com> wrote:
>
> On Wed, Apr 15, 2020 at 4:46 PM Ranier Vilela <ranier.vf@gmail.com> wrote:
>>
>> Em qua., 15 de abr. de 2020 às 03:08, davinder singh <davindersingh2692@gmail.com> escreveu:
>>>
>>>
>>>> 5. Why call _create_locale if _WIN32_WINNT >= 0x0600 is true and loct is not used?
>>>
>>> _create_locale can take bigger input than GetLocaleInfoEx. But we are interested in
>>> language[_country-region[.code-page]]. We are using _create_locale to validate
>>> the given input. The reason is we can't verify the locale name if it is appended with
>>> code-page by using GetLocaleInfoEx. So before parsing, we verify if the whole input
>>> locale name is valid by using _create_locale. I hope that answers your question.
>>
>> Understood. In this case, _create_locale, is being used only to validate the input.
>> Perhaps, in addition, you could create an additional function, which only validates winlocname, without having to
createstructures or use malloc, to be used when _WIN32_WINNT> = 0x0600 is true, but it is only a suggestion, if you
thinkit is necessary. 
>
>
> Looking at the comments for IsoLocaleName() I see: "MinGW headers declare _create_locale(), but msvcrt.dll lacks that
symbol".This is outdated [1][2], and  _create_locale() could be used from Windows 8, but I think we should use
GetLocaleInfoEx()as a complete alternative to  _create_locale(). 
>

I see some differences in the output when _create_locale() is used vs.
when GetLocaleInfoEx() is used.  Forex.

Set LC_MESSAGES="English_New Zealand";

-- returns en-NZ, then code changes it to en_NZ when _create_locale()
is used whereas GetLocaleInfoEx returns error.

Set LC_MESSAGES="English_Republic of the Philippines";

-- returns en-PH, then code changes it to en_PH when _create_locale()
is used whereas GetLocaleInfoEx returns error.

Set LC_MESSAGES="English_New Zealand";

-- returns en-NZ, then code changes it to en_NZ when _create_locale()
is used whereas GetLocaleInfoEx returns error.

Set LC_MESSAGES="French_Canada";

--returns fr-CA when _create_locale() is used whereas GetLocaleInfoEx
returns error.

The function IsoLocaleName() header comment says "Convert a Windows
setlocale() argument to a Unix-style one", so I was expecting above
cases which gives valid values with _create_locale() should also work
with GetLocaleInfoEx().  If it is fine for GetLocaleInfoEx() to return
an error for the above cases, then we need an explanation of the same
and probably add a few comments as well.  So, I am not sure if we can
conclude that GetLocaleInfoEx() is an alternative to _create_locale()
at this stage.

I have used the attached hack to make _create_locale work on the
latest MSVC.  Just to be clear this is mainly for the purpose of
testing the behavior _create_locale.

On the code side,
+ GetLocaleInfoEx(wc_locale_name, LOCALE_SNAME, (LPWSTR) &buffer,
+ LOCALE_NAME_MAX_LENGTH);

  /* Locale names use only ASCII, any conversion locale suffices. */
- rc = wchar2char(iso_lc_messages, loct->locinfo->locale_name[LC_CTYPE],
- sizeof(iso_lc_messages), NULL);
+ rc = wchar2char(iso_lc_messages, buffer, sizeof(iso_lc_messages), NULL);

Check the return value of GetLocaleInfoEx and if it is successful,
then only use wchar2char, otherwise, this function will return an
empty string ("") instead of NULL.

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Sat, Apr 18, 2020 at 12:14 AM Juan José Santamaría Flecha
<juanjo.santamaria@gmail.com> wrote:
>
>
> We can get a match for those locales in non-ISO format by enumerating available locales with EnumSystemLocalesEx(),
andtrying to find a match. 
>
> Please find a new patch for so.
>

I have not reviewed or tested the new patch but one thing I would like
to see is the impact of setting LC_MESAGGES with different locale
information.  Basically, the error messages after setting the locale
with _create_locale and with the new method being discussed.  This
will help us in ensuring that we didn't break anything which was
working with prior versions of MSVC.  Can you or someone try to test
and share the results of the same?

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Sun, Apr 19, 2020 at 03:59:50PM -0300, Ranier Vilela wrote:
> Em dom., 19 de abr. de 2020 às 12:34, Juan José Santamaría Flecha <
> juanjo.santamaria@gmail.com> escreveu:
>> This line needs a break, other than that LGTM.
>
> Sure. Attached new patch with this revision.

Amit, are you planning to look at this patch?  I may be able to spend
a couple of hours on this thread this week and that's an area of the
code I have worked with in the past, though I am not sure.
--
Michael

On Mon, Apr 20, 2020 at 4:32 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Sun, Apr 19, 2020 at 03:59:50PM -0300, Ranier Vilela wrote:
> > Em dom., 19 de abr. de 2020 às 12:34, Juan José Santamaría Flecha <
> > juanjo.santamaria@gmail.com> escreveu:
> >> This line needs a break, other than that LGTM.
> >
> > Sure. Attached new patch with this revision.
>
> Amit, are you planning to look at this patch?
>

Yes, I am planning to look into it.  Actually, I think the main thing
here is to ensure that we don't break something which was working with
_create_locale API.

>  I may be able to spend
> a couple of hours on this thread this week and that's an area of the
> code I have worked with in the past, though I am not sure.
>

Okay, that will be good.

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Sun, Apr 19, 2020 at 3:46 PM Juan José Santamaría Flecha
<juanjo.santamaria@gmail.com> wrote:
>
>
> On Sat, Apr 18, 2020 at 6:07 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>>
>> On Sat, Apr 18, 2020 at 12:14 AM Juan José Santamaría Flecha
>> <juanjo.santamaria@gmail.com> wrote:
>> >
>> > We can get a match for those locales in non-ISO format by enumerating available locales with
EnumSystemLocalesEx(),and trying to find a match. 
>>
>> I have not reviewed or tested the new patch but one thing I would like
>> to see is the impact of setting LC_MESAGGES with different locale
>> information.  Basically, the error messages after setting the locale
>> with _create_locale and with the new method being discussed.  This
>> will help us in ensuring that we didn't break anything which was
>> working with prior versions of MSVC.  Can you or someone try to test
>> and share the results of the same?
>
>
> I cannot find a single place where all supported locales are listed, but I have created a small test program
(WindowsNLSLocales.c)based on: <language>[_<location>] format locales [1], additional supported language strings [2],
andadditional supported country and region strings [3]. Based on the results from this test program, it is possible to
todo a good job with the <language>[_<location>] types using the proposed logic, but the two later cases are Windows
specific,and there is no way arround a lookup-table. 
>
> The attached results (WindowsNLSLocales.ods) come from Windows 10 (1903) and Visual C++ build 1924, 64-bit.
>

I think these are quite intensive tests but I wonder do we need to
test some locales with code_page?  Generally speaking, in this case it
should not matter as we are trying to get the locale name but no harm
in testing.  Also, I think it would be good if we can test how this
impacts the error messages as Davinder is trying to do.

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Mon, Apr 20, 2020 at 6:32 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Sun, Apr 19, 2020 at 3:46 PM Juan José Santamaría Flecha
> <juanjo.santamaria@gmail.com> wrote:
> >
> > I cannot find a single place where all supported locales are listed, but I have created a small test program
(WindowsNLSLocales.c)based on: <language>[_<location>] format locales [1], additional supported language strings [2],
andadditional supported country and region strings [3]. Based on the results from this test program, it is possible to
todo a good job with the <language>[_<location>] types using the proposed logic, but the two later cases are Windows
specific,and there is no way arround a lookup-table. 
> >
> > The attached results (WindowsNLSLocales.ods) come from Windows 10 (1903) and Visual C++ build 1924, 64-bit.
> >
>
> I think these are quite intensive tests but I wonder do we need to
> test some locales with code_page?  Generally speaking, in this case it
> should not matter as we are trying to get the locale name but no harm
> in testing.  Also, I think it would be good if we can test how this
> impacts the error messages as Davinder is trying to do.
>


I have tried a simple test with the latest patch and it failed for me.

Set LC_MESSAGES="English_United Kingdom";
-- returns en-GB, then code changes it to en_NZ when _create_locale()
is used whereas with the patch it returns "" (empty string).

There seem to be two problems here (a) The input to enum_locales_fn
doesn't seem to get the input name as "English_United Kingdom" due to
which it can't find a match even if the same exists. (b) After
executing EnumSystemLocalesEx, there is no way the patch can detect if
it is successful in finding the passed name due to which it appends
empty string in such cases.

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Tue, Apr 21, 2020 at 12:51 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Mon, Apr 20, 2020 at 6:32 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Sun, Apr 19, 2020 at 3:46 PM Juan José Santamaría Flecha
> > <juanjo.santamaria@gmail.com> wrote:
> > >
> > > I cannot find a single place where all supported locales are listed, but I have created a small test program
(WindowsNLSLocales.c)based on: <language>[_<location>] format locales [1], additional supported language strings [2],
andadditional supported country and region strings [3]. Based on the results from this test program, it is possible to
todo a good job with the <language>[_<location>] types using the proposed logic, but the two later cases are Windows
specific,and there is no way arround a lookup-table. 
> > >
> > > The attached results (WindowsNLSLocales.ods) come from Windows 10 (1903) and Visual C++ build 1924, 64-bit.
> > >
> >
> > I think these are quite intensive tests but I wonder do we need to
> > test some locales with code_page?  Generally speaking, in this case it
> > should not matter as we are trying to get the locale name but no harm
> > in testing.  Also, I think it would be good if we can test how this
> > impacts the error messages as Davinder is trying to do.
> >
>
>
> I have tried a simple test with the latest patch and it failed for me.
>
> Set LC_MESSAGES="English_United Kingdom";
> -- returns en-GB, then code changes it to en_NZ when _create_locale()
> is used whereas with the patch it returns "" (empty string).
>
> There seem to be two problems here (a) The input to enum_locales_fn
> doesn't seem to get the input name as "English_United Kingdom" due to
> which it can't find a match even if the same exists. (b) After
> executing EnumSystemLocalesEx, there is no way the patch can detect if
> it is successful in finding the passed name due to which it appends
> empty string in such cases.
>

Few more comments:
1. I have tried the first one in the list provided by you and that
also didn't work. Basically, I got  empty string when I tried Set
LC_MESSAGES='Afar';

2. Getting below warning
pg_locale.c(1072): warning C4133: 'function': incompatible types -
from 'const char *' to 'const wchar_t *'

3.
+ if (GetLocaleInfoEx(pStr, LOCALE_SENGLISHCOUNTRYNAME,
+ test_locale + len, LOCALE_NAME_MAX_LENGTH - len) > 0)

All > or <= 0 checks should be changed to "!" types which mean to
check whether the call toGetLocaleInfoEx is success or not.

4. In the patch, first, we try to get with LCType as LOCALE_SNAME and
then with LOCALE_SENGLISHLANGUAGENAME and LOCALE_SENGLISHCOUNTRYNAME.
I think we should add comments indicating why we try to get the locale
information with three LCTypes and why the specific order of trying
those types is required.

5. In one of the previous emails, you asked whether we have a list of
supported locales.  I don't find any such list. I think it depends on
Windows locales for which you can get the information from
https://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-lcid/a9eac961-e77d-41a6-90a5-ce1a8b0cdb9c

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Tue, Apr 21, 2020 at 5:32 PM Juan José Santamaría Flecha
<juanjo.santamaria@gmail.com> wrote:
>
> On Tue, Apr 21, 2020 at 12:41 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>>
>> On Tue, Apr 21, 2020 at 12:51 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>> >
>> > I have tried a simple test with the latest patch and it failed for me.
>> >
>> > Set LC_MESSAGES="English_United Kingdom";
>> > -- returns en-GB, then code changes it to en_NZ when _create_locale()
>> > is used whereas with the patch it returns "" (empty string).
>> >
>> > There seem to be two problems here (a) The input to enum_locales_fn
>> > doesn't seem to get the input name as "English_United Kingdom" due to
>> > which it can't find a match even if the same exists. (b) After
>> > executing EnumSystemLocalesEx, there is no way the patch can detect if
>> > it is successful in finding the passed name due to which it appends
>> > empty string in such cases.
>>
>> Few more comments:
>> 1. I have tried the first one in the list provided by you and that
>> also didn't work. Basically, I got  empty string when I tried Set
>> LC_MESSAGES='Afar';
>
>
> I cannot reproduce any of these errors on my end.
>

The first problem related to the English_United Kingdom was due to the
usage of wcslen instead of pg_mbstrlen to compute the length of
winlocname.  So, this is fixed with your latest patch.  I have
debugged the case for 'Afar' and found that _create_locale also didn't
return anything for that in my machine, so probably that locale
information is not there in my environment.

>  When using _create_locale(),  returning "en_NZ" is also a wrong result.
>

Hmm, that was a typo, it should be en_GB instead.

>
>> 4. In the patch, first, we try to get with LCType as LOCALE_SNAME and
>> then with LOCALE_SENGLISHLANGUAGENAME and LOCALE_SENGLISHCOUNTRYNAME.
>> I think we should add comments indicating why we try to get the locale
>> information with three LCTypes and why the specific order of trying
>> those types is required.
>
>
> Agreed.
>

But, I don't see much in the comments?

Few more comments:
1.
  if (rc == -1 || rc == sizeof(iso_lc_messages))
- return NULL;
+
iso_lc_messages[0] = '\0';

I don't think this change is required.  The caller expects NULL in
case the API is not successful so that it can point result directly to
the locale passed.  I have changed this back to the original code in
the attached patch.

2.
I see some differences in the output of GetLocaleInfoEx and
_create_locale for some locales as mentioned in one of the documents
shared by you. Ex.

Bemba_Zambia bem-ZM bem
Bena_Tanzania bez-TZ bez
Bulgarian_Bulgaria bg-BG bg

Now, these might be okay but I think unless we test such things by
seeing the error message changes due to these locales we can't be
sure.

3. In the attached patch, I have handled one of the problem reported
earlier aka "After executing EnumSystemLocalesEx, there is no way the
patch can detect if it is successful in finding the passed name due to
which it appends empty string in such cases."

4. I think for the matter of this API, it is better to use _MSC_VER
related checks instead of _WIN32_WINNT so as to be consistent with
similar usage in chklocale.c (see win32_langinfo).  We can later
change the checks at all places to _WIN32_WINNT if required.  I have
changed this as well in the attached patch.

5. I am slightly nervous about the usage of wchar functions like
_wcsicmp, wcslen, etc. as those are not used anywhere in the code.
OTOH, I don't see any problem with that.  There is pg_wchar datatype
in the code and some corresponding functions to manipulate it.  Have
you considered using it?

6. I have additionally done some cosmetic changes in the attached patch.

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Wed, Apr 22, 2020 at 7:37 PM Ranier Vilela <ranier.vf@gmail.com> wrote:
>
> Em qua., 22 de abr. de 2020 às 08:43, Amit Kapila <amit.kapila16@gmail.com> escreveu:
>>
>> On Tue, Apr 21, 2020 at 5:32 PM Juan José Santamaría Flecha
>> <juanjo.santamaria@gmail.com> wrote:
>> >
>> > On Tue, Apr 21, 2020 at 12:41 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>> >>
>>
>> 6. I have additionally done some cosmetic changes in the attached patch.
>
> I made some style changes too.
>
> 1. Change:
>  strcpy(iso_lc_messages, "C");
> to
> iso_lc_messages[0] = 'C';
> iso_lc_messages[1] = '\0';
>

This is an existing code and this patch has no purpose to touch it.
So, I don't think we should make this change.

> 2. Remove vars hyphen and underscore;
> 3. Avoid call second wcsrchr, if hyphen is not found.
>
> If it's not too much perfectionism.
>

(2) and (3) are improvements, so we can take those.

Thanks for participating in the review and development of this patch.
It really helps if more people help in improving the patch.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Wed, Apr 22, 2020 at 9:27 PM Juan José Santamaría Flecha
<juanjo.santamaria@gmail.com> wrote:
>
>
> On Wed, Apr 22, 2020 at 1:43 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>>
>>
>> >> 4. In the patch, first, we try to get with LCType as LOCALE_SNAME and
>> >> then with LOCALE_SENGLISHLANGUAGENAME and LOCALE_SENGLISHCOUNTRYNAME.
>> >> I think we should add comments indicating why we try to get the locale
>> >> information with three LCTypes and why the specific order of trying
>> >> those types is required.
>> >
>> >
>> > Agreed.
>> >
>>
>> But, I don't see much in the comments?
>
>
> I take notice.
>

Okay, I hope we will see better comments in the next version.

>>
>> Few more comments:
>> 1.
>>   if (rc == -1 || rc == sizeof(iso_lc_messages))
>> - return NULL;
>> +
>> iso_lc_messages[0] = '\0';
>>
>> I don't think this change is required.  The caller expects NULL in
>> case the API is not successful so that it can point result directly to
>> the locale passed.  I have changed this back to the original code in
>> the attached patch.
>
>
> I did not want to return anything without logging its value.
>

Hmm, if you really want to log the value then do it in the caller.  I
don't think making special arrangements just for logging this value is
a good idea.

>>
>> 2.
>> I see some differences in the output of GetLocaleInfoEx and
>> _create_locale for some locales as mentioned in one of the documents
>> shared by you. Ex.
>>
>> Bemba_Zambia bem-ZM bem
>> Bena_Tanzania bez-TZ bez
>> Bulgarian_Bulgaria bg-BG bg
>>
>> Now, these might be okay but I think unless we test such things by
>> seeing the error message changes due to these locales we can't be
>> sure.
>
>
> There are some cases where the language tag does not match, although I do not think is wrong:
>
> Asu asa Asu
> Edo bin Edo
> Ewe ee Ewe
> Rwa rwk Rwa
>
> To check the messages, do you have a regression test in mind?
>

I think we can check with simple error messages.  So, basically after
setting a particular value of LC_MESSAGES, execute a query which
returns syntax or any other error, if the error message is the same
irrespective of the locale name returned by _create_locale and
GetLocaleInfoEx, then we should be fine.  I want to especially try
where the return value is slightly different by _create_locale and
GetLocaleInfoEx.   I know Davinder is trying something like this but
if you can also try then it would be good.

>
>> 5. I am slightly nervous about the usage of wchar functions like
>> _wcsicmp, wcslen, etc. as those are not used anywhere in the code.
>> OTOH, I don't see any problem with that.  There is pg_wchar datatype
>> in the code and some corresponding functions to manipulate it.  Have
>> you considered using it?
>
>
> In Windows  wchar_t is 2 bytes, so we would have to do make UTF16 to UFT32 conversions back and forth. Not sure if it
isworth the effort. 
>

Yeah, I am also not sure about this.  So, let us see if anyone else
has any thoughts on this point, otherwise, we can go with wchar
functions as you have in the patch.

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Thu, Apr 23, 2020 at 5:37 PM Juan José Santamaría Flecha
<juanjo.santamaria@gmail.com> wrote:
>
>
> On Thu, Apr 23, 2020 at 5:30 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>>
>>
>> I think we can check with simple error messages.  So, basically after
>> setting a particular value of LC_MESSAGES, execute a query which
>> returns syntax or any other error, if the error message is the same
>> irrespective of the locale name returned by _create_locale and
>> GetLocaleInfoEx, then we should be fine.  I want to especially try
>> where the return value is slightly different by _create_locale and
>> GetLocaleInfoEx.   I know Davinder is trying something like this but
>> if you can also try then it would be good.
>
>
> I have composed a small set of queries to test the output with different lc_message settings (lc_messages_test.sql).
Pleasefind attached the output from debug3 logging using both EnumSystemLocalesEx (lc_messages_EnumSystemLocalesEx.log)
and_create_locale (lc_messages_create_locale.log). 
>

Thanks, I will verify these.  BTW, have you done something special to
get the error messages which are not in English because on my Windows
box I am not getting that in spite of setting it to the appropriate
locale.  Did you use ICU or something else?

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Thu, Apr 23, 2020 at 6:30 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Thu, Apr 23, 2020 at 5:37 PM Juan José Santamaría Flecha
> <juanjo.santamaria@gmail.com> wrote:
> >
> > I have composed a small set of queries to test the output with different lc_message settings
(lc_messages_test.sql).Please find attached the output from debug3 logging using both EnumSystemLocalesEx
(lc_messages_EnumSystemLocalesEx.log)and _create_locale (lc_messages_create_locale.log). 
> >
>
> Thanks, I will verify these.
>

The result looks good to me.  However, I think we should test a few
more locales, especially where we know there is some difference in
what _create_locale returns and what we get via enumerating locales
and using GetLocaleInfoEx.  Also, we should test some more locales
with the code page.  For ex.

Bemba_Zambia
Bena_Tanzania
Bulgarian_Bulgaria
Swedish_Sweden.1252
Swedish_Sweden

Then, I think we can also test a few where you mentioned that the
language tag is different.
Asu asa Asu
Edo bin Edo
Ewe ee Ewe
Rwa rwk Rwa

BTW, we have a list of code page which can be used for this testing in
below link.  I think we can primarily test Windows code page
identifiers (like 1250, 1251, .. 1258) from the link [1].

I think we should backpatch this till 9.5 as I could see the changes
made by commit 0fb54de9 to support MSVC2015 are present in that branch
and the same is mentioned in the commit message.  Would you like to
prepare patches (and test those) for back-branches?

I have made few cosmetic changes in the attached patch which includes
adding/editing a few comments, ran pgindent, etc.  I have replaced the
reference of "IETF-standardized" with "Unix-style" as we are already
using it at other places in the comments as well.

[1] - https://docs.microsoft.com/en-us/windows/win32/intl/code-page-identifiers

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Wed, Apr 29, 2020 at 1:32 AM Ranier Vilela <ranier.vf@gmail.com> wrote:
>
> Em ter., 28 de abr. de 2020 às 16:53, Juan José Santamaría Flecha <juanjo.santamaria@gmail.com> escreveu:
>>
>>
>>
>> On Tue, Apr 28, 2020 at 9:38 PM Ranier Vilela <ranier.vf@gmail.com> wrote:
>>>
>>> "pt" means portuguese language.
>>> "pt_BR" means portuguese language from Brazil, "divisão por zero", is correct.
>>> "pt_PT" means portuguese language from Portugal, "division by zero"? poderia ser "divisão por zero", too.
>>>
>>> Why "pt_PT" do not is translated?
>>
>>
>> The translation files are generated as 'pt_BR.po', so this is the expected behaviour.
>>
>> With my limited knowledge of Portuguese, it makes little sense to have a localized version.
>
> Well, both are PORTUGUE language, but, do not the same words.
> pt_PT.po, obviously is missing, I can provide a version, but still, it wouldn't be 100%, but it's something.
> Would it be useful?
>

I am not sure but that doesn't seem to be related to this patch.  If
it is not related to this patch then it is better to start a separate
thread (probably on pgsql-translators list).

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Wed, Apr 29, 2020 at 8:21 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Wed, Apr 29, 2020 at 1:32 AM Ranier Vilela <ranier.vf@gmail.com> wrote:
> >
> > Em ter., 28 de abr. de 2020 às 16:53, Juan José Santamaría Flecha <juanjo.santamaria@gmail.com> escreveu:
> >>
> >>
> >>
> >> On Tue, Apr 28, 2020 at 9:38 PM Ranier Vilela <ranier.vf@gmail.com> wrote:
> >>>
> >>> "pt" means portuguese language.
> >>> "pt_BR" means portuguese language from Brazil, "divisão por zero", is correct.
> >>> "pt_PT" means portuguese language from Portugal, "division by zero"? poderia ser "divisão por zero", too.
> >>>
> >>> Why "pt_PT" do not is translated?
> >>
> >>
> >> The translation files are generated as 'pt_BR.po', so this is the expected behaviour.
> >>
> >> With my limited knowledge of Portuguese, it makes little sense to have a localized version.
> >
> > Well, both are PORTUGUE language, but, do not the same words.
> > pt_PT.po, obviously is missing, I can provide a version, but still, it wouldn't be 100%, but it's something.
> > Would it be useful?
> >
>
> I am not sure but that doesn't seem to be related to this patch.  If
> it is not related to this patch then it is better to start a separate
> thread (probably on pgsql-translators list).
>

BTW, do you see any different results for pt_PT with create_locale
version or the new patch version being discussed here?

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Tue, Apr 28, 2020 at 11:39 PM Juan José Santamaría Flecha
<juanjo.santamaria@gmail.com> wrote:
>
>
> On Mon, Apr 27, 2020 at 1:20 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>>
>> I think we should backpatch this till 9.5 as I could see the changes
>> made by commit 0fb54de9 to support MSVC2015 are present in that branch
>> and the same is mentioned in the commit message.  Would you like to
>> prepare patches (and test those) for back-branches?
>
>
> I do not have means to test these patches using Visual Studio previous to 2012, but please find attached patches for
9.5-9.6and 10-11-12 as of version 14. The extension is 'txt' not to break the cfbot. 
>

I see some problems with these patches.
1.
+ loct = _create_locale(LC_CTYPE, winlocname);
+ if (loct != NULL)
+ {
+ lcid = loct->locinfo->lc_handle[LC_CTYPE];
+ if (lcid == 0)
+ lcid = MAKELCID(MAKELANGID(LANG_ENGLISH, SUBLANG_ENGLISH_US), SORT_DEFAULT);
+ _free_locale(loct);
+ }

  if (!GetLocaleInfoA(lcid, LOCALE_SISO639LANGNAME, isolang, sizeof(isolang)))
  return NULL;
  if (!GetLocaleInfoA(lcid, LOCALE_SISO3166CTRYNAME, isocrty, sizeof(isocrty)))
  return NULL;

In the above change even if loct is NULL, we call GetLocaleInfoA()
which is wrong and the same is not a problem without the patch.

2. I think the code in IsoLocaleName is quite confusing and difficult
to understand in back branches and the changes due to this bug-fix
made it more complicated.  I am thinking to refactor it such that the
code for (_MSC_VER >= 1700 && _MSC_VER  < 1900), (_MSC_VER >= 1900)
and last #else code (the code for version < 17) resides in their own
functions.  That might make this function easier to understand, what
do you think?

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Mon, Apr 27, 2020 at 4:50 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> I think we should backpatch this till 9.5 as I could see the changes
> made by commit 0fb54de9 to support MSVC2015 are present in that branch
> and the same is mentioned in the commit message.
>

Today, I was thinking about the pros and cons of backpatching this.
The pros are that this is bug-fix and is reported multiple times so it
is good to backpatch it.  The cons are the code in the back branches
is not very straight forward and this change will make it a bit more
complicated, so we might want to do it only in HEAD.  I am not
completely sure about this.  What do others think?

Michael, others who have worked in this area, do you have any opinion
on this matter?

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Wed, Apr 29, 2020 at 5:57 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
>
> 2. I think the code in IsoLocaleName is quite confusing and difficult
> to understand in back branches and the changes due to this bug-fix
> made it more complicated.  I am thinking to refactor it such that the
> code for (_MSC_VER >= 1700 && _MSC_VER  < 1900), (_MSC_VER >= 1900)
> and last #else code (the code for version < 17) resides in their own
> functions.
>

Another possibility could be to add just a branch for (_MSC_VER >=
1900) and add that code in a separate function without touching other
parts of this function.  That would avoid testing it various versions
of MSVC.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Wed, Apr 29, 2020 at 9:36 PM Juan José Santamaría Flecha
<juanjo.santamaria@gmail.com> wrote:
>
> On Wed, Apr 29, 2020 at 3:50 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>>
>> On Wed, Apr 29, 2020 at 5:57 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>> >
>> > 2. I think the code in IsoLocaleName is quite confusing and difficult
>> > to understand in back branches and the changes due to this bug-fix
>> > made it more complicated.  I am thinking to refactor it such that the
>> > code for (_MSC_VER >= 1700 && _MSC_VER  < 1900), (_MSC_VER >= 1900)
>> > and last #else code (the code for version < 17) resides in their own
>> > functions.
>> >
>>
>> Another possibility could be to add just a branch for (_MSC_VER >=
>> 1900) and add that code in a separate function without touching other
>> parts of this function.  That would avoid testing it various versions
>> of MSVC.
>
>
> I was not aware of how many switches IsoLocaleName() already had before trying to backpatch. I think offering an
alternativemight be a cleaner approach, I will work on that. 
>

Okay, thanks.  The key point to keep in mind is to avoid touching the
code related to prior MSVC versions as we might not have set up to
test those.

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Mon, May 4, 2020 at 6:59 PM Juan José Santamaría Flecha
<juanjo.santamaria@gmail.com> wrote:
>
> On Thu, Apr 30, 2020 at 5:07 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>>
>>
>> Okay, thanks.  The key point to keep in mind is to avoid touching the
>> code related to prior MSVC versions as we might not have set up to
>> test those.
>
>
> Please find attached a new version following this approach.
>

Thanks for the new version.  I have found few problems and made
changes accordingly.  In back-branch patches, I found one major
problem.

+#if (_MSC_VER >= 1900) /* Visual Studio 2015 or later */
+ rc = get_iso_localename(winlocname, iso_lc_messages);
+#else

Here, we need to free loct, otherwise, it will leak each time this
function is called on a newer MSVC version.  Also, call to
_create_locale is redundant in _MSC_VER >= 1900. So, I have tried to
write it differently, see what do you think about it?

*
+ * BEWARE: this function is WIN32 specific, so wchar_t are UTF-16.
I am not sure how much relevant is this comment so removed for now.

Apart from that, I have made a few other changes in comments, fixed
typos, and ran pgindent.  Let me know what do you think of attached
patches?

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Wed, May 6, 2020 at 4:19 AM Juan José Santamaría Flecha
<juanjo.santamaria@gmail.com> wrote:
>
> On Tue, May 5, 2020 at 1:34 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>>
>>
>> Apart from that, I have made a few other changes in comments, fixed
>> typos, and ran pgindent.  Let me know what do you think of attached
>> patches?
>
>
> The patches are definitely in better shape.
>
> I think that the definition of get_iso_localename() should be consistent across all versions, that is HEAD like
back-patched.
>

Fair enough.  I have changed such that get_iso_localename is the same
in HEAD as it is backbranch patches.  I have attached backbranch
patches for the ease of verification.


--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Wed, May 6, 2020 at 11:01 PM Juan José Santamaría Flecha
<juanjo.santamaria@gmail.com> wrote:
>
> On Wed, May 6, 2020 at 6:41 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>>
>> On Wed, May 6, 2020 at 4:19 AM Juan José Santamaría Flecha
>> >
>> > I think that the definition of get_iso_localename() should be consistent across all versions, that is HEAD like
back-patched.
>> >
>>
>> Fair enough.  I have changed such that get_iso_localename is the same
>> in HEAD as it is backbranch patches.  I have attached backbranch
>> patches for the ease of verification.
>
>
> LGTM, and I see no regression in the manual SQL tests, so no further comments from my part.
>

Thanks, Juan and Davinder for verifying the latest patches. I think
this patch is ready to commit unless someone else has any comments.  I
will commit and backpatch this early next week (probably on Monday)
unless I see more comments.

To summarize, this is a longstanding issue of Windows build (NLS
enabled builds) for Visual Studio 2015 and later releases.  Visual
Studio 2015 and later versions should still be able to do the same as
Visual Studio 2012, but the declaration of locale_name is missing in
_locale_t, causing the code compilation to fail, hence this patch
falls back
instead on to enumerating all system locales by using
EnumSystemLocalesEx to find the required locale name.  If the input
argument is in Unix-style then we can get ISO Locale name directly by
using GetLocaleInfoEx() with LCType as LOCALE_SNAME.

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Amit Kapila <amit.kapila16@gmail.com> writes:
> Thanks, Juan and Davinder for verifying the latest patches. I think
> this patch is ready to commit unless someone else has any comments.  I
> will commit and backpatch this early next week (probably on Monday)
> unless I see more comments.

Monday is a back-branch release wrap day.  If you push a back-patched
change on that day (or immediately before it), it had better be a security
fix.

            regards, tom lane

On Thu, May 7, 2020 at 11:57 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Amit Kapila <amit.kapila16@gmail.com> writes:
> > Thanks, Juan and Davinder for verifying the latest patches. I think
> > this patch is ready to commit unless someone else has any comments.  I
> > will commit and backpatch this early next week (probably on Monday)
> > unless I see more comments.
>
> Monday is a back-branch release wrap day.  If you push a back-patched
> change on that day (or immediately before it), it had better be a security
> fix.
>

Okay.  I'll wait in that case and will push it after that.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Thu, May 7, 2020 at 11:57 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Amit Kapila <amit.kapila16@gmail.com> writes:
> > Thanks, Juan and Davinder for verifying the latest patches. I think
> > this patch is ready to commit unless someone else has any comments.  I
> > will commit and backpatch this early next week (probably on Monday)
> > unless I see more comments.
>
> Monday is a back-branch release wrap day.
>

How can I get the information about release wrap day?  The minor
release dates are mentioned on the website [1], but this information
is not available.  Do we keep it some-fixed number of days before
minor release?


[1] - https://www.postgresql.org/developer/roadmap/

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Amit Kapila <amit.kapila16@gmail.com> writes:
> On Thu, May 7, 2020 at 11:57 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Monday is a back-branch release wrap day.

> How can I get the information about release wrap day?  The minor
> release dates are mentioned on the website [1], but this information
> is not available.  Do we keep it some-fixed number of days before
> minor release?

Yes, we've been using the same release schedule for years.  The
actual tarball wrap is always on a Monday --- if I'm doing it,
as is usually the case, I try to get it done circa 2100-2300 UTC.
There's a "quiet period" where we discourage nonessential commits
both before (starting perhaps on the Saturday) and after (until
the releases are tagged in git, about 24 hours after wrap).
The delay till public announcement on the Thursday is so the
packagers can produce their builds.  Likewise, the reason for
a wrap-to-tag delay is in case the packagers find anything that
forces a re-wrap, which has happened a few times.

            regards, tom lane

On Thu, May 7, 2020 at 6:51 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Amit Kapila <amit.kapila16@gmail.com> writes:
> > On Thu, May 7, 2020 at 11:57 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> Monday is a back-branch release wrap day.
>
> > How can I get the information about release wrap day?  The minor
> > release dates are mentioned on the website [1], but this information
> > is not available.  Do we keep it some-fixed number of days before
> > minor release?
>
> Yes, we've been using the same release schedule for years.  The
> actual tarball wrap is always on a Monday --- if I'm doing it,
> as is usually the case, I try to get it done circa 2100-2300 UTC.
> There's a "quiet period" where we discourage nonessential commits
> both before (starting perhaps on the Saturday) and after (until
> the releases are tagged in git, about 24 hours after wrap).
> The delay till public announcement on the Thursday is so the
> packagers can produce their builds.  Likewise, the reason for
> a wrap-to-tag delay is in case the packagers find anything that
> forces a re-wrap, which has happened a few times.
>

Now that branches are tagged, I would like to commit and backpatch
this patch tomorrow unless there are any more comments/objections.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Amit Kapila <amit.kapila16@gmail.com> writes:
> Now that branches are tagged, I would like to commit and backpatch
> this patch tomorrow unless there are any more comments/objections.

The "quiet period" is over as soon as the tags appear in git.

            regards, tom lane

On Wed, May 13, 2020 at 7:34 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Amit Kapila <amit.kapila16@gmail.com> writes:
> > Now that branches are tagged, I would like to commit and backpatch
> > this patch tomorrow unless there are any more comments/objections.
>
> The "quiet period" is over as soon as the tags appear in git.
>

Pushed.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com