Re: PG compilation error with Visual Studio 2015/2017/2019

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. This function 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 the name of that locale by pointing
to internal elements of the structure loct->locinfo->locale_name[LC_CTYPE]
but it has been missing from the _locale_t since VS2015 which is causing
the compilation error. I found a few useful APIs that can be 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 name is 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 correct it 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 only difference is for
a bigger set.
I thought of using GetLocaleInfoEx for the fix because it behaves similarly
to the already existing and a similar issue was resolved earlier using the
same. I have attached the patch, let me know your thoughts.

[1]
/message-id/e317eec9-d40d-49b9-b776-e89cf1d18c82@postgrespro.ru
[2] /message-id/23073.1526049547%40sss.pgh.pa.us
[3] https://docs.microsoft.com/en-us/windows/win32/intl/locale-names
[4]
https://docs.microsoft.com/en-us/windows/win32/api/winnls/nf-winnls-resolvelocalename
[5]
https://docs.microsoft.com/en-us/windows/win32/api/winnls/nf-winnls-getlocaleinfoex
--
Regards,
Davinder.
EnterpriseDB: http://www.enterprisedb.com

On Mon, Apr 6, 2020 at 1:08 PM davinder singh <davindersingh2692(at)gmail(dot)com>
wrote:

>
> 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. This function is trying to
> convert the locale name into the Unix style. For example, it will change
> the locale name "en-US" into "en_US".
>

How do you reproduce this issue with Visual Studio? I see there is an ifdef
directive above IsoLocaleName():

#if defined(WIN32) && defined(LC_MESSAGES)

I would expect defined(LC_MESSAGES) to be false in MSVC.

Regards,

Juan José Santamaría Flecha

On Mon, Apr 6, 2020 at 8:17 PM Juan José Santamaría Flecha <
juanjo(dot)santamaria(at)gmail(dot)com> wrote:

>
> How do you reproduce this issue with Visual Studio? I see there is an
> ifdef directive above IsoLocaleName():
>
> #if defined(WIN32) && defined(LC_MESSAGES)
>
> I would expect defined(LC_MESSAGES) to be false in MSVC.
>

You need to enable NLS support in the config file. Let me know if that
answers your question.

--
Regards,
Davinder.

On Tue, Apr 7, 2020 at 7:44 AM davinder singh <davindersingh2692(at)gmail(dot)com>
wrote:

>
> You need to enable NLS support in the config file. Let me know if that
> answers your question.
>

Yes, I can see where this is coming from now, thanks.

Currently, it is using _create_locale it behaves similarly to
> GetLocaleInfoEx i.e. it also only checks the format only difference is for
> a bigger set.
> I thought of using GetLocaleInfoEx for the fix because it behaves
> similarly to the already existing and a similar issue was resolved earlier
> using the same. I have attached the patch, let me know your thoughts.
>

You are right, that the way to get the locale name information is through
GetLocaleInfoEx().

A couple of comments on the patch:

* 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.

* This looks like a spurious change:
- sizeof(iso_lc_messages), NULL);
+ sizeof(iso_lc_messages), NULL);

Regards,

Juan José Santamaría Flecha

On Tue, Apr 7, 2020 at 8:30 PM Juan José Santamaría Flecha <
juanjo(dot)santamaria(at)gmail(dot)com> wrote:

>
> * 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 think "_WIN32_WINNT >= 0x0600" represents windows versions only and
doesn't include any information about Visual Studio versions. So I am
sticking to " defined(_MSC_VER) && (_MSC_VER >= 1900)".
I have resolved other comments. I have attached a new version of the patch.
--
Regards,
Davinder.

On Wed, Apr 8, 2020 at 9:03 AM davinder singh <davindersingh2692(at)gmail(dot)com>
wrote:

> On Tue, Apr 7, 2020 at 8:30 PM Juan José Santamaría Flecha <
> juanjo(dot)santamaria(at)gmail(dot)com> wrote:
>
>>
>> * 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 think "_WIN32_WINNT >= 0x0600" represents windows versions only and
> doesn't include any information about Visual Studio versions. So I am
> sticking to " defined(_MSC_VER) && (_MSC_VER >= 1900)".
>

Let me explain further, in pg_config_os.h you can check that the value of
_WIN32_WINNT is solely based on checking _MSC_VER. This patch should also
be meaningful for WIN32 builds using MinGW, or we might see this issue
reappear in those systems if update the MIN_WINNT value to more current
OS versions. So, I still think _WIN32_WINNT is a better option.

I have resolved other comments. I have attached a new version of the patch.
>

