Thread: Re: [PATCHES] Solve a problem of LC_TIME of windows.
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
Attachment
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
Attachment
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
Attachment
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 > >
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
Attachment
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. > > 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)
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) > -------------------------------------------------------------------------------- > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers >
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: > 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) >> > > > -------------------------------------------------------------------------------- > > > >> >> -- >> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) >> To make changes to your subscription: >> http://www.postgresql.org/mailpref/pgsql-hackers >>
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
*** 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.
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
>
>
--------------------------------------------------------------------------------
> *** 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@sss.pgh.pa.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'sPostgreSQL training!
Gregory Stark <stark@enterprisedb.com> writes:
> Tom Lane <tgl@sss.pgh.pa.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@guitar.ocn.ne.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
Attachment
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.takahiro@oss.ntt.co.jp> > > "Hiroshi Saito" <z-saito@guitar.ocn.ne.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@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers >
"Hiroshi Saito" <z-saito@guitar.ocn.ne.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
Attachment
井上です。 ITAGAKI Takahiro wrote: > "Hiroshi Saito" <z-saito@guitar.ocn.ne.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@tpf.co.jp> > 井上です。 > > ITAGAKI Takahiro wrote: >> "Hiroshi Saito" <z-saito@guitar.ocn.ne.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@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers
"Hiroshi Saito" <z-saito@guitar.ocn.ne.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@guitar.ocn.ne.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@tpf.co.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@tpf.co.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@tpf.co.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
Attachment
ITAGAKI Takahiro wrote: > > Hiroshi Inoue <inoue@tpf.co.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@tpf.co.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@tpf.co.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@tpf.co.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@hagander.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.20932lc_ctype | Japanese_Japan.20932lc_messages | Japanese_Japan.20932lc_monetary| Japanese_Japan.20932lc_numeric | Japanese_Japan.20932lc_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 controlledby "client_encoding". Regards, --- ITAGAKI Takahiro NTT Open Source Software Center
ITAGAKI Takahiro wrote: > Magnus Hagander <magnus@hagander.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'spatch. Though it may not be the most effective code, it looks like the clearest one. regards, Hiroshi Inoue
ITAGAKI Takahiro wrote: > Hiroshi Inoue <inoue@tpf.co.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@hagander.net> > ITAGAKI Takahiro wrote: >> Hiroshi Inoue <inoue@tpf.co.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@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers
"Hiroshi Saito" <z-saito@guitar.ocn.ne.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