snprintf causes regression tests to fail

Linux and Solaris 10 x86 pass regression tests fine when I force the use of new
snprintf(). The problem should be win32 - specific. I will
investigate it throughly
tonight. Can someone experienced in win32 what can possibly be the problem?

Nick

Nicolai Tufar wrote:

Linux and Solaris 10 x86 pass regression tests fine when I force the use of new
snprintf(). The problem should be win32 - specific. I will
investigate it throughly
tonight. Can someone experienced in win32 what can possibly be the problem?

Yea, I am confused too because my BSD uses the new snprintf.c code was
well. Magnus, what failures are you seeing on Win32?

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

Linux and Solaris 10 x86 pass regression tests fine when I force the use
of new
snprintf(). The problem should be win32 - specific. I will
investigate it throughly
tonight. Can someone experienced in win32 what can possibly be the
problem?

Do we have any idea about what format string causes the regression failure?
It may be that certain integer types are not promoted uniformly when
pushed on the stack.

After some extensive debugging with Magnus's
help we finally managed to a kind of isolate the
problem. We placed snprintf.c in a separate
file, added necessary #includes and wrote
a simple main() function:

main()
{
unsigned long long ull=4567890123456789ULL;
static char buf[1024];
mysnprintf(buf,1024,"%lld\n",ull);
printf(buf);
}

When compiled with -D HAVE_LONG_LONG_INT_64=1
which declares long_long and ulong_long like:

typedef long long long_long;
typedef unsigned long long ulong_long;

It compiles fine and produces desired result. If not,
it produces "-869367531" as in regression tests.

Amazingly enough HAVE_LONG_LONG_INT_64 is
defined when compilation comes to src/port/snprintf.c
but the result is still wrong. I looked into configure.in
but the check for HAVE_LONG_LONG_INT_64 is too
complicated for me to understand. Bruce, could you
take a look at this? I am 90% sure it is an issue with
some configure definitions.

Best regards,
Nicolai

pgsql@mohawksoft.com writes:

Do we have any idea about what format string causes the regression failure?

I'll bet the problem is that configure.in is doing things in the wrong
order: it computes INT64_FORMAT against the system printf before
deciding we should use our own printf.

regards, tom lane

Tom Lane wrote:

pgsql@mohawksoft.com writes:

Do we have any idea about what format string causes the regression failure?

I'll bet the problem is that configure.in is doing things in the wrong
order: it computes INT64_FORMAT against the system printf before
deciding we should use our own printf.

Ah, the problem was introduced here:

revision 1.401
date: 2005/02/24 02:12:15; author: tgl; state: Exp; lines: +13 -12
We aren't supposed to try to run test programs until after we've
verified that AC_TRY_RUN works.

The problem is that the PGAC_FUNC_PRINTF_ARG_CONTROL call was moved
below the printf 64-bit tests. This commited patch moves
PGAC_FUNC_PRINTF_ARG_CONTROL which is after we know AC_TRY_RUN works and
just before printf 64-bit args are tested.

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

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Ah, the problem was introduced here:

Right, it was my fault.

The problem is that the PGAC_FUNC_PRINTF_ARG_CONTROL call was moved
below the printf 64-bit tests. This commited patch moves
PGAC_FUNC_PRINTF_ARG_CONTROL which is after we know AC_TRY_RUN works and
just before printf 64-bit args are tested.

This patch breaks things in a different way: you should not have moved
the AC_LIBOBJ(snprintf) step. Also you randomly placed the arg-control
test between a chunk of code and the comment describing same.

Corrected version committed.

regards, tom lane

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Ah, the problem was introduced here:

Right, it was my fault.

The problem is that the PGAC_FUNC_PRINTF_ARG_CONTROL call was moved
below the printf 64-bit tests. This commited patch moves
PGAC_FUNC_PRINTF_ARG_CONTROL which is after we know AC_TRY_RUN works and
just before printf 64-bit args are tested.

This patch breaks things in a different way: you should not have moved
the AC_LIBOBJ(snprintf) step. Also you randomly placed the arg-control
test between a chunk of code and the comment describing same.

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

Neither Bruce's nor subsequent Tom's patch did not fix
the issue. The command used is:

make maintainer-clean && ./configure && make && make install && make check

It should have be fine to recompile the source code
completely. I attach the resulting config.log. May be it
will give a clue. Regression test failure are in the
same places as previous ones.

Best regards,
Nicolai

On Mon, 28 Feb 2005 16:29:57 -0500 (EST), Bruce Momjian
<pgman@candle.pha.pa.us> wrote:

Nicolai Tufar wrote:

Neither Bruce's nor subsequent Tom's patch did not fix
the issue. The command used is:

make maintainer-clean && ./configure && make && make install && make check

It should have be fine to recompile the source code
completely. I attach the resulting config.log. May be it
will give a clue. Regression test failure are in the
same places as previous ones.

No log attached. Please email the log to me privately.

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

And while we are on it, I would like to submit minor
changes to make snprintf() vsnprintf() and printf()
functions in src/port/snprintf.c thread-safe.

Best regards,
Nicolai Tufar

Patch applied. Thanks.

---------------------------------------------------------------------------

Nicolai Tufar wrote:

And while we are on it, I would like to submit minor
changes to make snprintf() vsnprintf() and printf()
functions in src/port/snprintf.c thread-safe.

Best regards,
Nicolai Tufar

[ Attachment, skipping... ]

---------------------------(end of broadcast)---------------------------
TIP 6: Have you searched our list archives?

http://archives.postgresql.org

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

Nicolai Tufar wrote:

Regression test diff is attached.
It fails on the following tests:
int8
subselect
union
sequence

It fails to display correctly number "4567890123456789".
In output is shows "-869367531". Apparent overflow or
interpreting int8 as int4.

while rewriting snprintf() I did not touch the actual functions
that convert number to ASCII digit string. In truth, if you
force PostgreSQL to use snprintf.c without my patch applied
it produces the same errors.

What can be wrong? GCC bug? The one I use is:
gcc.exe (GCC) 3.3.1 (mingw special 20030804-1)

I can confirm your failure in current sources on Win32:

template1=# create table test(x int8);
CREATE TABLE
template1=# insert into test values ('4567890123456789');
INSERT 17235 1
template1=# select * from test;
x
------------
-869367531
(1 row)

and it knows it is a large number:

template1=# select * from test where x > 1000::int8;
x
------------
-869367531
(1 row)
template1=# select * from test where x < 1000::int8;
x
---
(0 rows)

I am going to add some debugs to see what is being passed to *printf().

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

Bruce Momjian wrote:

I can confirm your failure in current sources on Win32:

template1=# create table test(x int8);
CREATE TABLE
template1=# insert into test values ('4567890123456789');
INSERT 17235 1
template1=# select * from test;
x
------------
-869367531
(1 row)

and it knows it is a large number:

template1=# select * from test where x > 1000::int8;
x
------------
-869367531
(1 row)
template1=# select * from test where x < 1000::int8;
x
---
(0 rows)

I am going to add some debugs to see what is being passed to *printf().

I have not found the solution yet but am heading to bed. My next guess
is that Win32 isn't handling va_arg(..., long long int) properly.

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

On Tue, 1 Mar 2005 00:55:20 -0500 (EST), Bruce Momjian

My next guess
is that Win32 isn't handling va_arg(..., long long int) properly.