I still see the same last lines in both #ifdef blocks, and pgindent might
change a couple of lines to:
+ MultiByteToWideChar(CP_ACP, 0, winlocname, -1, wc_locale_name,
+ LOCALE_NAME_MAX_LENGTH);
+
+ if ((GetLocaleInfoEx(wc_locale_name, LOCALE_SNAME,
+ (LPWSTR)&buffer, LOCALE_NAME_MAX_LENGTH)) > 0)
+ {

But that is pretty trivial stuff.

Please open an item in the commitfest for this patch.

Regards,

Juan José Santamaría Flecha

On Wed, Apr 8, 2020 at 7:39 PM Juan José Santamaría Flecha

> Let me explain further, in pg_config_os.h you can check that the value of
> _WIN32_WINNT is solely based on checking _MSC_VER. This patch should also
> be meaningful for WIN32 builds using MinGW, or we might see this issue
> reappear in those systems if update the MIN_WINNT value to more current
> OS versions. So, I still think _WIN32_WINNT is a better option.
>
Thanks for explanation, I was not aware of that, you are right it make
sense to use " _WIN32_WINNT", Now I am using this only.

I still see the same last lines in both #ifdef blocks, and pgindent might
> change a couple of lines to:
> + MultiByteToWideChar(CP_ACP, 0, winlocname, -1, wc_locale_name,
> + LOCALE_NAME_MAX_LENGTH);
> +
> + if ((GetLocaleInfoEx(wc_locale_name, LOCALE_SNAME,
> + (LPWSTR)&buffer, LOCALE_NAME_MAX_LENGTH)) > 0)
> + {
>
Now I have resolved these comments also, Please check updated version of
the patch.

> Please open an item in the commitfest for this patch.
>
I have created with same title.

--
Regards,
Davinder
EnterpriseDB: http://www.enterprisedb.com

On Mon, Apr 6, 2020 at 4:38 PM davinder singh
<davindersingh2692(at)gmail(dot)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. This function 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 the name of that locale by pointing to internal elements of the structure loct->locinfo->locale_name[LC_CTYPE] but it has been missing from the _locale_t since VS2015 which is causing the compilation error. I found a few useful APIs that can be 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 name is 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 correct it 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 only difference is for a bigger set.
> I thought of using GetLocaleInfoEx for the fix because it behaves similarly to the already existing and a similar issue was 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(dot)santamaria(at)gmail(dot)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(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Tue, Apr 7, 2020 at 8:30 PM Juan José Santamaría Flecha
> <juanjo(dot)santamaria(at)gmail(dot)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?
>

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 Fri, Apr 10, 2020 at 2:25 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:

>
> 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.

Currently there are two constructs to test the same logic, which is not
great. I think that using _MSC_VER makes it seem as MSVC exclusive code,
when MinGW should also be considered.

In the longterm aligning Postgres with MS product obsolescence will make
these tests unneeded, but I can propose a patch for making the test
consistent in all cases, on a different thread since this has little to do
with $SUBJECT.

Regards,

Juan José Santamaría Flecha

>
>

On Fri, Apr 10, 2020 at 5:33 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> 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?
I am in investigating in similar lines, I think the following explanation
can help.
From Microsoft doc.
The locale argument to the setlocale, _wsetlocale, _create_locale, and
_wcreate_locale is
locale :: "locale-name"
| *"language[_country-region[.code-page]]"*
| ".code-page"
| "C"
| ""
| NULL

For GetLocaleInfoEx locale argument is
*<language>-<REGION>*
<language>-<Script>-<REGION>

As we can see _create_locale will also accept the locale appended with
code-page but that is not the case in lateral.
The important point is in our current issue we need locale name only and
both
functions(GetLocaleInfoEx, _create_locale) support an equal number of
locales
names if go by the syntax of the locale described on Microsoft documents.
With that thought, I am parsing the input
string to remove the code-page and give it as input to GelLocaleInfoEx.
I have attached the new patch.

> 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?
I don't know how to use these tests on windows, but I had a look in these
tests, I didn't found any test which could hit the function we are
modifying.
I m still working on testing this patch. If anyone has Idea please suggest.

[1] https://docs.microsoft.com/en-us/windows/win32/intl/locale-names
[2]
https://docs.microsoft.com/en-us/windows/win32/api/winnls/nf-winnls-getlocaleinfoex
[3]
https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/create-locale-wcreate-locale?view=vs-2019
--
Regards,
Davinder
EnterpriseDB: http://www.enterprisedb.com

Thanks for the review comments.

On Tue, Apr 14, 2020 at 9:12 PM Ranier Vilela <ranier(dot)vf(at)gmail(dot)com> wrote:

> >>I m still working on testing this patch. If anyone has Idea please
> suggest.
> I still see problems with this patch.
>
> 1. Variable loct have redundant initialization, it would be enough to
> declare so: _locale_t loct;
> 2. Style white space in variable rc declaration.
> 3. Style variable cp_index can be reduced.
> if (tmp != NULL) {
> size_t cp_index;
>
> cp_index = (size_t)(tmp - winlocname);
> strncpy(loc_name, winlocname, cp_index);
> loc_name[cp_index] = '\0';
> 4. Memory leak if _WIN32_WINNT >= 0x0600 is true, _free_locale(loct); is
> not called.
>
I resolved the above comments.

> 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.

I have attached the patch.
--
Regards,
Davinder
EnterpriseDB: http://www.enterprisedb.com

On Tue, Apr 14, 2020 at 1:07 PM davinder singh <davindersingh2692(at)gmail(dot)com>
wrote:

>
> On Fri, Apr 10, 2020 at 5:33 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
> wrote:
> >
> > 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?
> I am in investigating in similar lines, I think the following explanation
> can help.
> From Microsoft doc.
> The locale argument to the setlocale, _wsetlocale, _create_locale, and
> _wcreate_locale is
> locale :: "locale-name"
> | *"language[_country-region[.code-page]]"*
> | ".code-page"
> | "C"
> | ""
> | NULL
>
> For GetLocaleInfoEx locale argument is
> *<language>-<REGION>*
> <language>-<Script>-<REGION>
>
> As we can see _create_locale will also accept the locale appended with
> code-page but that is not the case in lateral.
> The important point is in our current issue we need locale name only and
> both
> functions(GetLocaleInfoEx, _create_locale) support an equal number of
> locales
> names if go by the syntax of the locale described on Microsoft documents.
> With that thought, I am parsing the input
> string to remove the code-page and give it as input to GelLocaleInfoEx.
> I have attached the new patch.
>
> > 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?
> I don't know how to use these tests on windows, but I had a look in these
> tests, I didn't found any test which could hit the function we are
> modifying.
> I m still working on testing this patch. If anyone has Idea please suggest.
>
> [1] https://docs.microsoft.com/en-us/windows/win32/intl/locale-names
> [2]
> https://docs.microsoft.com/en-us/windows/win32/api/winnls/nf-winnls-getlocaleinfoex
> [3]
> https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/create-locale-wcreate-locale?view=vs-2019
> --
> Regards,
> Davinder
> EnterpriseDB: http://www.enterprisedb.com
>

--
Regards,
Davinder
EnterpriseDB: http://www.enterprisedb.com

Em qua., 15 de abr. de 2020 às 03:08, davinder singh <
davindersingh2692(at)gmail(dot)com> escreveu:

>
> Thanks for the review comments.
>
> On Tue, Apr 14, 2020 at 9:12 PM Ranier Vilela <ranier(dot)vf(at)gmail(dot)com> wrote:
>
>> >>I m still working on testing this patch. If anyone has Idea please
>> suggest.
>> I still see problems with this patch.
>>
>> 1. Variable loct have redundant initialization, it would be enough to
>> declare so: _locale_t loct;
>> 2. Style white space in variable rc declaration.
>> 3. Style variable cp_index can be reduced.
>> if (tmp != NULL) {
>> size_t cp_index;
>>
>> cp_index = (size_t)(tmp - winlocname);
>> strncpy(loc_name, winlocname, cp_index);
>> loc_name[cp_index] = '\0';
>> 4. Memory leak if _WIN32_WINNT >= 0x0600 is true, _free_locale(loct); is
>> not called.
>>
> I resolved the above comments.
>
Ok, thanks.

>
>> 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 create structures or use malloc, to
be used when _WIN32_WINNT> = 0x0600 is true, but it is only a suggestion,
if you think it is necessary.

But have a last problem, in case GetLocaleInfoEx fail, there is still one
memory leak,
before return NULL is needed call: _free_locale(loct);

Another suggestion, if GetLocaleInfoEx fail wouldn't it be good to log the
error and the error number?

regards,
Ranier Vilela

On Wed, Apr 15, 2020 at 4:46 PM Ranier Vilela <ranier(dot)vf(at)gmail(dot)com> wrote:

> Em qua., 15 de abr. de 2020 às 03:08, davinder singh <
> davindersingh2692(at)gmail(dot)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 create structures or use malloc, to
> be used when _WIN32_WINNT> = 0x0600 is true, but it is only a suggestion,
> if you think it 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().

Please find attached a patch for so.

[1]
https://sourceforge.net/p/mingw-w64/mailman/mingw-w64-public/?limit=250&page=2
[2] https://github.com/mirror/mingw-w64/commit/b508bb87ad

Regards,

Juan José Santamaría Flecha

>
>

Em qua., 15 de abr. de 2020 às 15:28, Juan José Santamaría Flecha <
juanjo(dot)santamaria(at)gmail(dot)com> escreveu:

>
>
> On Wed, Apr 15, 2020 at 4:46 PM Ranier Vilela <ranier(dot)vf(at)gmail(dot)com> wrote:
>
>> Em qua., 15 de abr. de 2020 às 03:08, davinder singh <
>> davindersingh2692(at)gmail(dot)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 create structures or use malloc, to
>> be used when _WIN32_WINNT> = 0x0600 is true, but it is only a suggestion,
>> if you think it 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().
>
Sounds good to me, the exception maybe log error in case fail?

regards,
Ranier Vilela

On Wed, Apr 15, 2020 at 11:58 PM Juan José Santamaría Flecha
<juanjo(dot)santamaria(at)gmail(dot)com> wrote:
>
> On Wed, Apr 15, 2020 at 4:46 PM Ranier Vilela <ranier(dot)vf(at)gmail(dot)com> wrote:
>>
>> Em qua., 15 de abr. de 2020 às 03:08, davinder singh <davindersingh2692(at)gmail(dot)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 create structures or use malloc, to be used when _WIN32_WINNT> = 0x0600 is true, but it is only a suggestion, if you think it 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 Fri, Apr 17, 2020 at 10:33 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
wrote:

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

Thanks for the thorough review.

> 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.
>

We can get a match for those locales in non-ISO format by enumerating
available locales with EnumSystemLocalesEx(), and trying to find a match.

Please find a new patch for so.

Regards,

Juan José Santamaría Flecha

On Sat, Apr 18, 2020 at 12:14 AM Juan José Santamaría Flecha
<juanjo(dot)santamaria(at)gmail(dot)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.
>
> 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

Em sex., 17 de abr. de 2020 às 15:44, Juan José Santamaría Flecha <
juanjo(dot)santamaria(at)gmail(dot)com> escreveu:

>
> On Fri, Apr 17, 2020 at 10:33 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
> wrote:
>
>>
>> I see some differences in the output when _create_locale() is used vs.
>> when GetLocaleInfoEx() is used. Forex.
>>
>
> Thanks for the thorough review.
>
>
>> 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.
>>
>
> We can get a match for those locales in non-ISO format by enumerating
> available locales with EnumSystemLocalesEx(), and trying to find a match.
>
> Please find a new patch for so.
>
I have some observations about this patch, related to style, if you will
allow me.
1. argv variable on function enum_locales_fn can be reduced.
2. Var declaration len escapes the Postgres style.
3. Why call wcslen(test_locale), again, when var len have the size?

+static BOOL CALLBACK
+enum_locales_fn(LPWSTR pStr, DWORD dwFlags, LPARAM lparam)
+{
+ WCHAR test_locale[LOCALE_NAME_MAX_LENGTH];
+
+ memset(test_locale, 0, sizeof(test_locale));
+ if (GetLocaleInfoEx(pStr, LOCALE_SENGLISHLANGUAGENAME,
+ test_locale, LOCALE_NAME_MAX_LENGTH) > 0)
+ {
+ size_t len;
+
+ wcscat(test_locale, L"_");
+ len = wcslen(test_locale);
+ if (GetLocaleInfoEx(pStr, LOCALE_SENGLISHCOUNTRYNAME,
+ test_locale + len, LOCALE_NAME_MAX_LENGTH - len) > 0)
+ {
+ WCHAR **argv = (WCHAR **) lparam;
+
+ if (wcsncmp(argv[0], test_locale, len) == 0)
+ {
+ wcscpy(argv[1], pStr);
+ return FALSE;
+ }
+ }
+ }
+ return TRUE;
+}

regards,
Ranier Vilela

On Sat, Apr 18, 2020 at 6:07 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:

> On Sat, Apr 18, 2020 at 12:14 AM Juan José Santamaría Flecha
> <juanjo(dot)santamaria(at)gmail(dot)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], and additional supported country and region strings [3]. Based
on the results from this test program, it is possible to to do 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.

On Sat, Apr 18, 2020 at 1:43 PM Ranier Vilela <ranier(dot)vf(at)gmail(dot)com> wrote:

> I have some observations about this patch, related to style, if you will
> allow me.
>

Please find attached a revised version.

[1]
https://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-lcid/a9eac961-e77d-41a6-90a5-ce1a8b0cdb9c
[2] https://docs.microsoft.com/en-us/cpp/c-runtime-library/language-strings
<https://docs.microsoft.com/en-us/cpp/c-runtime-library/language-strings?view=vs-2019>
[3]
https://docs.microsoft.com/en-us/cpp/c-runtime-library/country-region-strings
<https://docs.microsoft.com/en-us/cpp/c-runtime-library/country-region-strings?view=vs-2019>

Em dom., 19 de abr. de 2020 às 07:16, Juan José Santamaría Flecha <
juanjo(dot)santamaria(at)gmail(dot)com> escreveu:

>
> On Sat, Apr 18, 2020 at 6:07 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
> wrote:
>
>> On Sat, Apr 18, 2020 at 12:14 AM Juan José Santamaría Flecha
>> <juanjo(dot)santamaria(at)gmail(dot)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], and additional supported country and region strings [3]. Based
> on the results from this test program, it is possible to to do 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.
>
> On Sat, Apr 18, 2020 at 1:43 PM Ranier Vilela <ranier(dot)vf(at)gmail(dot)com> wrote:
>
>> I have some observations about this patch, related to style, if you will
>> allow me.
>>
>
> Please find attached a revised version.
>
Looks good to me, but, sorry, I think I missed a glitch in the previous
review..

+#else /* _WIN32_WINNT < 0x0600 */
+ _locale_t loct;
+
+ loct = _create_locale(LC_CTYPE, winlocname);
+ if (loct != NULL)
+{
+ rc = wchar2char(iso_lc_messages, loct->locinfo->locale_name[LC_CTYPE],
+ sizeof(iso_lc_messages), NULL);
_free_locale(loct);
+}
+#endif /* _WIN32_WINNT >= 0x0600 */

If _create_locale fail, no need to call _free_locale(loct);.

Another point is, what is the difference between pg_mbstrlen and wcslen?
It would not be better to use only wcslen?

Attached have the patch with this comments.

regards,
Ranier Vilela

On Sun, Apr 19, 2020 at 1:58 PM Ranier Vilela <ranier(dot)vf(at)gmail(dot)com> wrote:

> Em dom., 19 de abr. de 2020 às 07:16, Juan José Santamaría Flecha <
> juanjo(dot)santamaria(at)gmail(dot)com> escreveu:
>
>> On Sat, Apr 18, 2020 at 1:43 PM Ranier Vilela <ranier(dot)vf(at)gmail(dot)com>
>> wrote:
>>
>>> I have some observations about this patch, related to style, if you will
>>> allow me.
>>>
>> Please find attached a revised version.
>>
> Looks good to me, but, sorry, I think I missed a glitch in the previous
> review.
> If _create_locale fail, no need to call _free_locale(loct);.
>
> Another point is, what is the difference between pg_mbstrlen and wcslen?
> It would not be better to use only wcslen?
>

pg_mbstrlen() is for multibyte strings and wcslen() is for wide-character
strings, the "pg" equivalent would be pg_wchar_strlen().

Attached have the patch with this comments.
>

+ } else

This line needs a break, other than that LGTM.

Regards,

Juan José Santamaría Flecha

Em dom., 19 de abr. de 2020 às 12:34, Juan José Santamaría Flecha <
juanjo(dot)santamaria(at)gmail(dot)com> escreveu:

>
> On Sun, Apr 19, 2020 at 1:58 PM Ranier Vilela <ranier(dot)vf(at)gmail(dot)com> wrote:
>
>> Em dom., 19 de abr. de 2020 às 07:16, Juan José Santamaría Flecha <
>> juanjo(dot)santamaria(at)gmail(dot)com> escreveu:
>>
>>> On Sat, Apr 18, 2020 at 1:43 PM Ranier Vilela <ranier(dot)vf(at)gmail(dot)com>
>>> wrote:
>>>
>>>> I have some observations about this patch, related to style, if you
>>>> will allow me.
>>>>
>>> Please find attached a revised version.
>>>
>> Looks good to me, but, sorry, I think I missed a glitch in the previous
>> review.
>> If _create_locale fail, no need to call _free_locale(loct);.
>>
>> Another point is, what is the difference between pg_mbstrlen and wcslen?
>> It would not be better to use only wcslen?
>>
>
> pg_mbstrlen() is for multibyte strings and wcslen() is for wide-character
> strings, the "pg" equivalent would be pg_wchar_strlen().
>
> Attached have the patch with this comments.
>>
>
> + } else
>
> This line needs a break, other than that LGTM.
>
Sure. Attached new patch with this revision.

regards,
Ranier Vilela

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(dot)santamaria(at)gmail(dot)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(at)paquier(dot)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(dot)santamaria(at)gmail(dot)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 Mon, Apr 20, 2020 at 10:10 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
wrote:

> 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 am trying to understand how lc_messages affect the error messages on
Windows,
but I haven't seen any change in the error message like on the Linux system
we change lc_messages.
Can someone help me with this? Please let me know if there is any
configuration setting that I need to adjust.

--
Regards,
Davinder
EnterpriseDB: http://www.enterprisedb.com

On Sun, Apr 19, 2020 at 3:46 PM Juan José Santamaría Flecha
<juanjo(dot)santamaria(at)gmail(dot)com> wrote:
>
>
> On Sat, Apr 18, 2020 at 6:07 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>>
>> On Sat, Apr 18, 2020 at 12:14 AM Juan José Santamaría Flecha
>> <juanjo(dot)santamaria(at)gmail(dot)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], and additional supported country and region strings [3]. Based on the results from this test program, it is possible to to do 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(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Sun, Apr 19, 2020 at 3:46 PM Juan José Santamaría Flecha
> <juanjo(dot)santamaria(at)gmail(dot)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], and additional supported country and region strings [3]. Based on the results from this test program, it is possible to to do 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(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Mon, Apr 20, 2020 at 6:32 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > On Sun, Apr 19, 2020 at 3:46 PM Juan José Santamaría Flecha
> > <juanjo(dot)santamaria(at)gmail(dot)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], and additional supported country and region strings [3]. Based on the results from this test program, it is possible to to do 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 12:41 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
wrote:

