Re: [PATCHES] Solve a problem of LC_TIME of windows.

Hi.

I have problem of LC_TIME of windows.(CVS-HEAD)

As for Version 8.3.3. It is edited by wrong gettext and is. (But, it is expressed.)
http://winpg.jp/~saito/pg_work/LC_MESSAGE_CHECK/LC_TIME_PATCH/pg8.3.3-to_char_gettext_format.png

As for Version 8.4. It came to be used by Tom-san in strftime of Native-windowsAPI.
It is good improvement.! But, strftime of Native returns a result by CODEPAGE of
environment of operation by Windows with LC_TIME. In Japanese environment, return
value is SJIS(CP932). Then, SJIS(CP932) can't be chosen by SERVER_ENCODING.:-(
http://winpg.jp/~saito/pg_work/LC_MESSAGE_CHECK/pg84beta-03-to_char.png

Then, I'm proposal patch. It is solved splendidly.
http://winpg.jp/~saito/pg_work/LC_MESSAGE_CHECK/LC_TIME_PATCH/DATECHECK_EUCJP.txt
http://winpg.jp/~saito/pg_work/LC_MESSAGE_CHECK/LC_TIME_PATCH/DATECHECK_UTF8.txt

Regards,
Hiroshi Saito

Hiroshi Saito wrote:
> Hi.
>
> I have problem of LC_TIME of windows.(CVS-HEAD)
>
> As for Version 8.3.3. It is edited by wrong gettext and is. (But, it is
> expressed.)
> http://winpg.jp/~saito/pg_work/LC_MESSAGE_CHECK/LC_TIME_PATCH/pg8.3.3-to_char_gettext_format.png
>
>
> As for Version 8.4. It came to be used by Tom-san in strftime of
> Native-windowsAPI.
> It is good improvement.! But, strftime of Native returns a result by
> CODEPAGE of
> environment of operation by Windows with LC_TIME. In Japanese
> environment, return
> value is SJIS(CP932). Then, SJIS(CP932) can't be chosen by
> SERVER_ENCODING.:-(
> http://winpg.jp/~saito/pg_work/LC_MESSAGE_CHECK/pg84beta-03-to_char.png
>
> Then, I'm proposal patch. It is solved splendidly.
> http://winpg.jp/~saito/pg_work/LC_MESSAGE_CHECK/LC_TIME_PATCH/DATECHECK_EUCJP.txt
>
> http://winpg.jp/~saito/pg_work/LC_MESSAGE_CHECK/LC_TIME_PATCH/DATECHECK_UTF8.txt

In principle, I think this patch looks good.

Do you (or somebody else) have an example where this breaks in an
encoding where I can actually understand the characters, though ;-) That
would make testing a whole lot easier...

Also, the patch needs error checking. strftime() can fail, and the
multibyte conversion functions can certainly fail. That will need to be
added.

//Magnus

Magnus Hagander wrote:

> Also, the patch needs error checking. strftime() can fail, and the
> multibyte conversion functions can certainly fail. That will need to be
> added.

What about this line?

#define STRLEN_MAX 255

Are we really sure the strftime format cannot be longer than that?

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Hi.

----- Original Message -----
From: "Magnus Hagander" <magnus(at)hagander(dot)net>

> In principle, I think this patch looks good.
>
> Do you (or somebody else) have an example where this breaks in an
> encoding where I can actually understand the characters, though ;-) That
> would make testing a whole lot easier...
>
> Also, the patch needs error checking. strftime() can fail, and the
> multibyte conversion functions can certainly fail. That will need to be
> added.

Ok, thanks!

strftime return to 0.
http://msdn.microsoft.com/en-us/library/fe06s4ak(VS.71).aspx
MultiByteToWideChar and WideCharToMultiByte return to GetLastError.
http://msdn.microsoft.com/en-us/library/ms776413(VS.85).aspx

I will proposal the next patch.:-)

BTW, this is SQL for a check.
http://winpg.jp/~saito/pg_work/LC_MESSAGE_CHECK/LC_TIME_PATCH/DATECHECK.sql
Probably, all are included.
http://winpg.jp/~saito/pg_work/LC_MESSAGE_CHECK/LC_TIME_PATCH/LC_TIME_CHECK_LOCALE.sql

Regards,
Hiroshi Saito

Hi.

----- Original Message -----
From: "Alvaro Herrera" <alvherre(at)commandprompt(dot)com>
> What about this line?
>
> #define STRLEN_MAX 255
>
> Are we really sure the strftime format cannot be longer than that?

Ahh, although the place to replace is here, is it said that MAX_L10 N_DATA is suitable?
--
cache_locale_time(void)
{
..
char buf[MAX_L10N_DATA];
..
strftime(buf, MAX_L10N_DATA, "%a", timeinfo);

Regards,
Hiroshi Saito

Hi.

I am sorry to be a very late reaction...

"Hiroshi Saito" <z-saito(at)guitar(dot)ocn(dot)ne(dot)jp> writes:
> From: "Magnus Hagander" <magnus(at)hagander(dot)net>
>> Also, the patch needs error checking. strftime() can fail, and the
>> multibyte conversion functions can certainly fail. That will need to be
>> added.

> I will proposal the next patch.:-)

next patch is this.

Regards,
Hiroshi Saito

Hello, Saito-san:

"Hiroshi Saito" <z-saito(at)guitar(dot)ocn(dot)ne(dot)jp> wrote:
> next patch is this.

I'm reviewing your patch and cleanup some parts:
- Avoid casting to LPWSTR.
- Use pre-defined MAX_L10N_DATA instead of STRLEN_MAX.
I'll send a new version.

BTW, we convert strings multiple times in the function.
Windows mbcs -> UTF16 -> UTF8 -> server_encoding

If we have 100% compatible encoding with Windows,
we could skip UTF16 and UTF8 conversions. i.e,

buflen = strftime(buffer);
result = pg_do_encoding_conversion(buffer, buflen,
GetPlatformEncoding(), GetDatabaseEncoding());

Is it possible to implement GetPlatformEncoding() ?
I think it is also needed to treat non-ascii file path
in COPY, LOAD, archive_command and so on.

Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center

On Mon, Nov 3, 2008 at 8:41 PM, ITAGAKI Takahiro
<itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp> wrote:
> Hello, Saito-san:
>
> "Hiroshi Saito" <z-saito(at)guitar(dot)ocn(dot)ne(dot)jp> wrote:
>> next patch is this.
>
> I'm reviewing your patch and cleanup some parts:

i'm confused, original patch has this signature:
+ conv_strftime(char *src, size_t len, const char *format, const struct tm *tm)

