Re: [PATCHES] [HACKERS] snprintf() argument reordering not working under Windows
Nicolai Tufar wrote: Greetings, Last April we have made some changes to src/ports/snprintf.c so that it would support argument reordering like %2$s, %1$d and such on platforms where original snprintf() does not support it, like Windows, HP-UX or NetBSD. Sure, I remember. So glad you returned at this time. I found a design limitation in that file yesterday. It can not output more then 4096 characters, and there are some cases with NUMERIC that try to output more than that. For example: SELECT pow(10::numeric, 1) + 1; should show a '1' at the end of the number, but with the existing code you will just see 4095 0's and no more. I am attaching the new snprintf.c and the patch itself for your review. The change is to pass 'stream' down into the routines and output to the FILE* right from inside the routine, rather than using a string. This fixes the problem. I am also thinking of modifying the code so if we are using snprintf.c only because we need positional parameter control, we check for '$' in the string and only use snprintf.c in those cases. NLS messages of some languages, like Turkish, rely heavily on argument reordering because of the language structure. In 8.1 Turkish messages in Windows version are all broken because argument reordering is not there. Really? I have not heard any report of that but this is new code in 8.1. I examined commit logs and came to conclusion that src/port/snprintf.c is not included in server when compiling under Windows because of change to src/port/Makefile made in revision 1.28 by Bruce Momjian. See here: http://developer.postgresql.org/cvsweb.cgi/pgsql/src/port/Makefile Comment to the commit says: `No server version of snprintf needed, so remove Makefile rule.' In fact I think we need snprintf in server because NLS messages are printed by the server. Actually, that changes means that there was nothing backend-specific in snprintf.c so we don't need a _special_ version for the backend. The real change not to use snprintf.c on Win32 is in configure.in with this comment: # Force use of our snprintf if system's doesn't do arg control # This feature is used by NLS if test $enable_nls = yes test $pgac_need_repl_snprintf = no # On Win32, libintl replaces snprintf() with its own version that # understands arg control, so we don't need our own. In fact, it # also uses macros that conflict with ours, so we _can't_ use # our own. test $PORTNAME != win32; then PGAC_FUNC_PRINTF_ARG_CONTROL if test $pgac_cv_printf_arg_control != yes ; then pgac_need_repl_snprintf=yes fi fi Here is the commit: revision 1.409 date: 2005/05/05 19:15:54; author: momjian; state: Exp; lines: +8 -2 On Win32, libintl replaces snprintf() with its own version that understands arg control, so we don't need our own. In fact, it also uses macros that conflict with ours, so we _can't_ use our own. So, I think it was Magnus who said that Win32 didn' need and couldn't use our snprintf. Magnus, any ideas? -- Bruce Momjian| http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 /* * Copyright (c) 1983, 1995, 1996 Eric P. Allman * Copyright (c) 1988, 1993 * The Regents of the University of California. All rights reserved. * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions * are met: * 1. Redistributions of source code must retain the above copyright *notice, this list of conditions and the following disclaimer. * 2. Redistributions in binary form must reproduce the above copyright *notice, this list of conditions and the following disclaimer in the *documentation and/or other materials provided with the distribution. * 3. All advertising materials mentioning features or use of this software *must display the following acknowledgement: * This product includes software developed by the University of * California, Berkeley and its contributors. * 4. Neither the name of the University nor the names of its contributors *may be used to endorse or promote products derived from this software *without specific prior written permission. * * THIS SOFTWARE IS PROVIDED BY THE REGENTS AND CONTRIBUTORS ``AS IS'' AND * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE * ARE DISCLAIMED. IN NO EVENT SHALL THE REGENTS OR CONTRIBUTORS BE LIABLE * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL *
Re: [PATCHES] [HACKERS] snprintf() argument reordering not working under Windows
Bruce Momjian pgman@candle.pha.pa.us writes: I am also thinking of modifying the code so if we are using snprintf.c only because we need positional parameter control, we check for '$' in the string and only use snprintf.c in those cases. What's the point? If the code is in there we may as well use it. regards, tom lane ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] [HACKERS] snprintf() argument reordering not working under Windows in 8.1
Here is the commit: revision 1.409 date: 2005/05/05 19:15:54; author: momjian; state: Exp; lines: +8 -2 On Win32, libintl replaces snprintf() with its own version that understands arg control, so we don't need our own. In fact, it also uses macros that conflict with ours, so we _can't_ use our own. So, I think it was Magnus who said that Win32 didn' need and couldn't use our snprintf. Magnus, any ideas? I don't recall having said that. I can't promise I didn't, but if so I don't remember why. (And I do remember Nicolais fixes, so I think I should've thought of that before saying something like that) //Magnus ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [PATCHES] [HACKERS] snprintf() argument reordering not working under Windows in 8.1
Greetings, I thought it will be as simple as changing Makefile, the issue seem to be much more complicated. Unfortunately I have no PostgreSQL building environment handy and will not be able to look at it until the end of next week because I am moving my house :( But since this issue waited for so long it may wait a week more. 2005/12/3, Bruce Momjian pgman@candle.pha.pa.us: Sure, I remember. So glad you returned at this time. I found a design limitation in that file yesterday. It can not output more then 4096 characters, and there are some cases with NUMERIC that try to output more than that. For example: SELECT pow(10::numeric, 1) + 1; should show a '1' at the end of the number, but with the existing code you will just see 4095 0's and no more. I am attaching the new snprintf.c and the patch itself for your review. The change is to pass 'stream' down into the routines and output to the FILE* right from inside the routine, rather than using a string. This fixes the problem. Your patch increase buffers from 4095 to 8192. Sounds good to me. I am also thinking of modifying the code so if we are using snprintf.c only because we need positional parameter control, we check for '$' in the string and only use snprintf.c in those cases. I too, was thinking of it at the beginning but decided that the code would get even more complicated than it was at the moment and was afraid that core team would not accept my patch. :) NLS messages of some languages, like Turkish, rely heavily on argument reordering because of the language structure. In 8.1 Turkish messages in Windows version are all broken because argument reordering is not there. Really? I have not heard any report of that but this is new code in 8.1. Windows installer does not set lc_messages configuration variable correctly in postgresql.conf file. It is a known issue and will be solved in next version. Meanwhile user needs to open postgresql.conf file and change lc_messages = 'Turkish_Turkey.28599' to lc_messages = 'tr_TR.UTF-8' manually. Apparently nobody cared to do it and Devrim never ever touches Windows. The problem is there, I am definitely positive about it, here are two lines from log file: 2005-11-20 12:36:37 HATA: $s yerinde $s 1 karakterinde 2005-12-03 19:14:27 LOG: $s veritabanın transaction ID warp limiti $u Actually, that changes means that there was nothing backend-specific in snprintf.c so we don't need a _special_ version for the backend. The real change not to use snprintf.c on Win32 is in configure.in with this comment: # Force use of our snprintf if system's doesn't do arg control # This feature is used by NLS if test $enable_nls = yes test $pgac_need_repl_snprintf = no # On Win32, libintl replaces snprintf() with its own version that # understands arg control, so we don't need our own. In fact, it # also uses macros that conflict with ours, so we _can't_ use # our own. test $PORTNAME != win32; then PGAC_FUNC_PRINTF_ARG_CONTROL if test $pgac_cv_printf_arg_control != yes ; then pgac_need_repl_snprintf=yes fi fi Here is the commit: revision 1.409 date: 2005/05/05 19:15:54; author: momjian; state: Exp; lines: +8 -2 On Win32, libintl replaces snprintf() with its own version that understands arg control, so we don't need our own. In fact, it also uses macros that conflict with ours, so we _can't_ use our own. I don't have MinGW build environment on my computer at the moment and will not be able to install it until the end of next week but log messages above were produced by PostgreSQL installed with 8.1.0-2 Windows installer downloaded from postgresql.org. Since Turkish messages are printed I suppose it was compiled with $enable_nls = yes Thank you for your attention, Regards, Nicolai Tufar ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