> On Tue, Apr 21, 2020 at 12:51 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)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. When using
_create_locale(), returning "en_NZ" is also a wrong result.

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

Yes, that is a regression.

> 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.
>

MSVC does not recommend "!" in all cases, but GetLocaleInfoEx() looks fine,
so agreed.

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.

> 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

Yes, that is the information we get from EnumSystemLocalesEx(), without the
additional entries _create_locale() has.

Please find attached a new version addressing the above mentioned, and so
adding a debug message for trying to get more information on the failed
cases.

Regards,

Juan José Santamaría Flecha
.

>
>

Em ter., 21 de abr. de 2020 às 09:02, Juan José Santamaría Flecha <
juanjo(dot)santamaria(at)gmail(dot)com> escreveu:

>
> On Tue, Apr 21, 2020 at 12:41 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
> wrote:
>
>> On Tue, Apr 21, 2020 at 12:51 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)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. When using
> _create_locale(), returning "en_NZ" is also a wrong result.
>
>
>> 2. Getting below warning
>> pg_locale.c(1072): warning C4133: 'function': incompatible types -
>> from 'const char *' to 'const wchar_t *'
>>
>
> Yes, that is a regression.
>
>
>> 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.
>>
>
> MSVC does not recommend "!" in all cases, but GetLocaleInfoEx() looks
> fine, so agreed.
>
> 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.
>
>
>> 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
>
>
> Yes, that is the information we get from EnumSystemLocalesEx(), without
> the additional entries _create_locale() has.
>
> Please find attached a new version addressing the above mentioned, and so
> adding a debug message for trying to get more information on the failed
> cases.
>
More few comments.