your's has:
+strftime_win32(char *dst, size_t dstlen, const char *format, const
struct tm *tm

you change all src for dst, just a variable name decision but a
radical one... why was that (i honestly doesn't understand this patch
very well ;)?

--
Atentamente,
Jaime Casanova
Soporte y capacitación de PostgreSQL
Asesoría y desarrollo de sistemas
Guayaquil - Ecuador
Cel. +59387171157

"Jaime Casanova" <jcasanov(at)systemguards(dot)com(dot)ec> wrote:

> i'm confused, original patch has this signature:
> + conv_strftime(char *src, size_t len, const char *format, const struct tm *tm)
> your's has:
> +strftime_win32(char *dst, size_t dstlen, const char *format, const

> you change all src for dst, just a variable name decision but a
> radical one... why was that (i honestly doesn't understand this patch
> very well ;)?

That's because the first argument is not an input buffer,
but an output buffer. MSDN also calls it 'strDest'.
http://msdn.microsoft.com/en-us/library/fe06s4ak(VS.71).aspx
Linux manpage calls it 's', but I think it means String, not Src.
http://man.cx/strftime

If you can review the patch, please use the attached one instead.
I modified it in response to the discussion of pg_do_encoding_conversion.

BTW, I cannot understand the comment in the function head,

+ * result is obtained by locale setup of LC_TIME in the environment
+ * of windows at present CP_ACP. Therefore, conversion is needed
+ * for SERVER_ENCODING. SJIS which is not especially made to server
+ * encoding in Japan returns.

but it probably says:

----
strftime in Windows returns in CP_ACP encoding, but it could be
different from SERVER_ENCODING. Especially, Windows Japanese edition
requires conversions because it uses SJIS as CP_ACP, but we don't
support SJIS as a server encoding.
----

I hope you would review my English not only C ;-)

Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center

Hi ITAGAKI-san.

Sorry, very late reaction..
I lost time resources in an individual my machine trouble and busyness.:-(
Now, I appreciate your exact work. ! Then, I desire the best patch for PostgreSQL.
Probably, I think that it is finally helpful in not a problem of only Japan but many countries.
Tom-san, and Alvaro-san, Magnus-san understands the essence of this problem.
Therefore, the suggestion is expected for me.

Anyway, thank you very much.!!

Regards,
Hiroshi Saito

----- Original Message -----
From: "ITAGAKI Takahiro" <itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp>

>
> "Jaime Casanova" <jcasanov(at)systemguards(dot)com(dot)ec> wrote:
>
>> i'm confused, original patch has this signature:
>> + conv_strftime(char *src, size_t len, const char *format, const struct tm *tm)
>> your's has:
>> +strftime_win32(char *dst, size_t dstlen, const char *format, const
>
>> you change all src for dst, just a variable name decision but a
>> radical one... why was that (i honestly doesn't understand this patch
>> very well ;)?
>
> That's because the first argument is not an input buffer,
> but an output buffer. MSDN also calls it 'strDest'.
> http://msdn.microsoft.com/en-us/library/fe06s4ak(VS.71).aspx
> Linux manpage calls it 's', but I think it means String, not Src.
> http://man.cx/strftime
>
> If you can review the patch, please use the attached one instead.
> I modified it in response to the discussion of pg_do_encoding_conversion.
>
>
> BTW, I cannot understand the comment in the function head,
>
> + * result is obtained by locale setup of LC_TIME in the environment
> + * of windows at present CP_ACP. Therefore, conversion is needed
> + * for SERVER_ENCODING. SJIS which is not especially made to server
> + * encoding in Japan returns.
>
> but it probably says:
>
> ----
> strftime in Windows returns in CP_ACP encoding, but it could be
> different from SERVER_ENCODING. Especially, Windows Japanese edition
> requires conversions because it uses SJIS as CP_ACP, but we don't
> support SJIS as a server encoding.
> ----
>
> I hope you would review my English not only C ;-)
>
> Regards,
> ---
> ITAGAKI Takahiro
> NTT Open Source Software Center
>
>

Hi.

All suggestion is appropriate and has been checked.

CVS-HEAD was examined by MinGW.
$ make check NO_LOCALE=true
...
=======================
All 118 tests passed.
=======================

Then, It continues and a review is desired.
Thanks!

Regatrds,
Hiroshi Saito

----- Original Message -----
From: "Hiroshi Saito" <z-saito(at)guitar(dot)ocn(dot)ne(dot)jp>

> Hi ITAGAKI-san.
>
> Sorry, very late reaction..
> I lost time resources in an individual my machine trouble and busyness.:-(
> Now, I appreciate your exact work. ! Then, I desire the best patch for PostgreSQL.
> Probably, I think that it is finally helpful in not a problem of only Japan but many countries.
> Tom-san, and Alvaro-san, Magnus-san understands the essence of this problem.
> Therefore, the suggestion is expected for me.
>
> Anyway, thank you very much.!!
>
> Regards,
> Hiroshi Saito
>
> ----- Original Message -----
> From: "ITAGAKI Takahiro" <itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp>
>
>
>>
>> "Jaime Casanova" <jcasanov(at)systemguards(dot)com(dot)ec> wrote:
>>
>>> i'm confused, original patch has this signature:
>>> + conv_strftime(char *src, size_t len, const char *format, const struct tm *tm)
>>> your's has:
>>> +strftime_win32(char *dst, size_t dstlen, const char *format, const
>>
>>> you change all src for dst, just a variable name decision but a
>>> radical one... why was that (i honestly doesn't understand this patch
>>> very well ;)?
>>
>> That's because the first argument is not an input buffer,
>> but an output buffer. MSDN also calls it 'strDest'.
>> http://msdn.microsoft.com/en-us/library/fe06s4ak(VS.71).aspx
>> Linux manpage calls it 's', but I think it means String, not Src.
>> http://man.cx/strftime
>>
>> If you can review the patch, please use the attached one instead.
>> I modified it in response to the discussion of pg_do_encoding_conversion.
>>
>>
>> BTW, I cannot understand the comment in the function head,
>>
>> + * result is obtained by locale setup of LC_TIME in the environment
>> + * of windows at present CP_ACP. Therefore, conversion is needed
>> + * for SERVER_ENCODING. SJIS which is not especially made to server
>> + * encoding in Japan returns.
>>
>> but it probably says:
>>
>> ----
>> strftime in Windows returns in CP_ACP encoding, but it could be
>> different from SERVER_ENCODING. Especially, Windows Japanese edition
>> requires conversions because it uses SJIS as CP_ACP, but we don't
>> support SJIS as a server encoding.
>> ----
>>
>> I hope you would review my English not only C ;-)
>>
>> Regards,
>> ---
>> ITAGAKI Takahiro
>> NTT Open Source Software Center
>>
>>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers

On Sun, Nov 16, 2008 at 8:36 AM, Hiroshi Saito <z-saito(at)guitar(dot)ocn(dot)ne(dot)jp> wrote:
> Hi.
>
> Then, It continues and a review is desired. Thanks!
>

In http://msdn.microsoft.com/en-us/library/fe06s4ak(VS.71).aspx says:
"""
Return Value
strftime returns the number of characters placed in strDest and
wcsftime returns the corresponding number of wide characters.
If the total number of characters, including the terminating null, is
more than maxsize, both strftime and wcsftime return 0 and the
contents of strDest is indeterminate.
"""

If i'm reading it right, the patch should contain something like:

if (len > dstlen)
{
return 0;
}

--
Atentamente,
Jaime Casanova
Soporte y capacitación de PostgreSQL
Asesoría y desarrollo de sistemas
Guayaquil - Ecuador
Cel. +59387171157

Hi Jaime-san.

Thank you for a review.

I think this purpose to return the value which should originally obtain strftime
by only replacing here. Then, I think that it is a superfluous reaction.

However, some consideration may be necessities.

Regards,
Hiroshi Saito

----- Original Message -----
From: "Jaime Casanova" <jcasanov(at)systemguards(dot)com(dot)ec>

On Sun, Nov 16, 2008 at 8:36 AM, Hiroshi Saito <z-saito(at)guitar(dot)ocn(dot)ne(dot)jp> wrote:
> Hi.
>
> Then, It continues and a review is desired. Thanks!
>

In http://msdn.microsoft.com/en-us/library/fe06s4ak(VS.71).aspx says:
"""
Return Value
strftime returns the number of characters placed in strDest and
wcsftime returns the corresponding number of wide characters.
If the total number of characters, including the terminating null, is
more than maxsize, both strftime and wcsftime return 0 and the
contents of strDest is indeterminate.
"""

If i'm reading it right, the patch should contain something like:

if (len > dstlen)
{
return 0;
}

--
Atentamente,
Jaime Casanova
Soporte y capacitación de PostgreSQL
Asesoría y desarrollo de sistemas
Guayaquil - Ecuador
Cel. +59387171157

The way I read this patch we do:
1) Get the time in CP_ACP
2) Convert to UTF16
3) Convert to UTF8
4) If necessary, convert to the charset in the database

We could be more efficient by at least folding 1 and 2 into a single
step, which means getting it in UTF16 from the beginning. How about
attached patch? It builds, but please verify that it fixes the problem.