I am trying various combination of number and types
of parameters in my test program and everything prints fine.
When it comes to pg, it fails :(

template1=# select * from test where x > 1000::int8;
x
------------
-869367531
(1 row)

I am not too fluent in source code, could someone
point me to there actual call to snprintf() is being done
when a query like this is executed. I could not find it myslef
:(

Regards,
Nick

Nicolai Tufar wrote:

On Tue, 1 Mar 2005 00:55:20 -0500 (EST), Bruce Momjian

My next guess
is that Win32 isn't handling va_arg(..., long long int) properly.

I am trying various combination of number and types
of parameters in my test program and everything prints fine.
When it comes to pg, it fails :(

template1=# select * from test where x > 1000::int8;
x
------------
-869367531
(1 row)

I am not too fluent in source code, could someone
point me to there actual call to snprintf() is being done
when a query like this is executed. I could not find it myslef

Sure, in src/backend/utils/adt/int8.c, there is a call in int8out():

if ((len = snprintf(buf, MAXINT8LEN, INT64_FORMAT, val)) < 0)

and that calls port/snprintf.c.

I have added a puts() in snprintf.c to make sure it is getting the
long/long specifier.

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

I spent all day debugging it. Still have absolutely
no idea what could possibly go wrong. Does
anyone have a slightest clue what can it be and
why it manifests itself only on win32?

On Tue, 1 Mar 2005 09:29:07 -0500 (EST), Bruce Momjian
<pgman@candle.pha.pa.us> wrote:

Nicolai Tufar wrote:

On Tue, 1 Mar 2005 00:55:20 -0500 (EST), Bruce Momjian

My next guess
is that Win32 isn't handling va_arg(..., long long int) properly.

I am trying various combination of number and types
of parameters in my test program and everything prints fine.
When it comes to pg, it fails :(

template1=# select * from test where x > 1000::int8;
x
------------
-869367531
(1 row)

I am not too fluent in source code, could someone
point me to there actual call to snprintf() is being done
when a query like this is executed. I could not find it myslef

Sure, in src/backend/utils/adt/int8.c, there is a call in int8out():

if ((len = snprintf(buf, MAXINT8LEN, INT64_FORMAT, val)) < 0)

and that calls port/snprintf.c.

I have added a puts() in snprintf.c to make sure it is getting the
long/long specifier.

Just a question, and don't mind me if I am being rude, isn't this the
WRONG PLACE for a "printf" function? Wouldn't an "itoa" function be more
efficient and be less problematic?

pgsql@mohawksoft.com writes:

Just a question, and don't mind me if I am being rude, isn't this the
WRONG PLACE for a "printf" function? Wouldn't an "itoa" function be more
efficient and be less problematic?

This particular call could be so replaced, but it wouldn't solve the
general problem. snprintf has to work.

regards, tom lane

I spent all day debugging it. Still have absolutely
no idea what could possibly go wrong. Does
anyone have a slightest clue what can it be and
why it manifests itself only on win32?

It may be that the CLIB has badly broken support for 64bit integers on 32
bit platforms. Does anyone know of any Cygwin/Ming issues?

Is this only with the new snprintf code in Win32?

Is this a problem with snprintf as implemented in src/port?

Is there a reason why we don't use the snprintf that comes with the
various C compilers?

I spent all day debugging it. Still have absolutely
no idea what could possibly go wrong. Does
anyone have a slightest clue what can it be and
why it manifests itself only on win32?

It may be that the CLIB has badly broken support for 64bit
integers on 32
bit platforms. Does anyone know of any Cygwin/Ming issues?

Is this only with the new snprintf code in Win32?

Yes.

Is this a problem with snprintf as implemented in src/port?

Yes. Only. It works with the snprintf() in the runtime (this particular
part).

Is there a reason why we don't use the snprintf that comes with the
various C compilers?

It does not support "positional parameters" (I think it's called) which
is required for proper translations.
We do use that one when it works...

//Magnus

On Tue, 1 Mar 2005 15:38:58 -0500 (EST), pgsql@mohawksoft.com
<pgsql@mohawksoft.com> wrote:

Is there a reason why we don't use the snprintf that comes with the
various C compilers?

snprintf() is usually buried in OS libraries. We implement
our own snprintf to make things like this:
snprintf(buf,"%2$s %1$s","world","Hello");
which is not supported on some platforms work.

We do it for national language translation of
messages. In some languages you may need
to change order of parameters to make a meaningful
sentence.

Another question is why we are using it for printing
values from database. I am not too good on function
overriding in standard C but maybe there is a way
to usage of standard snprintf() in a particular place.

On Tue, 1 Mar 2005 15:38:58 -0500 (EST), pgsql@mohawksoft.com
<pgsql@mohawksoft.com> wrote:

Is there a reason why we don't use the snprintf that comes with the
various C compilers?

snprintf() is usually buried in OS libraries. We implement
our own snprintf to make things like this:
snprintf(buf,"%2$s %1$s","world","Hello");
which is not supported on some platforms work.

We do it for national language translation of
messages. In some languages you may need
to change order of parameters to make a meaningful
sentence.

Another question is why we are using it for printing
values from database. I am not too good on function
overriding in standard C but maybe there is a way
to usage of standard snprintf() in a particular place.

Well, here is a stupid question, do we even know which snprintf we are
using on Win32? May it be possible that we are using the MingW version
which may be broken?

pgsql@mohawksoft.com writes:

Well, here is a stupid question, do we even know which snprintf we are
using on Win32? May it be possible that we are using the MingW version
which may be broken?

The regression tests were not failing until we started using the port/
version ...

regards, tom lane

Hi,

On Tuesday 01 March 2005 21:44, you wrote:

I spent all day debugging it. Still have absolutely
no idea what could possibly go wrong. Does
anyone have a slightest clue what can it be and
why it manifests itself only on win32?

It may be that the CLIB has badly broken support for 64bit
integers on 32
bit platforms. Does anyone know of any Cygwin/Ming issues?

Is this only with the new snprintf code in Win32?

Yes.

Is this a problem with snprintf as implemented in src/port?

Yes. Only. It works with the snprintf() in the runtime (this particular
part).

Is there a reason why we don't use the snprintf that comes with the
various C compilers?

It does not support "positional parameters" (I think it's called) which
is required for proper translations.
We do use that one when it works...

//Magnus

Some stupid idea just crossed my mind: what if the /ports version just
re-arranges the va_list according to the positional args and calls
vsnprintf()?
At least we know compiler and library...

Or, another idea: why not format the va_args individually using the original
format specifiers alone (without positional args), and assemble the resulting
string?

Am I on dope? Or does this sound at least doable?

Didn't code too much C lately...

Greetings,
J�rg
--
Leading SW developer - S.E.A GmbH
Mail: joerg.hessdoerfer@sea-gmbh.com
WWW: http://www.sea-gmbh.com

On Tue, 1 Mar 2005 22:42:52 +0100, Joerg Hessdoerfer
<Joerg.Hessdoerfer@sea-gmbh.com> wrote:

Some stupid idea just crossed my mind: what if the /ports version just
re-arranges the va_list according to the positional args and calls
vsnprintf()?
At least we know compiler and library...

I thought about it a lot. Some platforms do not support
all of % formant strings. src/port/snprintf.c is used both
for the platforms that do not support all necessary
% modifiers and the ones that do not support %n$
modifiers. Here is a comment from configure:

# If we found "long int" is 64 bits, assume snprintf handles it. If
# we found we need to use "long long int", better check. We cope with
# snprintfs that use %lld, %qd, or %I64d as the format. If none of these
# work, fall back to our own snprintf emulation (which we know uses %lld).

Any comments on this idea? Should we have two
versions of snprintf.c for these occasions?

Or, another idea: why not format the va_args individually using the original
format specifiers alone (without positional args), and assemble the resulting
string?

That is exactly what the code does :)

Greetings,
Jörg

Nick

Joerg Hessdoerfer <Joerg.Hessdoerfer@sea-gmbh.com> writes:

Some stupid idea just crossed my mind: what if the /ports version just
re-arranges the va_list according to the positional args and calls
vsnprintf()?

Having looked at the current snprintf.c, I don't actually believe that
it works at all in the presence of positional parameter specs. It picks
up the arguments in the order they are mentioned in the format string,
which is exactly not what it's supposed to do, if I understand the
concept of positional specifiers properly. This is certain to give the
wrong answers when the arguments are of different widths.

I'm also less than thrilled with the 300K+ local array ;-). I don't see
any point in saving the width specs from the first pass to the second
when they are not needed in the first pass. NL_ARGMAX could have a more
sane value (surely not more than a couple hundred) and we need some
checks that it's not exceeded in any case. On the other side of the
coin, the hardwired 4K limit in printf() is certainly *not* enough.

I think a correct implementation is probably a three-pass affair:

1. Scan format string to determine the basic datatypes (int, float, char*,
etc) of each actual parameter and their positions. Note that runtime
width and precision specs have to be counted as actual parameters.

2. Perform the va_arg() fetches in position order, and stash the actual
parameter values into a local array.

3. Rescan format string and do the outputting.

I don't offhand see what is making the regression tests fail (it's not
the above because the problem occurs with a nonpositional format string);
but there's not much point in trying to get rid of porting issues when
the fundamental algorithm is wrong.

regards, tom lane

On Tue, 1 Mar 2005 16:20:50 -0500 (EST), pgsql@mohawksoft.com
<pgsql@mohawksoft.com> wrote:

Well, here is a stupid question, do we even know which snprintf we are
using on Win32? May it be possible that we are using the MingW version
which may be broken?

Defenitely not. I checked it by placing debugging print
statements in code.

Nicolai Tufar <ntufar@gmail.com> writes:

Amazingly enough HAVE_LONG_LONG_INT_64 is
defined when compilation comes to src/port/snprintf.c
but the result is still wrong. I looked into configure.in
but the check for HAVE_LONG_LONG_INT_64 is too
complicated for me to understand. Bruce, could you
take a look at this? I am 90% sure it is an issue with
some configure definitions.

Just out of curiosity, do either HAVE_INT64 or HAVE_UINT64 get set
in pg_config.h? The observed symptoms would be explained if typedef
int64 were ending up as "long" rather than "long long". Looking at
the #ifdef nest in include/c.h, there are a couple of ways that could
happen, including importing a definition from system header files.

If this were happening, it would presumably break all int8 math not
only snprintf, so I'm not sure it's the story. As far as I've seen,
no one has actually posted the regression diffs seen in this failure,
so most of us are in the dark about the details of the problem.

regards, tom lane

Having looked at the current snprintf.c, I don't actually believe that
it works at all in the presence of positional parameter specs. It picks
up the arguments in the order they are mentioned in the format string,
which is exactly not what it's supposed to do, if I understand the
concept of positional specifiers properly. This is certain to give the
wrong answers when the arguments are of different widths.

It picks up arguments in order of appearance, places them in
array then shuffles them according to %n$ positional parameter.
I checked it with in many different combinations, it works!

I'm also less than thrilled with the 300K+ local array ;-). I don't see
any point in saving the width specs from the first pass to the second
when they are not needed in the first pass. NL_ARGMAX could have a more
sane value (surely not more than a couple hundred) and we need some
checks that it's not exceeded in any case. On the other side of the
coin, the hardwired 4K limit in printf() is certainly *not* enough.

How would one solve this issue. Calling malloc() from a print function
would be rather expensive. Maybe call snprintf() from printf() and
see if resulting string is 4K long then allocate a new buffer and
call snprintf() again? And by the way, what should I use malloc()
or palloc()?

What would you think will be a good value for NL_ARGMAX?

I think a correct implementation is probably a three-pass affair:

1. Scan format string to determine the basic datatypes (int, float, char*,
etc) of each actual parameter and their positions. Note that runtime
width and precision specs have to be counted as actual parameters.

2. Perform the va_arg() fetches in position order, and stash the actual
parameter values into a local array.

I actually do it. But in one step. I left the scanning loop in place
but replaced calls to actual printing functions with code to fill
the array, preserving width and precision. Plus I store pointers
to beginning and endings of format placeholders to not to have
to scan format string again in the next step.

3. Rescan format string and do the outputting.

My version: reorder the array and do the outputting.
/* shuffle pointers */ comment in source is misleading,
it should have been /* reorder pointers */.

I don't offhand see what is making the regression tests fail (it's not
the above because the problem occurs with a nonpositional format string);
but there's not much point in trying to get rid of porting issues when
the fundamental algorithm is wrong.

I believe my algorithm is working and it is faster than your proposed algorithm.
But if it is not acceptable I am willing to change as deemed necessary. I tested
the function separately and it passed regression tests on many platforms.
Situation with win32 is really unusual. We may need a win32 and MinGG internals
guru to have a look a the issue.

regards, tom lane

Nicolai Tufar

On Tue, 01 Mar 2005 17:45:31 -0500, Tom Lane > Just out of curiosity,
do either HAVE_INT64 or HAVE_UINT64 get set

in pg_config.h?

pg_config.h is attached. What drew my attention is the
following declaration:

/* Define to 1 if `long long int' works and is 64 bits. */
#define HAVE_LONG_LONG_INT_64

is it normal? should it not be like this:
#define HAVE_LONG_LONG_INT_64 1

Nicolai Tufar <ntufar@gmail.com> writes:

Having looked at the current snprintf.c, I don't actually believe that
it works at all in the presence of positional parameter specs.

It picks up arguments in order of appearance, places them in
array then shuffles them according to %n$ positional parameter.
I checked it with in many different combinations, it works!

Did you try combinations of parameters of different sizes? For instance

snprintf(..., "%g %d", doubleval, intval);
and
snprintf(..., "%2$d %1$g", doubleval, intval);

You cannot pick this up in order of appearance and expect it to work.
It might fail to fail on some machine architectures, but it's not going
to be portable.

On the other side of the
coin, the hardwired 4K limit in printf() is certainly *not* enough.

How would one solve this issue. Calling malloc() from a print function
would be rather expensive.

printf shouldn't use a buffer at all. I was thinking in terms of
passing around a state struct like

typedef struct {
char *output;
char *end;
FILE *outfile;
} printf_target;

and making dopr_outch look like

static void
dopr_outch(int c, printf_target *state)
{
if (state->output)
{
if (state->end == NULL || state->output < state->end)
*(state->output++) = c;
}
else
fputc(c, state->outfile);
}

regards, tom lane

On Tue, 01 Mar 2005 18:15:49 -0500, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Nicolai Tufar <ntufar@gmail.com> writes:

Having looked at the current snprintf.c, I don't actually believe that
it works at all in the presence of positional parameter specs.

It picks up arguments in order of appearance, places them in
array then shuffles them according to %n$ positional parameter.
I checked it with in many different combinations, it works!

Did you try combinations of parameters of different sizes? For instance

snprintf(..., "%g %d", doubleval, intval);
and
snprintf(..., "%2$d %1$g", doubleval, intval);

It works perfectly fine. Just checked.

You cannot pick this up in order of appearance and expect it to work.
It might fail to fail on some machine architectures, but it's not going
to be portable.

I did not modify the code that picked up the values.
I just modified it to instead of passing to printing function
store values in array.

On the other side of the
coin, the hardwired 4K limit in printf() is certainly *not* enough.

How would one solve this issue. Calling malloc() from a print function
would be rather expensive.

printf shouldn't use a buffer at all. I was thinking in terms of
passing around a state struct like

typedef struct {
char *output;
char *end;
FILE *outfile;
} printf_target;

and making dopr_outch look like

static void
dopr_outch(int c, printf_target *state)
{
if (state->output)
{
if (state->end == NULL || state->output < state->end)
*(state->output++) = c;
}
else
fputc(c, state->outfile);
}

I will consider it tomorrow. It is too late on this side of
the pond, I need to sleep. Thank you for your attention
and help.

best regards,
Nicolai Tufar

Nicolai Tufar <ntufar@gmail.com> writes:

On Tue, 01 Mar 2005 18:15:49 -0500, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Did you try combinations of parameters of different sizes? For instance

snprintf(..., "%g %d", doubleval, intval);
and
snprintf(..., "%2$d %1$g", doubleval, intval);

It works perfectly fine. Just checked.

Well, whatever architecture you're checking on may happen to make it
work, but that does *not* mean it's portable. On my machine it fails:

int main()
{
char buf[1000];
double d = 3.14159265358979;
int i = 42;

snprintf(buf, sizeof buf, "%.15g %d", d, i);
printf("result = '%s'\n", buf);
snprintf(buf, sizeof buf, "%2$d %1$.15g", d, i);
printf("result = '%s'\n", buf);
return 0;
}

"Correct" output with the system snprintf is

result = '3.14159265358979 42'
result = '42 3.14159265358979'

With CVS-tip snprintf I get

result = '3 42'
result = '3 3505'

The second value in the last line varies erratically from run to run,
presumably because it's picking up garbage. This output appears to
indicate some problems in passing around precision specs as well as the
values themselves...

regards, tom lane

Nicolai Tufar wrote:

On Tue, 01 Mar 2005 17:45:31 -0500, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Nicolai Tufar <ntufar@gmail.com> writes:
Just out of curiosity, do either HAVE_INT64 or HAVE_UINT64 get set
in pg_config.h? The observed symptoms would be explained if typedef
int64 were ending up as "long" rather than "long long". Looking at
the #ifdef nest in include/c.h, there are a couple of ways that could
happen, including importing a definition from system header files.

If this were happening, it would presumably break all int8 math not
only snprintf, so I'm not sure it's the story.

I am looking into it. Will report if find something of importance.

As far as I've seen,
no one has actually posted the regression diffs seen in this failure,
so most of us are in the dark about the details of the problem.

Regression diiff is attached. The problem is reported by Magnus
and me. Probably nobody else compiles pg under Win32 these
days.

I am testing the failure here. I will keep at it until I find the
cause.

The only downside is that Win32 compiles are much slower than Unix.

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

BTW, you should read the official spec for snprintf:

http://www.opengroup.org/onlinepubs/007908799/xsh/fprintf.html

There are a couple of interesting things that the present code is most
certainly not doing correctly, notably:

In format strings containing the %n$ form of conversion
specifications, numbered arguments in the argument list can be
referenced from the format string as many times as required.

Also it seems that runtime precision specs are required to have explicit
numbers when used in a %n$ string, which is something I didn't know
until just now. This example in the spec is instructive:
printf("%1$d:%2$.*3$d:%4$.*3$d\n", hour, min, precision, sec);

It might be a good idea to go look at whichever *BSD we got this code
from originally, and see if they've upgraded it to do %n$. 'Cause it
will take a nontrivial amount of work to get from where we are now to
something that follows the full spec.

regards, tom lane

I have some new information. If I add the attached patch to snprintf.c,
I should see see snprintf() being called printing "0", vsnprintf()
printing "1" and dopr(), "2". However, I only see "0" printing in the
server logs.

I think this means it is finding our /port/snprintf(), but when it calls
vsnprintf, it must be using some other version, probably the operating
system version that doesn't support %lld.

I am also attaching the 'nm' output from libpgport_srv.a which does show
vsnprintf as being defined.

Win32 doesn't like multiply defined symbols so we use
-Wl,--allow-multiple-definition to allow multiple symbols.

I bet if I define LONG_LONG_INT_FORMAT as '%I64d' it would pass the
regression tests. (I will test now.) Our config/c-library.m4 file
confirms that format for MinGW:

# MinGW uses '%I64d', though gcc throws an warning with -Wall,

The big question is why our own vsnprintf() is not being called from
snprintf() in our port file.

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

Bruce Momjian <pgman@candle.pha.pa.us> writes:

I think this means it is finding our /port/snprintf(), but when it calls
vsnprintf, it must be using some other version, probably the operating
system version that doesn't support %lld.

Ya know, I was wondering about that but dismissed it because the
routines were all declared in the same file. Windows' linker must
behave very oddly to do this.

Does it help if you flip the order of the snprintf and vsnprintf
functions in snprintf.c?

regards, tom lane

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

I think this means it is finding our /port/snprintf(), but when it calls
vsnprintf, it must be using some other version, probably the operating
system version that doesn't support %lld.

I can confirm that using "%I64d" for the printf format allows the
regression tests to pass for int8.

Ya know, I was wondering about that but dismissed it because the
routines were all declared in the same file. Windows' linker must
behave very oddly to do this.

Yep, quite unusual.

Does it help if you flip the order of the snprintf and vsnprintf
functions in snprintf.c?

Testing now.

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

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

I think this means it is finding our /port/snprintf(), but when it calls
vsnprintf, it must be using some other version, probably the operating
system version that doesn't support %lld.

Ya know, I was wondering about that but dismissed it because the
routines were all declared in the same file. Windows' linker must
behave very oddly to do this.

Does it help if you flip the order of the snprintf and vsnprintf
functions in snprintf.c?

Yes, it fixes the problem and I have applied the reordering with a
comment.

I will start working on fixing the large fmtpar allocations now.

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

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Tom Lane wrote:

Does it help if you flip the order of the snprintf and vsnprintf
functions in snprintf.c?

Yes, it fixes the problem and I have applied the reordering with a
comment.

Fascinating.

I will start working on fixing the large fmtpar allocations now.

Quite honestly, this code is not worth micro-optimizing because it
is fundamentally broken. See my other comments in this thread.

Can we find a BSD-license implementation that follows the spec?

regards, tom lane

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Tom Lane wrote:

Does it help if you flip the order of the snprintf and vsnprintf
functions in snprintf.c?

Yes, it fixes the problem and I have applied the reordering with a
comment.

Fascinating.

I will start working on fixing the large fmtpar allocations now.

Quite honestly, this code is not worth micro-optimizing because it
is fundamentally broken. See my other comments in this thread.

I am working on something that just counts the '%' characters in the
format string and allocates an array that size.

Can we find a BSD-license implementation that follows the spec?

I would think NetBSD would be our best bet. I will download it and take
a look.

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

Bruce Momjian wrote:

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Tom Lane wrote:

Does it help if you flip the order of the snprintf and vsnprintf
functions in snprintf.c?

Yes, it fixes the problem and I have applied the reordering with a
comment.

Fascinating.

I will start working on fixing the large fmtpar allocations now.

Quite honestly, this code is not worth micro-optimizing because it
is fundamentally broken. See my other comments in this thread.

I am working on something that just counts the '%' characters in the
format string and allocates an array that size.

Can we find a BSD-license implementation that follows the spec?

I would think NetBSD would be our best bet. I will download it and take
a look.

Oops, I remember now that NetBSD doesn't support it. I think FreeBSD does.

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

Bruce Momjian wrote:

Bruce Momjian wrote:

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Tom Lane wrote:

Does it help if you flip the order of the snprintf and vsnprintf
functions in snprintf.c?

Yes, it fixes the problem and I have applied the reordering with a
comment.

Fascinating.

I will start working on fixing the large fmtpar allocations now.

Quite honestly, this code is not worth micro-optimizing because it
is fundamentally broken. See my other comments in this thread.

I am working on something that just counts the '%' characters in the
format string and allocates an array that size.

Can we find a BSD-license implementation that follows the spec?

I would think NetBSD would be our best bet. I will download it and take
a look.

Oops, I remember now that NetBSD doesn't support it. I think FreeBSD does.

OK, I have the structure exceess patched at least with this applied
patch.

Have we considered what is going to happen to applications when they use
our snprintf instead of the one from the operating system? I don't
think it is going to always match and might cause confusion. Look at
the confusion is caused us.

Can we force only gettext() to call our special snprintf verions but
leave the application symbol space unchanged?

I just looked at my BSD/OS libpq based on CVS and see [v]snprintf
exported:

$ nm /pg/interfaces/libpq/libpq.so|grep snprintf
00052434 T snprintf
000523e0 T vsnprintf

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

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Have we considered what is going to happen to applications when they use
our snprintf instead of the one from the operating system?

Hmm ...

First line of thought: we surely must not insert a snprintf into
libpq.so unless it is 100% up to spec *and* has no performance issues
... neither of which can be claimed of the CVS-tip version.

Second line of thought: libpq already feels free to insert allegedly
up-to-spec versions of a number of things, and no one has complained.
Maybe the linker prevents problems by not linking these versions to
any calls from outside libpq?

Third thought: Windows' linker seems to be broken enough that it may
create problems of this ilk that exist on no other platform.

regards, tom lane

Tom lane wrote:

With CVS-tip snprintf I get
result = '3 42'
result = '3 3505'

I get similar results:
result = '3 42'
result = '9e-313 1413754129'

Now I agree with you, it is fundamentally broken.
We need to replace this implementation.

Bruce Momjian wrote:

I can confirm that using "%I64d" for the printf format allows the
regression tests to pass for int8.

But snprintf.c code does not support "%I64d" construct. It must
be picking OS's vsnprintf()

Bruce Momjian wrote:

I think FreeBSD does.

I started with FreeBSD's vsnprintf() at first
but was set back by it's complexity and decided to
modify the port/snprintf.c code. Now would you like me
to incorporate FreeBSD's one into the code.
Give me a week and I will come with the patch.

Best regards,
Nicolai Tufar

Nicolai Tufar <ntufar@gmail.com> writes:

I started with FreeBSD's vsnprintf() at first
but was set back by it's complexity and decided to
modify the port/snprintf.c code. Now would you like me
to incorporate FreeBSD's one into the code.
Give me a week and I will come with the patch.

It's all yours ...

regards, tom lane

Hi,

On Wednesday 02 March 2005 01:28, you wrote:

BTW, you should read the official spec for snprintf:

http://www.opengroup.org/onlinepubs/007908799/xsh/fprintf.html

There are a couple of interesting things that the present code is most
certainly not doing correctly, notably:

In format strings containing the %n$ form of conversion
specifications, numbered arguments in the argument list can be
referenced from the format string as many times as required.

Also it seems that runtime precision specs are required to have explicit
numbers when used in a %n$ string, which is something I didn't know
until just now. This example in the spec is instructive:
printf("%1$d:%2$.*3$d:%4$.*3$d\n", hour, min, precision, sec);

It might be a good idea to go look at whichever *BSD we got this code
from originally, and see if they've upgraded it to do %n$. 'Cause it
will take a nontrivial amount of work to get from where we are now to
something that follows the full spec.

don't know if PG borrowed the code from here, but according to the manpage
FreeBSD 5.3 seems to have a quite complete implementation, see
http://www.freebsd.org/cgi/man.cgi?query=snprintf&amp;apropos=0&amp;sektion=3&amp;manpath=FreeBSD+5.3-RELEASE+and+Ports&amp;format=html

Would this help?

Greetings,
J�rg
--
Leading SW developer - S.E.A GmbH
Mail: joerg.hessdoerfer@sea-gmbh.com
WWW: http://www.sea-gmbh.com

On Wed, 2 Mar 2005 09:24:32 +0100, Joerg Hessdoerfer
<Joerg.Hessdoerfer@sea-gmbh.com> wrote:

don't know if PG borrowed the code from here, but according to the manpage
FreeBSD 5.3 seems to have a quite complete implementation, see
http://www.freebsd.org/cgi/man.cgi?query=snprintf&amp;apropos=0&amp;sektion=3&amp;manpath=FreeBSD+5.3-RELEASE+and+Ports&amp;format=html

Would this help?

Yes, it would. With Tom Lane's blessing I am starting on incorporating it into
PG now.

The big question is why our own vsnprintf() is not being called from
snprintf() in our port file.

I have seen this "problem" before, well, it isn't really a problem I guess.

I'm not sure of the gcc compiler options, but....

On the Microsoft compiler if you specify the option "/Gy" it separates the
functions within the object file, that way you don't load all the
functions from the object if they are not needed.

If, however, you create a function with the same name as another function,
and one is declared in an object compiled with the "/Gy" option, and the
other's object file is not, then if you also use a different function or
reference variable in the object file compiled without the "/Gy" option,
then the conflicting function will probably be used. Make sense?

I would suggest using macro to redefine snprintf and vnsprintf to avoid
the issue:

#define snprintf pg_snprintf
#define vnsprintf pg_vnsprintf

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Have we considered what is going to happen to applications when they use
our snprintf instead of the one from the operating system?

Hmm ...

First line of thought: we surely must not insert a snprintf into
libpq.so unless it is 100% up to spec *and* has no performance issues
... neither of which can be claimed of the CVS-tip version.

Agreed, and we have to support all the 64-bit specifications a port
might support like %qd and %I64d as well as %lld. I have added that to
our current CVS version.

Second line of thought: libpq already feels free to insert allegedly
up-to-spec versions of a number of things, and no one has complained.
Maybe the linker prevents problems by not linking these versions to
any calls from outside libpq?

I just tested on BSD/OS and a program with a single printf() call does
call our printf if libpq is used on the link line. The program makes no
libpq calls at all.

Third thought: Windows' linker seems to be broken enough that it may
create problems of this ilk that exist on no other platform.

Yes, strangly the Window's linker is fine because libpqdll.def defines
what symbols are exported. I don't think Unix has that capability.

Is there any way we can have just gettext() call our snprintf under a
special name?

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

Yes, strangly the Window's linker is fine because libpqdll.def defines
what symbols are exported. I don't think Unix has that capability.

A non-static "public" function in a Windows DLL is not available for
dynamic linking unless explicitly declared as dll export. This behavior is
completely different than UNIX shared libraries.

Windows static libraries operate completely differently than Windows DLLs,
they work like their UNIX equivilents.

So, if you create an snprintf function in code that will be in both a
static and dynamic library, the static library may have conflicts where as
the dynamic one will not.

Don't you love Windows?

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Tom Lane wrote:

First line of thought: we surely must not insert a snprintf into
libpq.so unless it is 100% up to spec *and* has no performance issues
... neither of which can be claimed of the CVS-tip version.

Agreed, and we have to support all the 64-bit specifications a port
might support like %qd and %I64d as well as %lld. I have added that to
our current CVS version.

I really dislike that idea and request that you revert it.

Is there any way we can have just gettext() call our snprintf under a
special name?

The issue only comes up in libpq --- in the backend there is no reason
that snprintf can't be our snprintf, and likewise in self-contained
programs like psql. It might be worth the pain-in-the-neck quality to
have libpq refer to the functions as pq_snprintf etc. Perhaps we could
do this via macros

#define snprintf pq_snprintf

and not have to uglify the source code.

regards, tom lane

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Tom Lane wrote:

First line of thought: we surely must not insert a snprintf into
libpq.so unless it is 100% up to spec *and* has no performance issues
... neither of which can be claimed of the CVS-tip version.

Agreed, and we have to support all the 64-bit specifications a port
might support like %qd and %I64d as well as %lld. I have added that to
our current CVS version.

I really dislike that idea and request that you revert it.

Is there any way we can have just gettext() call our snprintf under a
special name?

The issue only comes up in libpq --- in the backend there is no reason
that snprintf can't be our snprintf, and likewise in self-contained
programs like psql. It might be worth the pain-in-the-neck quality to
have libpq refer to the functions as pq_snprintf etc. Perhaps we could
do this via macros

#define snprintf pq_snprintf

Didn't I suggest that earlier? :) Also, since it is vsnprintf that seems
to be a bigger issue:

#define vsnprintf pq_vsnprintf

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Tom Lane wrote:

First line of thought: we surely must not insert a snprintf into
libpq.so unless it is 100% up to spec *and* has no performance issues
... neither of which can be claimed of the CVS-tip version.

Agreed, and we have to support all the 64-bit specifications a port
might support like %qd and %I64d as well as %lld. I have added that to
our current CVS version.

I really dislike that idea and request that you revert it.

Done.

Is there any way we can have just gettext() call our snprintf under a
special name?

The issue only comes up in libpq --- in the backend there is no reason
that snprintf can't be our snprintf, and likewise in self-contained
programs like psql. It might be worth the pain-in-the-neck quality to
have libpq refer to the functions as pq_snprintf etc. Perhaps we could
do this via macros

#define snprintf pq_snprintf

and not have to uglify the source code.

Yes, this is what I was thinking of too. I think it would need a macro
in libpq to map the libc names to the pq_* names, and a separate /port C
file that maps the normal libc names to the pg_* names. For client
applications and the backend, this new C file would catch all the
snprintf calls, while for libpq the pg_* calls would be used directly
and the new C file with the libc symbols would not be pulled in.

Does that sound like a plan?

The reason we can't just use the macro everwhere is that we don't want
applications using libpq to all be using pg_* functions, only psql and
our own. The only other solution I can think of is to make sure all
client apps use FRONTEND as a define and trigger the macros from libc
names to pg_* names on that.

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

Dear all,
After struggling for one week to to integrate FreeBSD's vfprintf.c into
PostgreSQL I finally gave up. It is too dependent on underlying
FreeBSD system functions. To incorporate it into PostgreSQL we need
to move vfprintf.c file itself, two dozen files form gdtoa and a half
a dozen __XXtoa.c files scattered in apparently random fashion all
around FreeBSD source tree.

Instead I researched some other implementations of snprintf on
the web released under a license compatible with PostgreSQL's.
The most suitable one I have come upon is Trio
[http://daniel.haxx.se/projects/trio/].
It is distributed under a MIT-like license which, I think will be
compatible with us.

What do you think about it? Shall I abandon FreeBSD and go ahead
ıncorporatıng Trıo?

And by the way, what ıs the conclusıon of snprıntf() vs. pg_snprintf()
and UNIX libraries discussion a week ago? Which one shall
I implement?

Regards,
Nicolai Tufar

From what I recall from the conversation, I would say rename the vsprintf
and the sprintf functions in postgres to pq_vsnprintf and pq_snprintf.
Define a couple macros: (in some common header, pqprintf.h?)

#define snprintf pq_snprintf
#define vsnprintf pq_snprintf

Then just maintain the postgres forms of printf which have seemed to be OK
except that on Win32 vnsprintf, although in the same object file was not
being used.

Nicolai Tufar wrote:

Dear all,
After struggling for one week to to integrate FreeBSD's vfprintf.c into
PostgreSQL I finally gave up. It is too dependent on underlying
FreeBSD system functions. To incorporate it into PostgreSQL we need
to move vfprintf.c file itself, two dozen files form gdtoa and a half
a dozen __XXtoa.c files scattered in apparently random fashion all
around FreeBSD source tree.

Instead I researched some other implementations of snprintf on
the web released under a license compatible with PostgreSQL's.
The most suitable one I have come upon is Trio
[http://daniel.haxx.se/projects/trio/].
It is distributed under a MIT-like license which, I think will be
compatible with us.

What do you think about it? Shall I abandon FreeBSD and go ahead
?ncorporat?ng Tr?o?

Yes, maybe just add the proper %$ handling from Trio to what we have
now.

And by the way, what ?s the conclus?on of snpr?ntf() vs. pg_snprintf()
and UNIX libraries discussion a week ago? Which one shall
I implement?

I think the proper direction is not to export snprintf() from libpq so
that user applications will use the native snprintf, but our code can
use our custom version.

I will work on a patch now.

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

On Wed, 9 Mar 2005 22:51:27 -0500 (EST), Bruce Momjian
<pgman@candle.pha.pa.us> wrote:

What do you think about it? Shall I abandon FreeBSD and go ahead
Incorporating Trio?

Yes, maybe just add the proper %$ handling from Trio to what we have
now.

Adding proper %$ from Trio will require too much effort. I would
rather not do it. Not because I am lazy but because:

1) Trio team seem to be very serious about standards, update
the library as soon as new standards come out:
<quote>
Trio fully implements the C99 (ISO/IEC 9899:1999) and UNIX98 (the
Single Unix Specification, Version 2) standards, as well as many
features from other implementations, e.g. the GNU libc and BSD4.
</quote>

2) If we integrate the whole library in source code we will
not have to maintain it and will rely on Trio team for bug fixes
and updates. Integrating it will be very easy since all of the
functions begin with "trio_". I used it instead of the src/port/snrpintf.c
one and it passes regression tests under Win32 just fine.

The downside is that Trio library is rather big. It is 3 .c and 6 .h
files totalling 11556 lines. Compiled it is 71224 bytes not stripped
and 56204 bytes stripped on Solaris 10 for x86, 32-bit. Even for
a shared library it will probably be too much. Trio has a lot
of string handling functions which are probably not necessary.
Would you like me to try to remove everything unnecessary from
it or we will settle with the full version?

Regards,
Nicolai Tufar

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Tom Lane wrote:

First line of thought: we surely must not insert a snprintf into
libpq.so unless it is 100% up to spec *and* has no performance issues
... neither of which can be claimed of the CVS-tip version.

Agreed, and we have to support all the 64-bit specifications a port
might support like %qd and %I64d as well as %lld. I have added that to
our current CVS version.

I really dislike that idea and request that you revert it.

Is there any way we can have just gettext() call our snprintf under a
special name?

The issue only comes up in libpq --- in the backend there is no reason
that snprintf can't be our snprintf, and likewise in self-contained
programs like psql. It might be worth the pain-in-the-neck quality to
have libpq refer to the functions as pq_snprintf etc. Perhaps we could
do this via macros

#define snprintf pq_snprintf

and not have to uglify the source code.

OK, here is a patch that changes snprintf() to pg_snprintf() for
platforms that need our snprintf. We do the same thing with
pgrename/pgunlink.

It manages to preserve printf-like argument checking in gcc.

I considered using a macro just in libpq but realized it was going to be
too ugly and too easy to break without us detecting 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, Pennsylvania 19073

Nicolai Tufar wrote:

On Wed, 9 Mar 2005 22:51:27 -0500 (EST), Bruce Momjian
<pgman@candle.pha.pa.us> wrote:

What do you think about it? Shall I abandon FreeBSD and go ahead
Incorporating Trio?

Yes, maybe just add the proper %$ handling from Trio to what we have
now.

Adding proper %$ from Trio will require too much effort. I would
rather not do it. Not because I am lazy but because:

1) Trio team seem to be very serious about standards, update
the library as soon as new standards come out:
<quote>
Trio fully implements the C99 (ISO/IEC 9899:1999) and UNIX98 (the
Single Unix Specification, Version 2) standards, as well as many
features from other implementations, e.g. the GNU libc and BSD4.
</quote>

2) If we integrate the whole library in source code we will
not have to maintain it and will rely on Trio team for bug fixes
and updates. Integrating it will be very easy since all of the
functions begin with "trio_". I used it instead of the src/port/snrpintf.c
one and it passes regression tests under Win32 just fine.

The downside is that Trio library is rather big. It is 3 .c and 6 .h
files totalling 11556 lines. Compiled it is 71224 bytes not stripped
and 56204 bytes stripped on Solaris 10 for x86, 32-bit. Even for
a shared library it will probably be too much. Trio has a lot
of string handling functions which are probably not necessary.
Would you like me to try to remove everything unnecessary from
it or we will settle with the full version?

Please see my posting about using a macro for snprintf. If the current
implementation of snprintf is enough for our existing translation users
we probably don't need to add anything more to it because snprintf will
not be exported to client applications.

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

On Thu, 10 Mar 2005 16:26:47 -0500 (EST), Bruce Momjian
<pgman@candle.pha.pa.us> wrote:

Please see my posting about using a macro for snprintf. If the current
implementation of snprintf is enough for our existing translation users
we probably don't need to add anything more to it because snprintf will
not be exported to client applications.

Oh, Bruce. It will be the best solution. I was worried about
the problems with my modifications to snprintf.c Tom Lane
pointed out. But if we really separate snprintf() used by
messages and snprintf() used by the like of
src/backend/utils/adt/int8.c then we are safe. We can claim
current release safe and I will modify src/port/snprintf.c at my
leisure later. I will try out your modifications tomorrow. It
is late here and I have a PostgreSQL class to to teach
tomorrow ;)