1. Comments about order:
/*
* Callback function for EnumSystemLocalesEx.
* Stop enumerating if a match is found for a locale with the format
* <Language>_<Country>.
* The order for search locale is essential:
* Find LCType first as LOCALE_SNAME, if not found try
LOCALE_SENGLISHLANGUAGENAME and
* finally LOCALE_SENGLISHCOUNTRYNAME, before return.
*/

Typo "enumarating".

2. Maybe the fail has here:

if (hyphen == NULL || underscore == NULL)

Change || to &&, the logical is wrong?

3. Why iso_lc_messages[0] = '\0'?

If we go call strchr, soon after, it's a waste.

regards,
Ranier Vilela

On Tue, Apr 21, 2020 at 2:22 PM Ranier Vilela <ranier(dot)vf(at)gmail(dot)com> wrote:

> More few comments.
>
> 1. Comments about order:
> /*
> * Callback function for EnumSystemLocalesEx.
> * Stop enumerating if a match is found for a locale with the format
> * <Language>_<Country>.
> * The order for search locale is essential:
> * Find LCType first as LOCALE_SNAME, if not found try
> LOCALE_SENGLISHLANGUAGENAME and
> * finally LOCALE_SENGLISHCOUNTRYNAME, before return.
> */
>
> Typo "enumarating".
>

