Re: [PATCHES] snprintf() argument reordering not working under - Mailing list pgsql-hackers

Nicolai Tufar wrote:
> 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.

Agreed.  The problem is actually worse than I described --- see below.

> 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, 10000) + 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.

Well, that fixed-size buffer is now used only for sprintf().  The others
use the specified length (for snprintf()) or output directly to the
FILE* stream.

> > 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.   :)

Seems Tom didn't like that and no one else has commented.

> > > 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

OK.

> > 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

OK, here is what happened.  In March 2005, we got reports of compile
problems on Win32 using NLS:
http://archives.postgresql.org/pgsql-hackers/2005-03/msg00710.php

(See the quoted text under the posted text as well.)  Basically,
libintl.h on Win32 replaces *printf calls with its own versions, and
does it using macros, _just_ like we do.  This of course causes
conflicts and the system fails to compile.  The _fix_ was to disable
port/*printf on Win32 when using NLS because NLS wants to use its own
*printf.  I _assumed_ that libintl.h did this so it could use its own
routines that understood %$, but never verified that.  It didn't seem we
had any choice to fix this, and got no problem reports about %$ not
working until yours.

After over a month with no solution I added the code as you see it now:
http://archives.postgresql.org/pgsql-hackers-win32/2005-05/msg00011.php

Oh, and it was Andrew Dunstan who worked on this, not Magnus.  (Sorry
Magnus, Hello Andrew.)

Anyway, I think the big question is, was the pginstaller built with a
libintl that replaces *printf, and is it an *printf that doesn't
understand positional parameters, and so, how can we fix it.

--  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,
Pennsylvania19073