I still think that it is more convenient to rip off current
implementation of snprintf.c and replace it with a very much
stripped down of Trio's one. I will work on it and try to get
a patch in one week's time. Thank you all for your patience.

Best regards,
Nicolai Tufar

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Please see my posting about using a macro for snprintf. If the current
implementation of snprintf is enough for our existing translation users
we probably don't need to add anything more to it because snprintf will
not be exported to client applications.

The CVS-tip implementation is fundamentally broken and won't work even
for our internal uses. I've not wasted time complaining about it
because I thought we were going to replace it. If we can't find a
usable replacement then we're going to have to put a lot of effort
into fixing what's there. On the whole I think the effort would be
better spent importing someone else's solution.

regards, tom lane

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Please see my posting about using a macro for snprintf. If the current
implementation of snprintf is enough for our existing translation users
we probably don't need to add anything more to it because snprintf will
not be exported to client applications.

The CVS-tip implementation is fundamentally broken and won't work even
for our internal uses. I've not wasted time complaining about it
because I thought we were going to replace it. If we can't find a
usable replacement then we're going to have to put a lot of effort
into fixing what's there. On the whole I think the effort would be
better spent importing someone else's solution.

Oh, so our existing implementation doesn't even meet our needs. OK.

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

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Please see my posting about using a macro for snprintf. If the