I would not call the order essential, is just meant to try the easier ways
first: is already "ISO" formatted !-> is just a "language" !-> is a full
"language_country" tag.

I take note about "enumarating".

2. Maybe the fail has here:
>
> if (hyphen == NULL || underscore == NULL)
>
> Change || to &&, the logical is wrong?
>

If the Windows locale does not have a hyphen ("aa") *or* the lc_message
does not have an underscore ("Afar"), only a comparison on language is
needed.

3. Why iso_lc_messages[0] = '\0'?
>
> If we go call strchr, soon after, it's a waste.
>

Less code churn, and strchr() againts an empty string did not look too
awful.

I would like to find were the errors come from before sending a new
version, can you reproduce them?

Regards,

Juan José Santamaría Flecha

>
>

On Tue, Apr 21, 2020 at 5:32 PM Juan José Santamaría Flecha
<juanjo(dot)santamaria(at)gmail(dot)com> wrote:
>
> On Tue, Apr 21, 2020 at 12:41 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>>
>> On Tue, Apr 21, 2020 at 12:51 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)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

Em qua., 22 de abr. de 2020 às 08:43, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
escreveu:

> On Tue, Apr 21, 2020 at 5:32 PM Juan José Santamaría Flecha
> <juanjo(dot)santamaria(at)gmail(dot)com> wrote:
> >
> > On Tue, Apr 21, 2020 at 12:41 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)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';
2. Remove vars hyphen and underscore;
3. Avoid call second wcsrchr, if hyphen is not found.

