Solve a problem of LC_TIME of windows.

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@hagander.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@commandprompt.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@guitar.ocn.ne.jp> writes:

From: "Magnus Hagander" <magnus@hagander.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@guitar.ocn.ne.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.takahiro@oss.ntt.co.jp> wrote:

Hello, Saito-san:

"Hiroshi Saito" <z-saito@guitar.ocn.ne.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@systemguards.com.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.takahiro@oss.ntt.co.jp>

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@guitar.ocn.ne.jp>

On Sun, Nov 16, 2008 at 8:36 AM, Hiroshi Saito <z-saito@guitar.ocn.ne.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@systemguards.com.ec>

On Sun, Nov 16, 2008 at 8:36 AM, Hiroshi Saito <z-saito@guitar.ocn.ne.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

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

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 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@hagander.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@guitar.ocn.ne.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.takahiro@oss.ntt.co.jp>

"Jaime Casanova" <jcasanov@systemguards.com.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@postgresql.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)

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

Magnus Hagander <magnus@hagander.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@hagander.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:

Magnus Hagander wrote:

Tom Lane wrote:

Magnus Hagander <magnus@hagander.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@hagander.net>

Magnus Hagander wrote:

Tom Lane wrote:

Magnus Hagander <magnus@hagander.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

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