current

implementation of snprintf is enough for our existing translation

users

we probably don't need to add anything more to it because snprintf

will

not be exported to client applications.

The CVS-tip implementation is fundamentally broken and won't work even
for our internal uses. I've not wasted time complaining about it
because I thought we were going to replace it. If we can't find a
usable replacement then we're going to have to put a lot of effort
into fixing what's there. On the whole I think the effort would be
better spent importing someone else's solution.

Oh, so our existing implementation doesn't even meet our needs. OK.

Wasn't the issue about odd behavior of the Win32 linker choosing the wrong
vnsprintf?

pgsql@mohawksoft.com writes:

Please see my posting about using a macro for snprintf.

Wasn't the issue about odd behavior of the Win32 linker choosing the wrong
vnsprintf?

You're right, the point about the macro was to avoid linker weirdness on
Windows. We need to do that part in any case. I think Bruce confused
that issue with the one about whether our version supported %n$
adequately ... which it doesn't just yet ...

regards, tom lane

pgsql@mohawksoft.com wrote:

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Please see my posting about using a macro for snprintf. If the

current

implementation of snprintf is enough for our existing translation

users

we probably don't need to add anything more to it because snprintf

will

not be exported to client applications.

The CVS-tip implementation is fundamentally broken and won't work even
for our internal uses. I've not wasted time complaining about it
because I thought we were going to replace it. If we can't find a
usable replacement then we're going to have to put a lot of effort
into fixing what's there. On the whole I think the effort would be
better spent importing someone else's solution.