If it's not too much perfectionism.

regards,
Ranier Vilela

On Wed, Apr 22, 2020 at 1:43 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:

> On Tue, Apr 21, 2020 at 5:32 PM Juan José Santamaría Flecha
> <juanjo(dot)santamaria(at)gmail(dot)com> wrote:
> >
> > 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.
>

I am glad we could clear that out, sorry because it was on my hand to
prevent.

> >> 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.

> 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.

> 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?

> 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."
>

LGTM.

> 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.
>

Ok, there is substance for a cleanup 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?
>

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 is worth the effort.

Regards,

Juan José Santamaría Flecha

>
>

On Wed, Apr 22, 2020 at 7:37 PM Ranier Vilela <ranier(dot)vf(at)gmail(dot)com> wrote:
>
> Em qua., 22 de abr. de 2020 às 08:43, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> escreveu:
>>
>> On Tue, Apr 21, 2020 at 5:32 PM Juan José Santamaría Flecha
>> <juanjo(dot)santamaria(at)gmail(dot)com> wrote:
>> >
>> > On Tue, Apr 21, 2020 at 12:41 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)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(dot)santamaria(at)gmail(dot)com> wrote:
>
>
> On Wed, Apr 22, 2020 at 1:43 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)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 is worth 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:30 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:

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

I have focused on improving comments in this version.

> 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.
>

Agreed.

> 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). Please find attached
the output from debug3 logging using both EnumSystemLocalesEx
(lc_messages_EnumSystemLocalesEx.log) and _create_locale
(lc_messages_create_locale.log).

> 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 is worth 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.
>

Ok, the attached version still uses that approach.

Regards,

Juan José Santamaría Flecha

On Thu, Apr 23, 2020 at 5:37 PM Juan José Santamaría Flecha
<juanjo(dot)santamaria(at)gmail(dot)com> wrote:
>
>
> On Thu, Apr 23, 2020 at 5:30 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)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). 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. 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 3:00 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:

>
> 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?
>

If you are trying to view the messages using a CMD, I do not think is
possible unless you have the OS language installed. I read the results from
the log file.

Regards,

Juan José Santamaría Flecha

On Thu, Apr 23, 2020 at 6:49 PM Juan José Santamaría Flecha <
juanjo(dot)santamaria(at)gmail(dot)com> wrote:

>
> On Thu, Apr 23, 2020 at 3:00 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
> wrote:
>
>>
>> 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?
>>
>
> If you are trying to view the messages using a CMD, I do not think is
> possible unless you have the OS language installed. I read the results from
> the log file.
>
I have checked the log file also but still, I am not seeing any changes in
error message language. I am checking two log files one is by enabling
Logging_collector in the conf file and the second is generated using
"pg_ctl -l" option.
I am using windows 10.
Is there another way you are generating the log file?
Did you install any of the locales manually you mentioned in the test file?

Also after initdb I am seeing only following standard locales in the
pg_collation catalog.
postgres=# select * from pg_collation;
oid | collname | collnamespace | collowner | collprovider |
collisdeterministic | collencoding | collcollate | collctype | collversion
-------+-----------+---------------+-----------+--------------+---------------------+--------------+-------------+-----------+-------------
100 | default | 11 | 10 | d | t | -1 | | |
950 | C | 11 | 10 | c | t | -1 | C | C |
951 | POSIX | 11 | 10 | c | t | -1 | POSIX |
POSIX |
12327 | ucs_basic | 11 | 10 | c | t | 6 | C |
C |
(4 rows)

Maybe Postgres is not able to get all the installed locales from the system
in my case. Can you confirm if you are getting different results in
pg_collation?

--
Regards,
Davinder
EnterpriseDB: http://www.enterprisedb.com

On Fri, Apr 24, 2020 at 7:47 AM davinder singh <davindersingh2692(at)gmail(dot)com>
wrote:

> On Thu, Apr 23, 2020 at 6:49 PM Juan José Santamaría Flecha <
> juanjo(dot)santamaria(at)gmail(dot)com> wrote:
>
>> On Thu, Apr 23, 2020 at 3:00 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
>> wrote:
>>
>>>
>>> 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?
>>>
>>
>> If you are trying to view the messages using a CMD, I do not think is
>> possible unless you have the OS language installed. I read the results from
>> the log file.
>>
> I have checked the log file also but still, I am not seeing any changes in
> error message language. I am checking two log files one is by enabling
> Logging_collector in the conf file and the second is generated using
> "pg_ctl -l" option.
> I am using windows 10.
> Is there another way you are generating the log file?
> Did you install any of the locales manually you mentioned in the test file?
> Maybe Postgres is not able to get all the installed locales from the
> system in my case. Can you confirm if you are getting different results in
> pg_collation?
> <http://www.enterprisedb.com/>
>

Hmm, my building environment only has en_US and es_ES installed, and the
db has the same collations.

I am not sure it is a locale problem, the only thing that needed some
configuration on my end to make the build was related to gettext. I got the
libintl library from the PHP repositories [1] (libintl-0.18.3-5,
precompiled at [2]) and the utilities from MinGW (mingw32-libintl
0.18.3.2-2).

[1] https://github.com/winlibs/gettext
[2] https://windows.php.net/downloads/php-sdk/

Regards,

Juan José Santamaría Flecha

On Thu, Apr 23, 2020 at 6:30 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Thu, Apr 23, 2020 at 5:37 PM Juan José Santamaría Flecha
> <juanjo(dot)santamaria(at)gmail(dot)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 Mon, Apr 27, 2020 at 4:50 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:

> Bemba_Zambia
> Bena_Tanzania
> Bulgarian_Bulgaria
> Swedish_Sweden.1252
> Swedish_Sweden
>

I have tested with different locales with codepages including above. There
are few which return different locale code but the error messages in both
the cases are the same. I have attached the test and log files.
But there was one case, where locale code and error messages both are
different.
Portuguese_Brazil.1252

log from [1]
2020-04-28 14:27:39.785 GMT [2284] DEBUG: IsoLocaleName() executed;
locale: "pt"
2020-04-28 14:27:39.787 GMT [2284] ERROR: division by zero
2020-04-28 14:27:39.787 GMT [2284] STATEMENT: Select 1/0;

log from [2]
2020-04-28 14:36:20.666 GMT [14608] DEBUG: IsoLocaleName() executed;
locale: "pt_BR"
2020-04-28 14:36:20.673 GMT [14608] ERRO: divisão por zero
2020-04-28 14:36:20.673 GMT [14608] COMANDO: Select 1/0;

[1] full_locale_lc_message_test_create_locale_1.txt: log generated by using
the old patch (it uses _create_locale API to get locale info)
[2] full_locale_lc_message_test_getlocale_1.txt: log generated using the
patch v13

--
Regards,
Davinder
EnterpriseDB: http://www.enterprisedb.com

On Mon, Apr 27, 2020 at 1:20 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)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.6 and 10-11-12 as of
version 14. The extension is 'txt' not to break the cfbot.

> 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.

LGTM.

Regards,
Juan José Santamaría Flecha

>
>

On Tue, Apr 28, 2020 at 5:16 PM davinder singh <davindersingh2692(at)gmail(dot)com>
wrote:

> On Mon, Apr 27, 2020 at 4:50 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
> wrote:
>
>> Bemba_Zambia
>> Bena_Tanzania
>> Bulgarian_Bulgaria
>> Swedish_Sweden.1252
>> Swedish_Sweden
>>
>
> I have tested with different locales with codepages including above. There
> are few which return different locale code but the error messages in both
> the cases are the same. I have attached the test and log files.
>
But there was one case, where locale code and error messages both are
> different.
> Portuguese_Brazil.1252
>
> log from [1]
> 2020-04-28 14:27:39.785 GMT [2284] DEBUG: IsoLocaleName() executed;
> locale: "pt"
> 2020-04-28 14:27:39.787 GMT [2284] ERROR: division by zero
> 2020-04-28 14:27:39.787 GMT [2284] STATEMENT: Select 1/0;
>
> log from [2]
> 2020-04-28 14:36:20.666 GMT [14608] DEBUG: IsoLocaleName() executed;
> locale: "pt_BR"
> 2020-04-28 14:36:20.673 GMT [14608] ERRO: divisão por zero
> 2020-04-28 14:36:20.673 GMT [14608] COMANDO: Select 1/0;
>

AFAICT, the good result is coming from the new logic.

On Tue, Apr 28, 2020 at 9:38 PM Ranier Vilela <ranier(dot)vf(at)gmail(dot)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.

Regards,

Juan José Santamaría Flecha