(I've updated the comment as well)

Attached pg_locale_utf16.patch. I'm also attaching
pg_locale_diffdiff.patch which contains the changes I've made against
your patch only.

//Magnus

Hiroshi Saito wrote:
> Hi.
>
> All suggestion is appropriate and has been checked.
>
> CVS-HEAD was examined by MinGW. $ make check NO_LOCALE=true
> ...
> =======================
> All 118 tests passed. =======================
>
> Then, It continues and a review is desired. Thanks!
>
> Regatrds,
> Hiroshi Saito
>
> ----- Original Message ----- From: "Hiroshi Saito"
> <z-saito(at)guitar(dot)ocn(dot)ne(dot)jp>
>
>
>> Hi ITAGAKI-san.
>>
>> Sorry, very late reaction..
>> I lost time resources in an individual my machine trouble and
>> busyness.:-(
>> Now, I appreciate your exact work. ! Then, I desire the best patch for
>> PostgreSQL. Probably, I think that it is finally helpful in not a
>> problem of only Japan but many countries. Tom-san, and Alvaro-san,
>> Magnus-san understands the essence of this problem. Therefore, the
>> suggestion is expected for me.
>> Anyway, thank you very much.!!
>>
>> Regards,
>> Hiroshi Saito
>>
>> ----- Original Message ----- From: "ITAGAKI Takahiro"
>> <itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp>
>>
>>
>>>
>>> "Jaime Casanova" <jcasanov(at)systemguards(dot)com(dot)ec> wrote:
>>>
>>>> i'm confused, original patch has this signature:
>>>> + conv_strftime(char *src, size_t len, const char *format, const
>>>> struct tm *tm)
>>>> your's has:
>>>> +strftime_win32(char *dst, size_t dstlen, const char *format, const
>>>
>>>> you change all src for dst, just a variable name decision but a
>>>> radical one... why was that (i honestly doesn't understand this patch
>>>> very well ;)?
>>>
>>> That's because the first argument is not an input buffer,
>>> but an output buffer. MSDN also calls it 'strDest'.
>>> http://msdn.microsoft.com/en-us/library/fe06s4ak(VS.71).aspx
>>> Linux manpage calls it 's', but I think it means String, not Src.
>>> http://man.cx/strftime
>>>
>>> If you can review the patch, please use the attached one instead.
>>> I modified it in response to the discussion of
>>> pg_do_encoding_conversion.
>>>
>>>
>>> BTW, I cannot understand the comment in the function head,
>>>
>>> + * result is obtained by locale setup of LC_TIME in the environment
>>> + * of windows at present CP_ACP. Therefore, conversion is needed
>>> + * for SERVER_ENCODING. SJIS which is not especially made to server
>>> + * encoding in Japan returns.
>>> but it probably says:
>>>
>>> ----
>>> strftime in Windows returns in CP_ACP encoding, but it could be
>>> different from SERVER_ENCODING. Especially, Windows Japanese edition
>>> requires conversions because it uses SJIS as CP_ACP, but we don't
>>> support SJIS as a server encoding.
>>> ----
>>>
>>> I hope you would review my English not only C ;-)
>>>
>>> Regards,
>>> ---
>>> ITAGAKI Takahiro
>>> NTT Open Source Software Center
>>>
>>>
>>
>> --
>> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
>> To make changes to your subscription:
>> http://www.postgresql.org/mailpref/pgsql-hackers

Hi Magnus-san.

Um, Though regrettable, it is not in a good state.....
http://winpg.jp/~saito/pg_work/LC_MESSAGE_CHECK/LC_TIME_PATCH/Mugnus-patch.png

----- Original Message -----
From: "Magnus Hagander" <magnus(at)hagander(dot)net>

> The way I read this patch we do:
> 1) Get the time in CP_ACP
> 2) Convert to UTF16
> 3) Convert to UTF8
> 4) If necessary, convert to the charset in the database
>
> We could be more efficient by at least folding 1 and 2 into a single
> step, which means getting it in UTF16 from the beginning. How about
> attached patch? It builds, but please verify that it fixes the problem.
>
> (I've updated the comment as well)
>
> Attached pg_locale_utf16.patch. I'm also attaching
> pg_locale_diffdiff.patch which contains the changes I've made against
> your patch only.
>
> //Magnus
>
>
> Hiroshi Saito wrote:
>> Hi.
>>
>> All suggestion is appropriate and has been checked.
>>
>> CVS-HEAD was examined by MinGW. $ make check NO_LOCALE=true
>> ...
>> =======================
>> All 118 tests passed. =======================
>>
>> Then, It continues and a review is desired. Thanks!
>>
>> Regatrds,
>> Hiroshi Saito
>>
>> ----- Original Message ----- From: "Hiroshi Saito"
>> <z-saito(at)guitar(dot)ocn(dot)ne(dot)jp>
>>
>>
>>> Hi ITAGAKI-san.
>>>
>>> Sorry, very late reaction..
>>> I lost time resources in an individual my machine trouble and
>>> busyness.:-(
>>> Now, I appreciate your exact work. ! Then, I desire the best patch for
>>> PostgreSQL. Probably, I think that it is finally helpful in not a
>>> problem of only Japan but many countries. Tom-san, and Alvaro-san,
>>> Magnus-san understands the essence of this problem. Therefore, the
>>> suggestion is expected for me.
>>> Anyway, thank you very much.!!
>>>
>>> Regards,
>>> Hiroshi Saito
>>>
>>> ----- Original Message ----- From: "ITAGAKI Takahiro"
>>> <itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp>
>>>
>>>
>>>>
>>>> "Jaime Casanova" <jcasanov(at)systemguards(dot)com(dot)ec> wrote:
>>>>
>>>>> i'm confused, original patch has this signature:
>>>>> + conv_strftime(char *src, size_t len, const char *format, const
>>>>> struct tm *tm)
>>>>> your's has:
>>>>> +strftime_win32(char *dst, size_t dstlen, const char *format, const
>>>>
>>>>> you change all src for dst, just a variable name decision but a
>>>>> radical one... why was that (i honestly doesn't understand this patch
>>>>> very well ;)?
>>>>
>>>> That's because the first argument is not an input buffer,
>>>> but an output buffer. MSDN also calls it 'strDest'.
>>>> http://msdn.microsoft.com/en-us/library/fe06s4ak(VS.71).aspx
>>>> Linux manpage calls it 's', but I think it means String, not Src.
>>>> http://man.cx/strftime
>>>>
>>>> If you can review the patch, please use the attached one instead.
>>>> I modified it in response to the discussion of
>>>> pg_do_encoding_conversion.
>>>>
>>>>
>>>> BTW, I cannot understand the comment in the function head,
>>>>
>>>> + * result is obtained by locale setup of LC_TIME in the environment
>>>> + * of windows at present CP_ACP. Therefore, conversion is needed
>>>> + * for SERVER_ENCODING. SJIS which is not especially made to server
>>>> + * encoding in Japan returns.
>>>> but it probably says:
>>>>
>>>> ----
>>>> strftime in Windows returns in CP_ACP encoding, but it could be
>>>> different from SERVER_ENCODING. Especially, Windows Japanese edition
>>>> requires conversions because it uses SJIS as CP_ACP, but we don't
>>>> support SJIS as a server encoding.
>>>> ----
>>>>
>>>> I hope you would review my English not only C ;-)
>>>>
>>>> Regards,
>>>> ---
>>>> ITAGAKI Takahiro
>>>> NTT Open Source Software Center
>>>>
>>>>
>>>
>>> --
>>> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
>>> To make changes to your subscription:
>>> http://www.postgresql.org/mailpref/pgsql-hackers
>
>

--------------------------------------------------------------------------------

> *** a/src/backend/utils/adt/pg_locale.c
> --- b/src/backend/utils/adt/pg_locale.c
> ***************
> *** 54,59 ****
> --- 54,60 ----
> #include "utils/memutils.h"
> #include "utils/pg_locale.h"
>
> + #include "mb/pg_wchar.h"
>
> #define MAX_L10N_DATA 80
>
> ***************
> *** 452,457 **** PGLC_localeconv(void)
> --- 453,507 ----
> return &CurrentLocaleConv;
> }
>
> + #ifdef WIN32
> + /*
> + * On win32, strftime() returns the encoding in CP_ACP, which is likely
> + * different from SERVER_ENCODING. This is especially important in Japanese
> + * versions of Windows which will use SJIS encoding, which we don't support
> + * as a server encoding.
> + *
> + * Replace strftime() with a version that gets the string in UTF16 and then
> + * converts it to the appropriate encoding as necessary.
> + */
> + static size_t
> + strftime_win32(char *dst, size_t dstlen, const char *format, const struct tm *tm)
> + {
> + size_t len;
> + wchar_t wbuf[MAX_L10N_DATA];
> + int encoding;
> +
> + encoding = GetDatabaseEncoding();
> + if (encoding == PG_SQL_ASCII)
> + return len;
> +
> + len = wcsftime(wbuf, sizeof(wbuf), format, tm);
> + if (len == 0)
> + /* strftime called failed - return 0 with the contents of dst unspecified */
> + return 0;
> +
> + len = WideCharToMultiByte(CP_UTF8, 0, wbuf, len, dst, dstlen, NULL, NULL);
> + if (len == 0)
> + ereport(ERROR,
> + (errmsg("could not convert string to UTF-8:error %lu", GetLastError())));
> +
> + dst[len] = '\0';
> + if (encoding != PG_UTF8)
> + {
> + char *convstr = pg_do_encoding_conversion(dst, len, PG_UTF8, encoding);
> + if (dst != convstr)
> + {
> + StrNCpy(dst, convstr, dstlen);
> + len = strlen(dst);
> + }
> + }
> +
> + return len;
> + }
> +
> + #define strftime(a,b,c,d) strftime_win32(a,b,c,d)
> +
> + #endif /* WIN32 */
> +
>
> /*
> * Update the lc_time localization cache variables if needed.
>

--------------------------------------------------------------------------------

> diff --git a/src/backend/utils/adt/pg_locale.c b/src/backend/utils/adt/pg_locale.c
> index f78a80f..c37ddd5 100644
> --- a/src/backend/utils/adt/pg_locale.c
> +++ b/src/backend/utils/adt/pg_locale.c
> @@ -455,10 +455,13 @@ PGLC_localeconv(void)
>
> #ifdef WIN32
> /*
> - * result is obtained by locale setup of LC_TIME in the environment
> - * of windows at present CP_ACP. Therefore, conversion is needed
> - * for SERVER_ENCODING. SJIS which is not especially made to server
> - * encoding in Japan returns.
> + * On win32, strftime() returns the encoding in CP_ACP, which is likely
> + * different from SERVER_ENCODING. This is especially important in Japanese
> + * versions of Windows which will use SJIS encoding, which we don't support
> + * as a server encoding.
> + *
> + * Replace strftime() with a version that gets the string in UTF16 and then
> + * converts it to the appropriate encoding as necessary.
> */
> static size_t
> strftime_win32(char *dst, size_t dstlen, const char *format, const struct tm *tm)
> @@ -467,19 +470,19 @@ strftime_win32(char *dst, size_t dstlen, const char *format, const struct
> tm *tm
> wchar_t wbuf[MAX_L10N_DATA];
> int encoding;
>
> - len = strftime(dst, dstlen, format, tm);
> encoding = GetDatabaseEncoding();
> if (encoding == PG_SQL_ASCII)
> return len;
>
> - len = MultiByteToWideChar(CP_ACP, 0, dst, len, wbuf, MAX_L10N_DATA);
> + len = wcsftime(wbuf, sizeof(wbuf), format, tm);
> if (len == 0)
> - ereport(ERROR,
> - (errmsg("could not convert string to wide character:error %lu", GetLastError())));
> + /* strftime called failed - return 0 with the contents of dst unspecified */
> + return 0;
> +
> len = WideCharToMultiByte(CP_UTF8, 0, wbuf, len, dst, dstlen, NULL, NULL);
> if (len == 0)
> ereport(ERROR,
> - (errmsg("could not convert wide character to UTF-8:error %lu", GetLastError())));
> + (errmsg("could not convert string to UTF-8:error %lu", GetLastError())));
>
> dst[len] = '\0';
> if (encoding != PG_UTF8)
>

--------------------------------------------------------------------------------

>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

Magnus Hagander <magnus(at)hagander(dot)net> writes:
> *** a/src/backend/utils/adt/pg_locale.c
> --- b/src/backend/utils/adt/pg_locale.c
> ***************
> *** 54,59 ****
> --- 54,60 ----
> #include "utils/memutils.h"
> #include "utils/pg_locale.h"

> + #include "mb/pg_wchar.h"

> #define MAX_L10N_DATA 80

Please stick to the convention of including include files in
alphabetical order.

> + strftime_win32(char *dst, size_t dstlen, const char *format, const struct tm *tm)
> + {
> + size_t len;
> + wchar_t wbuf[MAX_L10N_DATA];
> + int encoding;
> +
> + encoding = GetDatabaseEncoding();
> + if (encoding == PG_SQL_ASCII)
> + return len;

Surely this is returning an uninitialized variable, not to mention
failing to accomplish any of the goals of the function. I don't think
breaking things completely for SQL_ASCII was part of the plan.

> + ereport(ERROR,
> + (errmsg("could not convert string to UTF-8:error %lu", GetLastError())));

This is not exactly per message style guidelines. Maybe it's just a
can't-happen case, but if so make it elog not ereport.

regards, tom lane

Tom Lane wrote:
> Magnus Hagander <magnus(at)hagander(dot)net> writes:
>> *** a/src/backend/utils/adt/pg_locale.c
>> --- b/src/backend/utils/adt/pg_locale.c
>> ***************
>> *** 54,59 ****
>> --- 54,60 ----
>> #include "utils/memutils.h"
>> #include "utils/pg_locale.h"
>
>> + #include "mb/pg_wchar.h"
>
>> #define MAX_L10N_DATA 80
>
> Please stick to the convention of including include files in
> alphabetical order.

Check.

>> + strftime_win32(char *dst, size_t dstlen, const char *format, const struct tm *tm)
>> + {
>> + size_t len;
>> + wchar_t wbuf[MAX_L10N_DATA];
>> + int encoding;
>> +
>> + encoding = GetDatabaseEncoding();
>> + if (encoding == PG_SQL_ASCII)
>> + return len;
>
> Surely this is returning an uninitialized variable, not to mention
> failing to accomplish any of the goals of the function. I don't think
> breaking things completely for SQL_ASCII was part of the plan.

Gah, true, that's me breaking it. That was correct in Hiroshi-san's
patch. My bad, sorry.

>
>> + ereport(ERROR,
>> + (errmsg("could not convert string to UTF-8:error %lu", GetLastError())));
>
> This is not exactly per message style guidelines. Maybe it's just a
> can't-happen case, but if so make it elog not ereport.

Check.

//Magnus

Does it work when you're not using C-locale? This looks like the
codepath Tom pointed out was wrong.

//Magnus

Hiroshi Saito wrote:
> Hi Magnus-san.
>
> Um, Though regrettable, it is not in a good state.....
> http://winpg.jp/~saito/pg_work/LC_MESSAGE_CHECK/LC_TIME_PATCH/Mugnus-patch.png
>
>
> ----- Original Message ----- From: "Magnus Hagander" <magnus(at)hagander(dot)net>
>
>
>> The way I read this patch we do:
>> 1) Get the time in CP_ACP
>> 2) Convert to UTF16
>> 3) Convert to UTF8
>> 4) If necessary, convert to the charset in the database
>>
>> We could be more efficient by at least folding 1 and 2 into a single
>> step, which means getting it in UTF16 from the beginning. How about
>> attached patch? It builds, but please verify that it fixes the problem.
>>
>> (I've updated the comment as well)
>>
>> Attached pg_locale_utf16.patch. I'm also attaching
>> pg_locale_diffdiff.patch which contains the changes I've made against
>> your patch only.
>>
>> //Magnus
>>
>>
>> Hiroshi Saito wrote:
>>> Hi.
>>>
>>> All suggestion is appropriate and has been checked.
>>>
>>> CVS-HEAD was examined by MinGW. $ make check NO_LOCALE=true
>>> ...
>>> =======================
>>> All 118 tests passed. =======================
>>>
>>> Then, It continues and a review is desired. Thanks!
>>>
>>> Regatrds,
>>> Hiroshi Saito
>>>
>>> ----- Original Message ----- From: "Hiroshi Saito"
>>> <z-saito(at)guitar(dot)ocn(dot)ne(dot)jp>
>>>
>>>
>>>> Hi ITAGAKI-san.
>>>>
>>>> Sorry, very late reaction..
>>>> I lost time resources in an individual my machine trouble and
>>>> busyness.:-(
>>>> Now, I appreciate your exact work. ! Then, I desire the best patch for
>>>> PostgreSQL. Probably, I think that it is finally helpful in not a
>>>> problem of only Japan but many countries. Tom-san, and Alvaro-san,
>>>> Magnus-san understands the essence of this problem. Therefore, the
>>>> suggestion is expected for me.
>>>> Anyway, thank you very much.!!
>>>>
>>>> Regards,
>>>> Hiroshi Saito
>>>>
>>>> ----- Original Message ----- From: "ITAGAKI Takahiro"
>>>> <itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp>
>>>>
>>>>
>>>>>
>>>>> "Jaime Casanova" <jcasanov(at)systemguards(dot)com(dot)ec> wrote:
>>>>>
>>>>>> i'm confused, original patch has this signature:
>>>>>> + conv_strftime(char *src, size_t len, const char *format, const
>>>>>> struct tm *tm)
>>>>>> your's has:
>>>>>> +strftime_win32(char *dst, size_t dstlen, const char *format, const
>>>>>
>>>>>> you change all src for dst, just a variable name decision but a
>>>>>> radical one... why was that (i honestly doesn't understand this patch
>>>>>> very well ;)?
>>>>>
>>>>> That's because the first argument is not an input buffer,
>>>>> but an output buffer. MSDN also calls it 'strDest'.
>>>>> http://msdn.microsoft.com/en-us/library/fe06s4ak(VS.71).aspx
>>>>> Linux manpage calls it 's', but I think it means String, not Src.
>>>>> http://man.cx/strftime
>>>>>
>>>>> If you can review the patch, please use the attached one instead.
>>>>> I modified it in response to the discussion of
>>>>> pg_do_encoding_conversion.
>>>>>
>>>>>
>>>>> BTW, I cannot understand the comment in the function head,
>>>>>
>>>>> + * result is obtained by locale setup of LC_TIME in the environment
>>>>> + * of windows at present CP_ACP. Therefore, conversion is needed
>>>>> + * for SERVER_ENCODING. SJIS which is not especially made to server
>>>>> + * encoding in Japan returns.
>>>>> but it probably says:
>>>>>
>>>>> ----
>>>>> strftime in Windows returns in CP_ACP encoding, but it could be
>>>>> different from SERVER_ENCODING. Especially, Windows Japanese edition
>>>>> requires conversions because it uses SJIS as CP_ACP, but we don't
>>>>> support SJIS as a server encoding.
>>>>> ----
>>>>>
>>>>> I hope you would review my English not only C ;-)
>>>>>
>>>>> Regards,
>>>>> ---
>>>>> ITAGAKI Takahiro
>>>>> NTT Open Source Software Center
>>>>>
>>>>>
>>>>
>>>> --
>>>> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
>>>> To make changes to your subscription:
>>>> http://www.postgresql.org/mailpref/pgsql-hackers
>>
>>
>
>
> --------------------------------------------------------------------------------
>
>
>
>> *** a/src/backend/utils/adt/pg_locale.c
>> --- b/src/backend/utils/adt/pg_locale.c
>> ***************
>> *** 54,59 ****
>> --- 54,60 ----
>> #include "utils/memutils.h"
>> #include "utils/pg_locale.h"
>>
>> + #include "mb/pg_wchar.h"
>>
>> #define MAX_L10N_DATA 80
>>
>> ***************
>> *** 452,457 **** PGLC_localeconv(void)
>> --- 453,507 ----
>> return &CurrentLocaleConv;
>> }
>>
>> + #ifdef WIN32
>> + /*
>> + * On win32, strftime() returns the encoding in CP_ACP, which is likely
>> + * different from SERVER_ENCODING. This is especially important in
>> Japanese
>> + * versions of Windows which will use SJIS encoding, which we don't
>> support
>> + * as a server encoding.
>> + *
>> + * Replace strftime() with a version that gets the string in UTF16
>> and then
>> + * converts it to the appropriate encoding as necessary.
>> + */
>> + static size_t
>> + strftime_win32(char *dst, size_t dstlen, const char *format, const
>> struct tm *tm)
>> + {
>> + size_t len;
>> + wchar_t wbuf[MAX_L10N_DATA];
>> + int encoding;
>> +
>> + encoding = GetDatabaseEncoding();
>> + if (encoding == PG_SQL_ASCII)
>> + return len;
>> +
>> + len = wcsftime(wbuf, sizeof(wbuf), format, tm);
>> + if (len == 0)
>> + /* strftime called failed - return 0 with the contents of dst
>> unspecified */
>> + return 0;
>> +
>> + len = WideCharToMultiByte(CP_UTF8, 0, wbuf, len, dst, dstlen, NULL,
>> NULL);
>> + if (len == 0)
>> + ereport(ERROR,
>> + (errmsg("could not convert string to UTF-8:error %lu",
>> GetLastError())));
>> +
>> + dst[len] = '\0';
>> + if (encoding != PG_UTF8)
>> + {
>> + char *convstr = pg_do_encoding_conversion(dst, len, PG_UTF8, encoding);
>> + if (dst != convstr)
>> + {
>> + StrNCpy(dst, convstr, dstlen);
>> + len = strlen(dst);
>> + }
>> + }
>> +
>> + return len;
>> + }
>> +
>> + #define strftime(a,b,c,d) strftime_win32(a,b,c,d)
>> +
>> + #endif /* WIN32 */
>> +
>>
>> /*
>> * Update the lc_time localization cache variables if needed.
>>
>
>
> --------------------------------------------------------------------------------
>
>
>
>> diff --git a/src/backend/utils/adt/pg_locale.c
>> b/src/backend/utils/adt/pg_locale.c
>> index f78a80f..c37ddd5 100644
>> --- a/src/backend/utils/adt/pg_locale.c
>> +++ b/src/backend/utils/adt/pg_locale.c
>> @@ -455,10 +455,13 @@ PGLC_localeconv(void)
>>
>> #ifdef WIN32
>> /*
>> - * result is obtained by locale setup of LC_TIME in the environment
>> - * of windows at present CP_ACP. Therefore, conversion is needed
>> - * for SERVER_ENCODING. SJIS which is not especially made to server
>> - * encoding in Japan returns.
>> + * On win32, strftime() returns the encoding in CP_ACP, which is likely
>> + * different from SERVER_ENCODING. This is especially important in
>> Japanese
>> + * versions of Windows which will use SJIS encoding, which we don't
>> support
>> + * as a server encoding.
>> + *
>> + * Replace strftime() with a version that gets the string in UTF16
>> and then
>> + * converts it to the appropriate encoding as necessary.
>> */
>> static size_t
>> strftime_win32(char *dst, size_t dstlen, const char *format, const
>> struct tm *tm)
>> @@ -467,19 +470,19 @@ strftime_win32(char *dst, size_t dstlen, const
>> char *format, const struct tm *tm
>> wchar_t wbuf[MAX_L10N_DATA];
>> int encoding;
>>
>> - len = strftime(dst, dstlen, format, tm);
>> encoding = GetDatabaseEncoding();
>> if (encoding == PG_SQL_ASCII)
>> return len;
>>
>> - len = MultiByteToWideChar(CP_ACP, 0, dst, len, wbuf, MAX_L10N_DATA);
>> + len = wcsftime(wbuf, sizeof(wbuf), format, tm);
>> if (len == 0)
>> - ereport(ERROR,
>> - (errmsg("could not convert string to wide character:error %lu",
>> GetLastError())));
>> + /* strftime called failed - return 0 with the contents of dst
>> unspecified */
>> + return 0;
>> +
>> len = WideCharToMultiByte(CP_UTF8, 0, wbuf, len, dst, dstlen, NULL,
>> NULL);
>> if (len == 0)
>> ereport(ERROR,
>> - (errmsg("could not convert wide character to UTF-8:error %lu",
>> GetLastError())));
>> + (errmsg("could not convert string to UTF-8:error %lu",
>> GetLastError())));
>>
>> dst[len] = '\0';
>> if (encoding != PG_UTF8)
>>
>
>
> --------------------------------------------------------------------------------
>
>
>
>>
>> --
>> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
>> To make changes to your subscription:
>> http://www.postgresql.org/mailpref/pgsql-hackers
>>

Magnus Hagander wrote:
> Tom Lane wrote:
>> Magnus Hagander <magnus(at)hagander(dot)net> writes:
>>> *** a/src/backend/utils/adt/pg_locale.c
>>> --- b/src/backend/utils/adt/pg_locale.c
>>> ***************
>>> *** 54,59 ****
>>> --- 54,60 ----
>>> #include "utils/memutils.h"
>>> #include "utils/pg_locale.h"
>>
>>> + #include "mb/pg_wchar.h"
>>
>>> #define MAX_L10N_DATA 80
>> Please stick to the convention of including include files in
>> alphabetical order.
>
> Check.
>
>
>>> + strftime_win32(char *dst, size_t dstlen, const char *format, const struct tm *tm)
>>> + {
>>> + size_t len;
>>> + wchar_t wbuf[MAX_L10N_DATA];
>>> + int encoding;
>>> +
>>> + encoding = GetDatabaseEncoding();
>>> + if (encoding == PG_SQL_ASCII)
>>> + return len;
>> Surely this is returning an uninitialized variable, not to mention
>> failing to accomplish any of the goals of the function. I don't think
>> breaking things completely for SQL_ASCII was part of the plan.
>
> Gah, true, that's me breaking it. That was correct in Hiroshi-san's
> patch. My bad, sorry.
>
>
>>> + ereport(ERROR,
>>> + (errmsg("could not convert string to UTF-8:error %lu", GetLastError())));
>> This is not exactly per message style guidelines. Maybe it's just a
>> can't-happen case, but if so make it elog not ereport.
>
> Check.

Forgot the attachment.

//Magnus

Hi Magnus-san.

Umm, format operand seems to be a wide character sequence.

----- Original Message -----
From: "Magnus Hagander" <magnus(at)hagander(dot)net>

> Magnus Hagander wrote:
>> Tom Lane wrote:
>>> Magnus Hagander <magnus(at)hagander(dot)net> writes:
>>>> *** a/src/backend/utils/adt/pg_locale.c
>>>> --- b/src/backend/utils/adt/pg_locale.c
>>>> ***************
>>>> *** 54,59 ****
>>>> --- 54,60 ----
>>>> #include "utils/memutils.h"
>>>> #include "utils/pg_locale.h"
>>>
>>>> + #include "mb/pg_wchar.h"
>>>
>>>> #define MAX_L10N_DATA 80
>>> Please stick to the convention of including include files in
>>> alphabetical order.
>>
>> Check.
>>
>>
>>>> + strftime_win32(char *dst, size_t dstlen, const char *format, const struct tm *tm)
>>>> + {
>>>> + size_t len;
>>>> + wchar_t wbuf[MAX_L10N_DATA];
>>>> + int encoding;
>>>> +
>>>> + encoding = GetDatabaseEncoding();
>>>> + if (encoding == PG_SQL_ASCII)
>>>> + return len;
>>> Surely this is returning an uninitialized variable, not to mention
>>> failing to accomplish any of the goals of the function. I don't think
>>> breaking things completely for SQL_ASCII was part of the plan.
>>
>> Gah, true, that's me breaking it. That was correct in Hiroshi-san's
>> patch. My bad, sorry.
>>
>>
>>>> + ereport(ERROR,
>>>> + (errmsg("could not convert string to UTF-8:error %lu", GetLastError())));
>>> This is not exactly per message style guidelines. Maybe it's just a
>>> can't-happen case, but if so make it elog not ereport.
>>
>> Check.
>
> Forgot the attachment.
>
> //Magnus
>
>

--------------------------------------------------------------------------------

> *** a/src/backend/utils/adt/pg_locale.c
> --- b/src/backend/utils/adt/pg_locale.c
> ***************
> *** 51,56 ****
> --- 51,57 ----
> #include <time.h>
>
> #include "catalog/pg_control.h"
> + #include "mb/pg_wchar.h"
> #include "utils/memutils.h"
> #include "utils/pg_locale.h"
>
> ***************
> *** 452,457 **** PGLC_localeconv(void)
> --- 453,507 ----
> return &CurrentLocaleConv;
> }
>
> + #ifdef WIN32
> + /*
> + * On win32, strftime() returns the encoding in CP_ACP, which is likely
> + * different from SERVER_ENCODING. This is especially important in Japanese
> + * versions of Windows which will use SJIS encoding, which we don't support
> + * as a server encoding.
> + *
> + * Replace strftime() with a version that gets the string in UTF16 and then
> + * converts it to the appropriate encoding as necessary.
> + */
> + static size_t
> + strftime_win32(char *dst, size_t dstlen, const char *format, const struct tm *tm)
> + {
> + size_t len;
> + wchar_t wbuf[MAX_L10N_DATA];
> + int encoding;
> +
> + encoding = GetDatabaseEncoding();
> + if (encoding == PG_SQL_ASCII)
> + return strftime(dst, dstlen, format, tm);
> +
> + len = wcsftime(wbuf, sizeof(wbuf), format, tm);
> + if (len == 0)
> + /* strftime call failed - return 0 with the contents of dst unspecified */
> + return 0;
> +
> + len = WideCharToMultiByte(CP_UTF8, 0, wbuf, len, dst, dstlen, NULL, NULL);
> + if (len == 0)
> + elog(ERROR,
> + "could not convert string to UTF-8:error %lu", GetLastError());
> +
> + dst[len] = '\0';
> + if (encoding != PG_UTF8)
> + {
> + char *convstr = pg_do_encoding_conversion(dst, len, PG_UTF8, encoding);
> + if (dst != convstr)
> + {
> + StrNCpy(dst, convstr, dstlen);
> + len = strlen(dst);
> + }
> + }
> +
> + return len;
> + }
> +
> + #define strftime(a,b,c,d) strftime_win32(a,b,c,d)
> +
> + #endif /* WIN32 */
> +
>
> /*
> * Update the lc_time localization cache variables if needed.
>

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:

> Please stick to the convention of including include files in
> alphabetical order.

I didn't realize we had such a convention. Is this for all include files or
only within a directory? Are there exceptions where the order matters or have
we managed to make sure it doesn't everywhere?

--
Gregory Stark
EnterpriseDB http://www.enterprisedb.com
Get trained by Bruce Momjian - ask me about EnterpriseDB's PostgreSQL training!

Gregory Stark <stark(at)enterprisedb(dot)com> writes:
> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
>> Please stick to the convention of including include files in
>> alphabetical order.

> I didn't realize we had such a convention. Is this for all include files or
> only within a directory? Are there exceptions where the order matters or have
> we managed to make sure it doesn't everywhere?

If the order matters, it's a bug in the include files. Bruce runs a
script periodically to check that no include file depends on anything
except postgres.h (or postgres_fe.h, as appropriate).

regards, tom lane

"Hiroshi Saito" <z-saito(at)guitar(dot)ocn(dot)ne(dot)jp> wrote:

> Umm, format operand seems to be a wide character sequence.

Here is a patch to work around the wide character format string.
The hack is the following line:
+#define strftime(a,b,c,d) strftime_win32(a,b,L##c,d)

We use only literals in the format strings, the macro adds
wide character prefix (L"...") to them.

Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center

Hi ITAGAKI-san.

Um, It was not supported.
http://winpg.jp/~saito/pg_work/LC_MESSAGE_CHECK/LC_TIME_PATCH/ITAGAKI_PATCH.txt

The test of my various past is reproduced. Then, I proposed the management
regarded as best in them. and, to Magnus-san.
Does the reason for persisting in wsftime exceed a safe reason?

However, I may have missed something...

Regards,
Hiroshi Saito

----- Original Message -----
From: "ITAGAKI Takahiro" <itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp>

>
> "Hiroshi Saito" <z-saito(at)guitar(dot)ocn(dot)ne(dot)jp> wrote:
>
>> Umm, format operand seems to be a wide character sequence.
>
> Here is a patch to work around the wide character format string.
> The hack is the following line:
> +#define strftime(a,b,c,d) strftime_win32(a,b,L##c,d)
>
> We use only literals in the format strings, the macro adds
> wide character prefix (L"...") to them.
>
> Regards,
> ---
> ITAGAKI Takahiro
> NTT Open Source Software Center
>
>

--------------------------------------------------------------------------------

>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

"Hiroshi Saito" <z-saito(at)guitar(dot)ocn(dot)ne(dot)jp> wrote:

> Um, It was not supported.
> http://winpg.jp/~saito/pg_work/LC_MESSAGE_CHECK/LC_TIME_PATCH/ITAGAKI_PATCH.txt

Hmm... the implementation of wcsftime() in msvcrt seems to be
completely broken.

I ran the attached test (localetest.c) and got the following results.
The point is that wcsftime() returns the same character codes as strftime()
i.e, the result is not an unicode string :-(

The bug might be fixed in recently msvcrts in VC2005 or VC2008,
but at least mingw uses the old broken C runtime. We'd better to
use strftime() and multiple conversions to avoid the Microsoft's bug.

----
locale: C
[Wednesday]
C:str = 57 65 64 6e 65 73 64 61 79
C:wcs = 57 65 64 6e 65 73 64 61 79
locale: Japanese_Japan.932
SJIS:str = 90 85 97 6a 93 fa
SJIS:wcs = 90 85 97 6a 93 fa
locale: Japanese_Japan.20932
EUCJP:str = 90 85 97 6a 93 fa
EUCJP:wcs = 90 85 97 6a 93 fa
----

NOTE: There is another problem that specified encoding is ignored
by the functions. The result encoding is always platform default one.

Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center

井上です。

ITAGAKI Takahiro wrote:
> "Hiroshi Saito" <z-saito(at)guitar(dot)ocn(dot)ne(dot)jp> wrote:
>
>> Um, It was not supported.
>> http://winpg.jp/~saito/pg_work/LC_MESSAGE_CHECK/LC_TIME_PATCH/ITAGAKI_PATCH.txt
>
> Hmm... the implementation of wcsftime() in msvcrt seems to be
> completely broken.
>
> I ran the attached test (localetest.c) and got the following results.
> The point is that wcsftime() returns the same character codes as strftime()
> i.e, the result is not an unicode string :-(
>
> The bug might be fixed in recently msvcrts in VC2005 or VC2008,
> but at least mingw uses the old broken C runtime.

私はmingwを使っていないのでよくわからないのですが、ランタイムも一緒に
提供しているのですか？

> We'd better to
> use strftime() and multiple conversions to avoid the Microsoft's bug.

Hi.

I think that MinGW does not have a direct relation.
#define_UNICODE is required for wcsftime.
Probably, ITAGAKI-san has only forgotten it.:-)

P.S)
日本語になっていましたです:-)

Regards,
Hiroshi Saito

----- Original Message -----
From: "Hiroshi Inoue" <inoue(at)tpf(dot)co(dot)jp>

> 井上です。
>
> ITAGAKI Takahiro wrote:
>> "Hiroshi Saito" <z-saito(at)guitar(dot)ocn(dot)ne(dot)jp> wrote:
>>
>>> Um, It was not supported.
>>> http://winpg.jp/~saito/pg_work/LC_MESSAGE_CHECK/LC_TIME_PATCH/ITAGAKI_PATCH.txt
>>
>> Hmm... the implementation of wcsftime() in msvcrt seems to be
>> completely broken.
>>
>> I ran the attached test (localetest.c) and got the following results.
>> The point is that wcsftime() returns the same character codes as strftime()
>> i.e, the result is not an unicode string :-(
>>
>> The bug might be fixed in recently msvcrts in VC2005 or VC2008,
>> but at least mingw uses the old broken C runtime.
>
> 私はmingwを使っていないのでよくわからないのですが、ランタイムも一緒に
> 提供しているのですか？
>
>> We'd better to
>> use strftime() and multiple conversions to avoid the Microsoft's bug.
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers

"Hiroshi Saito" <z-saito(at)guitar(dot)ocn(dot)ne(dot)jp> wrote:

> I think that MinGW does not have a direct relation.
> #define_UNICODE is required for wcsftime.
> Probably, ITAGAKI-san has only forgotten it.:-)

No, definition of _UNICODE is independent from wcsftime (and
other wcs functions). It affects only _tcs functions (_tcsftime).

Wednesday in japanese should be the following sequences in unicode:
wcs = 6c34 66dc 65e5 -- sui yo bi

I rebuild my test on VC++2005 SP1, but it has the same bug.
So, we cannot use wcsftime in Windows unless we build binaries
in VC++2008 at least (and the bug is fixed there).

Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center

ITAGAKI Takahiro wrote:
> "Hiroshi Saito" <z-saito(at)guitar(dot)ocn(dot)ne(dot)jp> wrote:
>
>> I think that MinGW does not have a direct relation.
>> #define_UNICODE is required for wcsftime.
>> Probably, ITAGAKI-san has only forgotten it.:-)
>
> No, definition of _UNICODE is independent from wcsftime (and
> other wcs functions). It affects only _tcs functions (_tcsftime).
>
> Wednesday in japanese should be the following sequences in unicode:
> wcs = 6c34 66dc 65e5 -- sui yo bi
>
> I rebuild my test on VC++2005 SP1, but it has the same bug.
> So, we cannot use wcsftime in Windows unless we build binaries
> in VC++2008 at least (and the bug is fixed there).

Please call setlocale(LC_CTYPE/LC_ALL, "") first.

regards,
Hiroshi Inoue

Hiroshi Inoue <inoue(at)tpf(dot)co(dot)jp> wrote:

> Please call setlocale(LC_CTYPE/LC_ALL, "") first.

Ah, it works! But setlocale(*, "") means that we always use platform
locale (Japanese and SJIS in Japan). It could be different from server
encoding and locale in postgres. Is it acceptable? I think we need to
set LC_CTYPE and other LC_* independently...

Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center

ITAGAKI Takahiro wrote:
> Hiroshi Inoue <inoue(at)tpf(dot)co(dot)jp> wrote:
>
>> Please call setlocale(LC_CTYPE/LC_ALL, "") first.
>
> Ah, it works! But setlocale(*, "") means that we always use platform
> locale (Japanese and SJIS in Japan).

Maybe you can call setlocale(LC_CTYPE, ".20932") instead and you
would get CP20932 encoding. The encoding of LC_TIME or LC_MESSAGES
has little meaning.

> It could be different from server
> encoding and locale in postgres. Is it acceptable? I think we need to
> set LC_CTYPE and other LC_* independently...

Seems LC_CTYPE and LC_TIME should be convertible even though we use
wcsftime (which internally calls strftime?).
As for gettext(LC_MESSAGES) on Windows we can set LC_CTYPE independently
because it is unrelated to the output encoding. In addition we can call
bind_textdomain_codeset to change the output encoding.
I'm providing a patch to adjust the output encoding of Windows
gettext.

regards,
Hiroshi Inoue

Hiroshi Inoue <inoue(at)tpf(dot)co(dot)jp> wrote:

> Seems LC_CTYPE and LC_TIME should be convertible even though we use
> wcsftime (which internally calls strftime?).

Ok, wcsftime() requries both LC_TIME and LC_CTYPE are the same setting
(at least encoding) on Windows.

The attached patch is an updated version to fix cache_locale_time().
Now it sets LC_TIME and LC_CTYPE to the specified locale and restore
them at end of the function. I tested the patch on Windows XP Japanese
Edition (SJIS) with UTF-8 and EUCJP databases, and worked expectedly.

"#ifdef WIN32" codes seems to be ugly in the patch,
but I have no other idea...

Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center

ITAGAKI Takahiro wrote:
>
> Hiroshi Inoue <inoue(at)tpf(dot)co(dot)jp> wrote:
>
> > Seems LC_CTYPE and LC_TIME should be convertible even though we use
> > wcsftime (which internally calls strftime?).
>
> Ok, wcsftime() requries both LC_TIME and LC_CTYPE are the same setting
> (at least encoding) on Windows.

shouldn't this use LC_TIME?

+ setlocale(LC_CTYPE, locale_time);

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

ITAGAKI Takahiro wrote:
> Hiroshi Inoue <inoue(at)tpf(dot)co(dot)jp> wrote:
>
>> Seems LC_CTYPE and LC_TIME should be convertible even though we use
>> wcsftime (which internally calls strftime?).
>
> Ok, wcsftime() requries both LC_TIME and LC_CTYPE are the same setting
> (at least encoding) on Windows.
>
> The attached patch is an updated version to fix cache_locale_time().
> Now it sets LC_TIME and LC_CTYPE to the specified locale and restore
> them at end of the function. I tested the patch on Windows XP Japanese
> Edition (SJIS) with UTF-8 and EUCJP databases, and worked expectedly.
>
> "#ifdef WIN32" codes seems to be ugly in the patch,
> but I have no other idea...

Hmm. Is this actually cleaner than using the original method as
suggested? Because if I understand things right, that version did *not*
require the setting of LC_CTYPE? (this is the version that uses strftime
and does two conversions)

//Magnus

ITAGAKI Takahiro wrote:
> Hiroshi Inoue <inoue(at)tpf(dot)co(dot)jp> wrote:
>
>> Seems LC_CTYPE and LC_TIME should be convertible even though we use
>> wcsftime (which internally calls strftime?).
>
> Ok, wcsftime() requries both LC_TIME and LC_CTYPE are the same setting
> (at least encoding) on Windows.
>
> The attached patch is an updated version to fix cache_locale_time().
> Now it sets LC_TIME and LC_CTYPE to the specified locale and restore
> them at end of the function. I tested the patch on Windows XP Japanese
> Edition (SJIS) with UTF-8 and EUCJP databases, and worked expectedly.

I've also thought a similar implementation but there seems
a problem of efficiency.
As far as I see wcsftime() is almost = strftime() + mbstowcs()
and so using strftime() is effective at least for the following
cases.

1) LC_CTIME is "C".
2) LC_CTYPE != C and the database encoding != UTF-8. In this
case the current restriction of PostgreSQL requires that
the database encoding matches the encoding of the LC_CTYPE.

We seem to be able to call strftime() directly in above cases.

Comments ?

regards,
Hiroshi Inoue

> I've also thought a similar implementation but there seems
> a problem of efficiency.
> As far as I see wcsftime() is almost = strftime() + mbstowcs()
> and so using strftime() is effective at least for the following
> cases.
>
> 1) LC_CTIME is "C".
> 2) LC_CTYPE != C and the database encoding != UTF-8. In this
> case the current restriction of PostgreSQL requires that
> the database encoding matches the encoding of the LC_CTYPE.
>
> We seem to be able to call strftime() directly in above cases.
>
> Comments ?

I quite agree on that point.

==
遅くまですみません、、もう寝ましょう:-)

Regards,
Hiroshi Saito

Alvaro Herrera wrote:
> ITAGAKI Takahiro wrote:
>> Hiroshi Inoue <inoue(at)tpf(dot)co(dot)jp> wrote:
>>
>>> Seems LC_CTYPE and LC_TIME should be convertible even though we use
>>> wcsftime (which internally calls strftime?).
>> Ok, wcsftime() requries both LC_TIME and LC_CTYPE are the same setting
>> (at least encoding) on Windows.
>
> shouldn't this use LC_TIME?
>
> + setlocale(LC_CTYPE, locale_time);

AFAIK the results of strftime() is determined by LC_TIME and
LC_CTYPE. Date/time representations by LC_TIME language are
converted to LC_CTYPE codeset. In fact I can see the Chinese
strftome() representaions by Japanese codeset fortunately.

Our goal is to convert LC_CTIME representaions to the database
encoding. Though we expected that wcsftime() doesn't rely on
LC_CTYPE, wcsftime() relies on LC_CTYPE because it is almost =
strftime() + mbstowcs() and a problem occured e.g. when the
LC_CTYPE is set to "C".

regards,
Hiroshi Inoue

Magnus Hagander <magnus(at)hagander(dot)net> wrote:

> ITAGAKI Takahiro wrote:
> > Ok, wcsftime() requries both LC_TIME and LC_CTYPE are the same setting
> > (at least encoding) on Windows.
> >
> Hmm. Is this actually cleaner than using the original method as
> suggested? Because if I understand things right, that version did *not*
> require the setting of LC_CTYPE? (this is the version that uses strftime
> and does two conversions)

No, we can't. I found strftime doesn't work if the database is
initialized with non-C locale. It works only when lc_ctype = C and
the encoding of lc_time is matched with Windows' one.
On the other hand, wcsftime seems to be more robust.
(The reason might come from mysterious msvcrt implementation.)

[For reviewers and testers]
For the above reason, please test the patch with some combinations
of locales and encodings, some of that might be different from
Windows' native ones.

[error result of strftime version]
$ initdb --locale=Japanese_Japan.20932 --encoding=eucjp

postgres=# SELECT name, setting FROM pg_settings WHERE name LIKE 'lc%';
name | setting
-------------+----------------------
lc_collate | Japanese_Japan.20932
lc_ctype | Japanese_Japan.20932
lc_messages | Japanese_Japan.20932
lc_monetary | Japanese_Japan.20932
lc_numeric | Japanese_Japan.20932
lc_time | Japanese_Japan.20932
(6 rows)

postgres=# SELECT to_char(now(), 'TMday');
ERROR: invalid byte sequence for encoding "UTF8": 0x00
HINT: This error can also happen if the byte sequence does not match the encoding expected by the server, which is controlled by "client_encoding".

Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center

ITAGAKI Takahiro wrote:
> Magnus Hagander <magnus(at)hagander(dot)net> wrote:
>
>> ITAGAKI Takahiro wrote:
>>> Ok, wcsftime() requries both LC_TIME and LC_CTYPE are the same setting
>>> (at least encoding) on Windows.
>>>
>> Hmm. Is this actually cleaner than using the original method as
>> suggested? Because if I understand things right, that version did *not*
>> require the setting of LC_CTYPE? (this is the version that uses strftime
>> and does two conversions)
>
> No, we can't. I found strftime doesn't work if the database is
> initialized with non-C locale. It works only when lc_ctype = C and
> the encoding of lc_time is matched with Windows' one.
> On the other hand, wcsftime seems to be more robust.
> (The reason might come from mysterious msvcrt implementation.)

The setting of LC_CTYPE acts like a catalyst for wcsftime() to
work safely.

I checked the code around the patch and found the central function
cache_locale_time() is rarely called (at most once per LC_TIME change).
The function cache_locale_time() calls strftime() only 38(=7*2+12*2)
times. Now I fundamentally agree with Itagaki-san's patch. Though
it may not be the most effective code, it looks like the clearest one.

regards,
Hiroshi Inoue

ITAGAKI Takahiro wrote:
> Hiroshi Inoue <inoue(at)tpf(dot)co(dot)jp> wrote:
>
>> Seems LC_CTYPE and LC_TIME should be convertible even though we use
>> wcsftime (which internally calls strftime?).
>
> Ok, wcsftime() requries both LC_TIME and LC_CTYPE are the same setting
> (at least encoding) on Windows.
>
> The attached patch is an updated version to fix cache_locale_time().
> Now it sets LC_TIME and LC_CTYPE to the specified locale and restore
> them at end of the function. I tested the patch on Windows XP Japanese
> Edition (SJIS) with UTF-8 and EUCJP databases, and worked expectedly.
>
> "#ifdef WIN32" codes seems to be ugly in the patch,
> but I have no other idea...

I have applied this version of the patch (with only a minor further
addition to the comment).

Thank you all for your work and patience in getting this fixed! Let's
hope it stays fixed :-)

//Magnus

Hi.

Thanks all.!!!!

I tried CVS-HEAD now.

================================================
HIROSHI=# select to_char(now(),'TMDay');
to_char
----------
Saturday
(1 行)

HIROSHI=# set LC_MESSAGES=Ja;
SET
HIROSHI=# select to_char(now(),'TMDay');
to_char
----------
Saturday
(1 行)
================================================

Umm, It does not look at a comfortable result.:-(
I will check it on tomorrow night. sorry busy now..

Regards,
Hiroshi Saito

----- Original Message -----
From: "Magnus Hagander" <magnus(at)hagander(dot)net>

> ITAGAKI Takahiro wrote:
>> Hiroshi Inoue <inoue(at)tpf(dot)co(dot)jp> wrote:
>>
>>> Seems LC_CTYPE and LC_TIME should be convertible even though we use
>>> wcsftime (which internally calls strftime?).
>>
>> Ok, wcsftime() requries both LC_TIME and LC_CTYPE are the same setting
>> (at least encoding) on Windows.
>>
>> The attached patch is an updated version to fix cache_locale_time().
>> Now it sets LC_TIME and LC_CTYPE to the specified locale and restore
>> them at end of the function. I tested the patch on Windows XP Japanese
>> Edition (SJIS) with UTF-8 and EUCJP databases, and worked expectedly.
>>
>> "#ifdef WIN32" codes seems to be ugly in the patch,
>> but I have no other idea...
>
> I have applied this version of the patch (with only a minor further
> addition to the comment).
>
> Thank you all for your work and patience in getting this fixed! Let's
> hope it stays fixed :-)
>
> //Magnus
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers

"Hiroshi Saito" <z-saito(at)guitar(dot)ocn(dot)ne(dot)jp> writes:
> HIROSHI=# set LC_MESSAGES=Ja;
> SET
> HIROSHI=# select to_char(now(),'TMDay');
> to_char
> ----------
> Saturday
> (1 )

I thought this was supposed to be driven by LC_TIME now, not
LC_MESSAGES.

regards, tom lane

> I thought this was supposed to be driven by LC_TIME now, not
> LC_MESSAGES.

Uga, yes yes!

HIROSHI=# set LC_TIME=Ja;
SET
HIROSHI=# select to_char(now(),'TMDay');
to_char
---------
土曜日
(1 行)

Thanks:-)

Hiroshi Saito wrote:
>> I thought this was supposed to be driven by LC_TIME now, not
>> LC_MESSAGES.
>
> Uga, yes yes!
>
> HIROSHI=# set LC_TIME=Ja;
> SET
> HIROSHI=# select to_char(now(),'TMDay');
> to_char
> ---------
> 土曜日
> (1 行)
>
> Thanks:-)

Great! Thanks for testing!

//Magnus

Ooooops,my mistake in the mail thread.:-(
sorry.

=== send the mistake other thread ==
Hi.

My swift attack test was the MinGW environment.
But, Inoue-san suggestion.

1. MinGW+gcc build
HIROSHI=# set LC_TIME=Ja;
SET
HIROSHI=# select to_char(now(),'TMDay');
to_char
---------
日曜日
(1 行)
HIROSHI=# set LC_TIME='Japan';
SET
HIROSHI=# select to_char(Now(),'TMDay');
to_char
---------
日曜日
(1 行)
HIROSHI=# set LC_TIME='Japanese';
SET
HIROSHI=# select to_char(Now(),'TMDay');
to_char
---------
日曜日
(1 行)

However, A setup of 'Ja' was strange.?_?
http://msdn.microsoft.com/en-us/library/aa246450(VS.60).aspx

2. MSVC build
HIROSHI=# set LC_TIME='Ja';
ERROR: invalid value for parameter "lc_time": "Ja"
STATEMENT: set LC_TIME='Ja';
ERROR: invalid value for parameter "lc_time": "Ja"
HIROSHI=# set LC_TIME='Japan';
ERROR: invalid value for parameter "lc_time": "Japan"
STATEMENT: set LC_TIME='Japan';
ERROR: invalid value for parameter "lc_time": "Japan"
HIROSHI=# set LC_TIME=Japanese;
SET
HIROSHI=# select to_char(Now(),'TMDay');
to_char
---------
日曜日
(1 行)

Umm, Re-investigation is required for this. :-(
However, If reasonable clear, it will be good for a document at suggestion.

Regards,
Hiroshi Saito