Oh, so our existing implementation doesn't even meet our needs. OK.

Wasn't the issue about odd behavior of the Win32 linker choosing the wrong
vnsprintf?

Ah, but with my new patch to be applied tomorrow to use macros and
rename to pg_snprintf there no longer is any conflict with the system
versions of these functions.

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

Tom Lane wrote:

The CVS-tip implementation is fundamentally broken and won't work even
for our internal uses. I've not wasted time complaining about it
because I thought we were going to replace it. If we can't find a
usable replacement then we're going to have to put a lot of effort
into fixing what's there. On the whole I think the effort would be
better spent importing someone else's solution.

Bruce Momjian wrote:

Oh, so our existing implementation doesn't even meet our needs. OK.

Very well, I too, tend to think that importing some else's solution
is the way to go. Tom, have you looked at Trio? If it is fine with you too,
I will strip it to the bare minimum needed for snprintf(), vsnprintf() and
printf() by Saturday.

Regards,
Nicolai Tufar

Nicolai Tufar <ntufar@gmail.com> writes:

Very well, I too, tend to think that importing some else's solution
is the way to go. Tom, have you looked at Trio?

I have not seen it ... but if it looks reasonable to you, have a go
at it.

regards, tom lane

On Fri, 11 Mar 2005 01:14:31 -0500, Tom Lane wrote:

Nicolai Tufar <ntufar@gmail.com> writes:

Very well, I too, tend to think that importing some else's solution
is the way to go. Tom, have you looked at Trio?

I have not seen it ... but if it looks reasonable to you, have a go
at it.

It looks reasonable to me. It claims to: fully implement C99 (ISO/IEC
9899:1999) and UNIX98 (the
Single Unix Specification, Version 2) standards, as well as many
features from other implementations, e.g. the GNU libc and BSD4.

I compiled and run regression tests with it used instead of
our current implementation and it worked fine under win32 and
Solaris x86. The only problem is that it is 11000 lines as
oposed to our curent implementation's 600. I will remove
everything unnecessary and submit a patch for consideration.

Regards,
Nicolai Tufar

Tom Lane wrote:

pgsql@mohawksoft.com writes:

Please see my posting about using a macro for snprintf.

Wasn't the issue about odd behavior of the Win32 linker choosing the wrong
vnsprintf?

You're right, the point about the macro was to avoid linker weirdness on
Windows. We need to do that part in any case. I think Bruce confused
that issue with the one about whether our version supported %n$
adequately ... which it doesn't just yet ...

Perhaps I am reading old email in this reply but I thought I should
clarify:

Once we do:

#define vsnprintf(...) pg_vsnprintf(__VA_ARGS__)
#define snprintf(...) pg_snprintf(__VA_ARGS__)
#define printf(...) pg_printf(__VA_ARGS__)

we also rename the functions in snprintf.c to pg_* names so there is no
longer a conflict with the system libc versions.

The macro is to prevent our snprintf from leaking out of libraries like
libpq, not to fix the win32 linker problem, which we already had fixed
by reordering the entries in the C file.

Perhaps the macro idea originally came as a fix for Win32 but it is much
larger that that now.

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

Nicolai Tufar wrote:

On Thu, 10 Mar 2005 16:26:47 -0500 (EST), Bruce Momjian
<pgman@candle.pha.pa.us> wrote:

Please see my posting about using a macro for snprintf. If the current
implementation of snprintf is enough for our existing translation users
we probably don't need to add anything more to it because snprintf will
not be exported to client applications.

Oh, Bruce. It will be the best solution. I was worried about
the problems with my modifications to snprintf.c Tom Lane
pointed out. But if we really separate snprintf() used by
messages and snprintf() used by the like of
src/backend/utils/adt/int8.c then we are safe. We can claim
current release safe and I will modify src/port/snprintf.c at my
leisure later. I will try out your modifications tomorrow. It
is late here and I have a PostgreSQL class to to teach
tomorrow ;)

I still think that it is more convenient to rip off current
implementation of snprintf.c and replace it with a very much
stripped down of Trio's one. I will work on it and try to get
a patch in one week's time. Thank you all for your patience.

I am not heading in the direction of using a different snprintf for
messages and for int8.c. I am just renaming the calls via macros so we
don't leak snprintf from libpq.

One new idea I had was to have pg_snprintf() look over the format string
and adjust the arguments to match what the format string is requesting,
remove %$ from the format string, and then pass it to the native libc
snprintf(). That might be the easiest solution for the many platforms
with a good snprintf but not %$ support.

Is that possible/easier?

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

Tom Lane wrote:

pgsql@mohawksoft.com writes:

Please see my posting about using a macro for snprintf.

Wasn't the issue about odd behavior of the Win32 linker choosing the