On Wed, Apr 29, 2020 at 1:32 AM Ranier Vilela <ranier(dot)vf(at)gmail(dot)com> wrote:
>
> Em ter., 28 de abr. de 2020 às 16:53, Juan José Santamaría Flecha <juanjo(dot)santamaria(at)gmail(dot)com> escreveu:
>>
>>
>>
>> On Tue, Apr 28, 2020 at 9:38 PM Ranier Vilela <ranier(dot)vf(at)gmail(dot)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(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Wed, Apr 29, 2020 at 1:32 AM Ranier Vilela <ranier(dot)vf(at)gmail(dot)com> wrote:
> >
> > Em ter., 28 de abr. de 2020 às 16:53, Juan José Santamaría Flecha <juanjo(dot)santamaria(at)gmail(dot)com> escreveu:
> >>
> >>
> >>
> >> On Tue, Apr 28, 2020 at 9:38 PM Ranier Vilela <ranier(dot)vf(at)gmail(dot)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 Wed, Apr 29, 2020 at 8:24 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:

> BTW, do you see any different results for pt_PT with create_locale
> version or the new patch version being discussed here?
>
No, there is no difference for pt_PT. The difference you are noticing is
because of the previous locale setting.

--
Regards,
Davinder
EnterpriseDB: http://www.enterprisedb.com

On Tue, Apr 28, 2020 at 11:45 PM Juan José Santamaría Flecha <
juanjo(dot)santamaria(at)gmail(dot)com> wrote:

>
> On Tue, Apr 28, 2020 at 5:16 PM davinder singh <
> davindersingh2692(at)gmail(dot)com> wrote:
>
>> I have tested with different locales with codepages including above.
>> There are few which return different locale code but the error messages in
>> both the cases are the same. I have attached the test and log files.
>>
> But there was one case, where locale code and error messages both are
>> different.
>> Portuguese_Brazil.1252
>>
>> log from [1]
>> 2020-04-28 14:27:39.785 GMT [2284] DEBUG: IsoLocaleName() executed;
>> locale: "pt"
>> 2020-04-28 14:27:39.787 GMT [2284] ERROR: division by zero
>> 2020-04-28 14:27:39.787 GMT [2284] STATEMENT: Select 1/0;
>>
>> log from [2]
>> 2020-04-28 14:36:20.666 GMT [14608] DEBUG: IsoLocaleName() executed;
>> locale: "pt_BR"
>> 2020-04-28 14:36:20.673 GMT [14608] ERRO: divisão por zero
>> 2020-04-28 14:36:20.673 GMT [14608] COMANDO: Select 1/0;
>>
>
> AFAICT, the good result is coming from the new logic.
>
Yes, I also feel the same.

--
Regards,
Davinder
EnterpriseDB: http://www.enterprisedb.com

On Tue, Apr 28, 2020 at 11:39 PM Juan José Santamaría Flecha
<juanjo(dot)santamaria(at)gmail(dot)com> wrote:
>
>
> On Mon, Apr 27, 2020 at 1:20 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)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.6 and 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(dot)kapila16(at)gmail(dot)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(dot)kapila16(at)gmail(dot)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 3:50 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:

> On Wed, Apr 29, 2020 at 5:57 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)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 alternative might be a cleaner
approach, I will work on that.

Regards,

Juan José Santamaría Flecha

On Wed, Apr 29, 2020 at 9:36 PM Juan José Santamaría Flecha
<juanjo(dot)santamaria(at)gmail(dot)com> wrote:
>
> On Wed, Apr 29, 2020 at 3:50 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>>
>> On Wed, Apr 29, 2020 at 5:57 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)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 alternative might 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 Thu, Apr 30, 2020 at 5:07 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:

> >
> > I was not aware of how many switches IsoLocaleName() already had before
> trying to backpatch. I think offering an alternative might 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.
>

Please find attached a new version following this approach.

Regards,

Juan José Santamaría Flecha

On Mon, May 4, 2020 at 6:59 PM Juan José Santamaría Flecha
<juanjo(dot)santamaria(at)gmail(dot)com> wrote:
>
> On Thu, Apr 30, 2020 at 5:07 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)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 Tue, May 5, 2020 at 1:34 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)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.

Regards,

Juan José Santamaría Flecha

On Wed, May 6, 2020 at 4:19 AM Juan José Santamaría Flecha
<juanjo(dot)santamaria(at)gmail(dot)com> wrote:
>
> On Tue, May 5, 2020 at 1:34 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)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 6:41 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)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.

Regards,

Juan José Santamaría Flecha

On Wed, May 6, 2020 at 10:11 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:

>
> > 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.
>

I have verified/tested the latest patches for all versions and didn't find
any problem.
--
Regards,
Davinder
EnterpriseDB: http://www.enterprisedb.com

On Wed, May 6, 2020 at 11:01 PM Juan José Santamaría Flecha
<juanjo(dot)santamaria(at)gmail(dot)com> wrote:
>
> On Wed, May 6, 2020 at 6:41 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)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(dot)kapila16(at)gmail(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Amit Kapila <amit(dot)kapila16(at)gmail(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Amit Kapila <amit(dot)kapila16(at)gmail(dot)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] - /developer/roadmap/

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

Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> writes:
> On Thu, May 7, 2020 at 11:57 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> writes:
> > On Thu, May 7, 2020 at 11:57 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(dot)kapila16(at)gmail(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Amit Kapila <amit(dot)kapila16(at)gmail(dot)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

Em qui., 14 de mai. de 2020 às 05:49, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
escreveu:

> On Wed, May 13, 2020 at 7:34 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >
> > Amit Kapila <amit(dot)kapila16(at)gmail(dot)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.
>
Thank you for the commit.

regards,
Ranier Vilela

On Thu, May 14, 2020 at 11:07 AM Ranier Vilela <ranier(dot)vf(at)gmail(dot)com> wrote:

> Em qui., 14 de mai. de 2020 às 05:49, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
> escreveu:
>
>> On Wed, May 13, 2020 at 7:34 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> >
>> > Amit Kapila <amit(dot)kapila16(at)gmail(dot)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.
>>
> Thank you for the commit.
>

Great. Thanks to everyone involved.