Re: [PATCHES] snprintf() argument reordering not working under

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(at)candle(dot)pha(dot)pa(dot)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(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073