wrong

vnsprintf?

You're right, the point about the macro was to avoid linker weirdness on
Windows. We need to do that part in any case. I think Bruce confused
that issue with the one about whether our version supported %n$
adequately ... which it doesn't just yet ...

Perhaps I am reading old email in this reply but I thought I should
clarify:

Once we do:

#define vsnprintf(...) pg_vsnprintf(__VA_ARGS__)
#define snprintf(...) pg_snprintf(__VA_ARGS__)
#define printf(...) pg_printf(__VA_ARGS__)

I'm not sure that macros can have variable number of arguments on all
supported platforms. I've been burnt by this before.

pgsql@mohawksoft.com wrote:

Tom Lane wrote:

pgsql@mohawksoft.com writes:

Please see my posting about using a macro for snprintf.

Wasn't the issue about odd behavior of the Win32 linker choosing the

wrong

vnsprintf?

You're right, the point about the macro was to avoid linker weirdness on
Windows. We need to do that part in any case. I think Bruce confused
that issue with the one about whether our version supported %n$
adequately ... which it doesn't just yet ...

Perhaps I am reading old email in this reply but I thought I should
clarify:

Once we do:

#define vsnprintf(...) pg_vsnprintf(__VA_ARGS__)
#define snprintf(...) pg_snprintf(__VA_ARGS__)
#define printf(...) pg_printf(__VA_ARGS__)

I'm not sure that macros can have variable number of arguments on all
supported platforms. I've been burnt by this before.

The actual patch is:

	+ #ifdef __GNUC__
	+ #define vsnprintf(...)        pg_vsnprintf(__VA_ARGS__)
	+ #define snprintf(...) pg_snprintf(__VA_ARGS__)
	+ #define printf(...)           pg_printf(__VA_ARGS__)
	+ #else
	+ #define vsnprintf             pg_vsnprintf
	+ #define snprintf              pg_snprintf
	+ #define printf                pg_printf
	+ #endif

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

Bruce Momjian <pgman@candle.pha.pa.us> writes:

I'm not sure that macros can have variable number of arguments on all
supported platforms. I've been burnt by this before.

The actual patch is:

+ #ifdef __GNUC__
+ #define vsnprintf(...)        pg_vsnprintf(__VA_ARGS__)
+ #define snprintf(...) pg_snprintf(__VA_ARGS__)
+ #define printf(...)           pg_printf(__VA_ARGS__)
+ #else
+ #define vsnprintf             pg_vsnprintf
+ #define snprintf              pg_snprintf
+ #define printf                pg_printf
+ #endif

Uh, why bother with the different approach for gcc?

Also, what happened to fprintf? We're going to need that too for
localization of the client programs.

regards, tom lane

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

I'm not sure that macros can have variable number of arguments on all
supported platforms. I've been burnt by this before.

The actual patch is:

+ #ifdef __GNUC__
+ #define vsnprintf(...)        pg_vsnprintf(__VA_ARGS__)
+ #define snprintf(...) pg_snprintf(__VA_ARGS__)
+ #define printf(...)           pg_printf(__VA_ARGS__)
+ #else
+ #define vsnprintf             pg_vsnprintf
+ #define snprintf              pg_snprintf
+ #define printf                pg_printf
+ #endif

Uh, why bother with the different approach for gcc?

Because if we don't do that then the code above fails:

extern int pg_snprintf(char *str, size_t count, const char *fmt,...)
/* This extension allows gcc to check the format string */
__attribute__((format(printf, 3, 4)));

The issue is that the "printf" here is interpreted specially by the
compiler to mean "check arguments as printf". If the preprocessor
changes that, we get a failure. The good news is that only gcc supports
arg checking using __attribute__ and it also supports the __VA_ARGS__
macros. What I think we do lose is argument checking for non-gcc, but
this seems as close as we can get.

Also, what happened to fprintf? We're going to need that too for
localization of the client programs.

It was never there. I will add it now.

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

Bruce Momjian wrote:

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

I'm not sure that macros can have variable number of arguments on all
supported platforms. I've been burnt by this before.

The actual patch is:

+ #ifdef __GNUC__
+ #define vsnprintf(...)        pg_vsnprintf(__VA_ARGS__)
+ #define snprintf(...) pg_snprintf(__VA_ARGS__)
+ #define printf(...)           pg_printf(__VA_ARGS__)
+ #else
+ #define vsnprintf             pg_vsnprintf
+ #define snprintf              pg_snprintf
+ #define printf                pg_printf
+ #endif

Uh, why bother with the different approach for gcc?

Because if we don't do that then the code above fails:

extern int pg_snprintf(char *str, size_t count, const char *fmt,...)
/* This extension allows gcc to check the format string */
__attribute__((format(printf, 3, 4)));

The issue is that the "printf" here is interpreted specially by the
compiler to mean "check arguments as printf". If the preprocessor
changes that, we get a failure. The good news is that only gcc supports
arg checking using __attribute__ and it also supports the __VA_ARGS__
macros. What I think we do lose is argument checking for non-gcc, but
this seems as close as we can get.

I am adding a comment explaining why those macros are used.

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

On Thu, 10 Mar 2005 19:21:41 -0500 (EST), Bruce Momjian
<pgman@candle.pha.pa.us> wrote:

The CVS-tip implementation is fundamentally broken and won't work even
for our internal uses. I've not wasted time complaining about it
because I thought we were going to replace it. If we can't find a
usable replacement then we're going to have to put a lot of effort
into fixing what's there. On the whole I think the effort would be
better spent importing someone else's solution.

Oh, so our existing implementation doesn't even meet our needs. OK.

Which made me wander why did I not aggree with
Tom Lane's suggestion to make do three passes
instead of two. Tom was right, as usual. It happened to
be much easier than I expected. The patch is attached.
Please apply.

Tom, what do you think? Will it be fine with you?

Best regards,
Nicolai

Resubmission of yesterday's patch so that it would
cont conflict with Bruce's cvs commit. Pleas apply.

Best regards,
Nicolai.

Nicolai Tufar wrote:

On Thu, 10 Mar 2005 19:21:41 -0500 (EST), Bruce Momjian
<pgman@candle.pha.pa.us> wrote:

The CVS-tip implementation is fundamentally broken and won't work even
for our internal uses. I've not wasted time complaining about it
because I thought we were going to replace it. If we can't find a
usable replacement then we're going to have to put a lot of effort
into fixing what's there. On the whole I think the effort would be
better spent importing someone else's solution.

Oh, so our existing implementation doesn't even meet our needs. OK.

(Your new patch is in the queue.)

I have been thinking about our current snprintf() implementation. As I
remember, we use snprintf mostly for an snprintf that doesn't support
long long, and now those that don't support %$. I am wondering if we
should just process long long args and %$ args, and pass everything else
to the native snprintf.

In fact, one trick would be to substitute long long and %$ in the printf
format string, and then pass that to the native libc printf, with
adjustments for the printf format arguments. That might be simpler than
emulating all of snprintf.

FYI, now that we are using pg_snprintf macros the native snprintf is
available to us.

Anyway, I am sure there are some platforms that don't have vsnprint or
snprintf, but could we just say we don't support them, or emulate one of
we only have the other?

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

Bruce Momjian <pgman@candle.pha.pa.us> writes:

I am wondering if we should just process long long args and %$ args,
and pass everything else to the native snprintf.

AFAICS this is a non-starter --- how will you construct the call to
snprintf? Or even vsnprintf? C doesn't provide the tools you need
to make it happen.

regards, tom lane

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

I am wondering if we should just process long long args and %$ args,
and pass everything else to the native snprintf.

AFAICS this is a non-starter --- how will you construct the call to
snprintf? Or even vsnprintf? C doesn't provide the tools you need
to make it happen.

Couldn't you spin through the varargs and reconstruct a new one?
Is there no way to create a va_arg va_list structure in C?

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

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Is there no way to create a va_arg va_list structure in C?

Exactly. The standard lets you *read out* from such a structure,
but there's no provision for creating one on-the-fly.

regards, tom lane

I have applied a modified version of your patch, attached.

initdb will not work without %*s support, so I had to add that. Please
look over my work. I don't think i handle %*1$ but I am not even sure
what that means or if our translators would ever use such a thing. You
can probably test that better than I can.

Your patch didn't handle signed vs. unsigned va_arg values properly..

There were also maxwidth tests around 's' that I had to remove to
support %*. I think those tests are down in fmtstr too but please
check.

initdb and regression pass.

---------------------------------------------------------------------------

Nicolai Tufar wrote:

Resubmission of yesterday's patch so that it would
cont conflict with Bruce's cvs commit. Pleas apply.

Best regards,
Nicolai.

On Sat, 12 Mar 2005 01:58:15 +0200, Nicolai Tufar <ntufar@gmail.com> wrote:

On Thu, 10 Mar 2005 19:21:41 -0500 (EST), Bruce Momjian
<pgman@candle.pha.pa.us> wrote:

The CVS-tip implementation is fundamentally broken and won't work even
for our internal uses. I've not wasted time complaining about it
because I thought we were going to replace it. If we can't find a
usable replacement then we're going to have to put a lot of effort
into fixing what's there. On the whole I think the effort would be
better spent importing someone else's solution.

