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

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)