Oh, so our existing implementation doesn't even meet our needs. OK.

Which made me wander why did I not aggree with
Tom Lane's suggestion to make do three passes
instead of two. Tom was right, as usual. It happened to
be much easier than I expected. The patch is attached.
Please apply.

Tom, what do you think? Will it be fine with you?

Best regards,
Nicolai

[ Attachment, skipping... ]

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

On Wed, 16 Mar 2005 01:00:21 -0500 (EST), Bruce Momjian
<pgman@candle.pha.pa.us> wrote:

I have applied a modified version of your patch, attached.

I am so sorry, I sent untested patch again. Thank you very
much for patience in fixing it. The patch looks perfectly
fine and works under Solaris.

Under win32 I am still struggling with build environment.
In many directories link fails with "undefined reference to
`pg_snprintf'" in other it fails with "undefined reference to
`_imp__libintl_sprintf'". In yet another directory it fails with
both, like in src/interfaces/ecpg/pgtypeslib:

dlltool --export-all --output-def pgtypes.def numeric.o datetime.o
common.o dt_common.o timestamp.o interval.o pgstrcasecmp.o
dllwrap -o libpgtypes.dll --dllname libpgtypes.dll --def pgtypes.def
numeric.o datetime.o common.o dt_common.o timestamp.o interval.o
pgstrcasecmp.o -L../../../../src/port -lm
numeric.o(.text+0x19ea):numeric.c: undefined reference to
`_imp__libintl_sprintf'
datetime.o(.text+0x476):datetime.c: undefined reference to `pg_snprintf'
common.o(.text+0x1cd):common.c: undefined reference to `pg_snprintf'
common.o(.text+0x251):common.c: undefined reference to `pg_snprintf'
dt_common.o(.text+0x538):dt_common.c: undefined reference to
`_imp__libintl_sprintf'
dt_common.o(.text+0x553):dt_common.c: undefined reference to
`_imp__libintl_sprintf'
dt_common.o(.text+0x597):dt_common.c: undefined reference to
`_imp__libintl_sprintf'
dt_common.o(.text+0x5d5):dt_common.c: undefined reference to
`_imp__libintl_sprintf'
dt_common.o(.text+0x628):dt_common.c: undefined reference to
`_imp__libintl_sprintf'
dt_common.o(.text+0x7e8):dt_common.c: more undefined references to
`_imp__libintl_sprintf' follow
c:\MinGW\bin\dllwrap.exe: c:\MinGW\bin\gcc exited with status 1
make: *** [libpgtypes.a] Error 1

Could someone with a better grasp of configure and
win32 environment check it? Aparently no one regularily
compiles source code under win32 during development cycle
these days.

Best regards,
Nicolai

Nicolai Tufar wrote:

On Wed, 16 Mar 2005 01:00:21 -0500 (EST), Bruce Momjian
<pgman@candle.pha.pa.us> wrote:

I have applied a modified version of your patch, attached.

Here is a patch that fixes the %*$ case.

FYI, I am going to pgindent snprintf.c to make it consistent so please
us CVS for your next patch.

I will work on your Win32 compile problem next.

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

Nicolai Tufar wrote:

On Wed, 16 Mar 2005 01:00:21 -0500 (EST), Bruce Momjian
<pgman@candle.pha.pa.us> wrote:

I have applied a modified version of your patch, attached.

I am so sorry, I sent untested patch again. Thank you very
much for patience in fixing it. The patch looks perfectly
fine and works under Solaris.

Here is another patch that adds sprintf() support, and support for '+',
'h', and fixes '%*s' support.

Applied.

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

Thanks to Andrew Dunstan, I found the cause of these link errors.
Andrew found this in libintl:

#undef snprintf
#define snprintf libintl_snprintf
extern int snprintf (char *, size_t, const char *, ...);

What is happening is that we do:

#define snprintf pg_snprintf

and then libintl.h (?) does:

#define snprintf libintl_snprintf

so the effect is:

#define pg_snprintf libintl_snprintf

In fact, in this example, the system complains about a missing X3 symbol:

#define X1 X2
#define X2 X3

int
main(int argc, char *argv[])
{
X1;
}

so the effet of the defines is:

#define X1 X3

Anyway, the reason ecpg is failing is that it is the only client-side
program that doesn't use libintl for internationalization. It is on our
TODO list to do that, but it hasn't been done yet.

However, only Win32 is seeing this failure, and only when configure
--enable-nls. I think this is because only Win32 does the redefine of
snprint and friends.

Comments?

---------------------------------------------------------------------------

Nicolai Tufar wrote:

On Wed, 16 Mar 2005 01:00:21 -0500 (EST), Bruce Momjian
<pgman@candle.pha.pa.us> wrote:

I have applied a modified version of your patch, attached.

I am so sorry, I sent untested patch again. Thank you very
much for patience in fixing it. The patch looks perfectly
fine and works under Solaris.

Under win32 I am still struggling with build environment.
In many directories link fails with "undefined reference to
`pg_snprintf'" in other it fails with "undefined reference to
`_imp__libintl_sprintf'". In yet another directory it fails with
both, like in src/interfaces/ecpg/pgtypeslib:

dlltool --export-all --output-def pgtypes.def numeric.o datetime.o
common.o dt_common.o timestamp.o interval.o pgstrcasecmp.o
dllwrap -o libpgtypes.dll --dllname libpgtypes.dll --def pgtypes.def
numeric.o datetime.o common.o dt_common.o timestamp.o interval.o
pgstrcasecmp.o -L../../../../src/port -lm
numeric.o(.text+0x19ea):numeric.c: undefined reference to
`_imp__libintl_sprintf'
datetime.o(.text+0x476):datetime.c: undefined reference to `pg_snprintf'
common.o(.text+0x1cd):common.c: undefined reference to `pg_snprintf'
common.o(.text+0x251):common.c: undefined reference to `pg_snprintf'
dt_common.o(.text+0x538):dt_common.c: undefined reference to
`_imp__libintl_sprintf'
dt_common.o(.text+0x553):dt_common.c: undefined reference to
`_imp__libintl_sprintf'
dt_common.o(.text+0x597):dt_common.c: undefined reference to
`_imp__libintl_sprintf'
dt_common.o(.text+0x5d5):dt_common.c: undefined reference to
`_imp__libintl_sprintf'
dt_common.o(.text+0x628):dt_common.c: undefined reference to
`_imp__libintl_sprintf'
dt_common.o(.text+0x7e8):dt_common.c: more undefined references to
`_imp__libintl_sprintf' follow
c:\MinGW\bin\dllwrap.exe: c:\MinGW\bin\gcc exited with status 1
make: *** [libpgtypes.a] Error 1

Could someone with a better grasp of configure and
win32 environment check it? Aparently no one regularily
compiles source code under win32 during development cycle
these days.

Best regards,
Nicolai

---------------------------(end of broadcast)---------------------------
TIP 4: Don't 'kill -9' the postmaster

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

Bruce Momjian <pgman@candle.pha.pa.us> writes:

so the effect is:

#define pg_snprintf libintl_snprintf

That's not how CPP works.

In fact, in this example, the system complains about a missing X3 symbol:

#define X1 X2
#define X2 X3

In this case any occurrence of X1 replaced by X2 but then the result is
rescanned for macros and X2 is turned into X3.

--
greg

After some further digging, I think we have 3 problems.

1. On Windows gettext wants to hijack printf and friends, as below. This
strikes me as rather unfriendly behaviour by a library header file.
Anyway, mercifully libintl.h is included in our source in exactly one
spot, so I think the thing to do for this problem is a) undo that
hijacking and b) make sure any hijacking we want to do occurs after the
point where that file in included (in c.h). This causes most of the
noise, but is probably harmless, since our hijacking does in fact win
out. We need to fix the arnings, though.

2. We have multiple #defines for snprintf and vsnprintf (in port.h and
win32.h).

3. ecpg wants to use our pg*printf routines (because USE_SNPRINTF is
defined) but doesn't know where to find them.

what a mess :-(

cheers

andrew

Bruce Momjian wrote:

Andrew Dunstan wrote:

After some further digging, I think we have 3 problems.

1. On Windows gettext wants to hijack printf and friends, as below. This
strikes me as rather unfriendly behaviour by a library header file.
Anyway, mercifully libintl.h is included in our source in exactly one
spot, so I think the thing to do for this problem is a) undo that
hijacking and b) make sure any hijacking we want to do occurs after the
point where that file in included (in c.h). This causes most of the
noise, but is probably harmless, since our hijacking does in fact win
out. We need to fix the arnings, though.

2. We have multiple #defines for snprintf and vsnprintf (in port.h and
win32.h).

3. ecpg wants to use our pg*printf routines (because USE_SNPRINTF is
defined) but doesn't know where to find them.

what a mess :-(

Based on the "mess" analysis, I decided it is better to allow libintl to
use its own snprintf() for Win32 when NLS is enabled, rather than try to
override that with our own snprintf. I have added code to configure.in
so that we don't check for arg control in native snprintf on Win32
because when we are using NLS, we are going to get their snprintf anyway
and not the native one.

Patch attached and applied.

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