Thread: Re: [pgsql-hackers-win32] [HACKERS] snprintf causes regression tests
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 Index: src/port/snprintf.c =================================================================== RCS file: /cvsroot/pgsql/src/port/snprintf.c,v retrieving revision 1.11 diff -c -c -r1.11 snprintf.c *** src/port/snprintf.c 2 Mar 2005 03:21:52 -0000 1.11 --- src/port/snprintf.c 2 Mar 2005 05:17:01 -0000 *************** *** 32,49 **** * SUCH DAMAGE. */ ! /* might be in either frontend or backend */ #include "postgres_fe.h" #ifndef WIN32 #include <sys/ioctl.h> #endif #include <sys/param.h> - #ifndef NL_ARGMAX - #define NL_ARGMAX 4096 - #endif - /* ** SNPRINTF, VSNPRINT -- counted versions of printf ** --- 32,48 ---- * SUCH DAMAGE. */ ! #ifndef FRONTEND ! #include "postgres.h" ! #else #include "postgres_fe.h" + #endif #ifndef WIN32 #include <sys/ioctl.h> #endif #include <sys/param.h> /* ** SNPRINTF, VSNPRINT -- counted versions of printf ** *************** *** 157,167 **** int realpos = 0; int position; char *output; ! /* In thread mode this structure is too large. */ ! #ifndef ENABLE_THREAD_SAFETY ! static ! #endif ! struct{ const char* fmtbegin; const char* fmtend; void* value; --- 156,164 ---- int realpos = 0; int position; char *output; ! int percents = 1; ! const char *p; ! struct fmtpar { const char* fmtbegin; const char* fmtend; void* value; *************** *** 179,188 **** int pointflag; char func; int realpos; ! } fmtpar[NL_ARGMAX+1], *fmtparptr[NL_ARGMAX+1]; ! format_save = format; output = buffer; while ((ch = *format++)) { --- 176,205 ---- int pointflag; char func; int realpos; ! } *fmtpar, **fmtparptr; + /* Create enough structures to hold all arguments */ + for (p = format; *p != '\0'; p++) + if (*p == '%') /* counts %% as two, so overcounts */ + percents++; + #ifndef FRONTEND + fmtpar = pgport_palloc(sizeof(struct fmtpar) * percents); + fmtparptr = pgport_palloc(sizeof(struct fmtpar *) * percents); + #else + if ((fmtpar = malloc(sizeof(struct fmtpar) * percents)) == NULL) + { + fprintf(stderr, _("out of memory\n")); + exit(1); + } + if ((fmtparptr = malloc(sizeof(struct fmtpar *) * percents)) == NULL) + { + fprintf(stderr, _("out of memory\n")); + exit(1); + } + #endif + format_save = format; + output = buffer; while ((ch = *format++)) { *************** *** 418,426 **** performpr: /* shuffle pointers */ for(i = 1; i < fmtpos; i++) - { fmtparptr[i] = &fmtpar[fmtpar[i].realpos]; - } output = buffer; format = format_save; while ((ch = *format++)) --- 435,441 ---- *************** *** 465,470 **** --- 480,493 ---- ; /* semicolon required because a goto has to be attached to a statement */ } *output = '\0'; + + #ifndef FRONTEND + pgport_pfree(fmtpar); + pgport_pfree(fmtparptr); + #else + free(fmtpar); + free(fmtparptr); + #endif } static void
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
Re: [pgsql-hackers-win32] [HACKERS] snprintf causes regression tests to fail
From
Nicolai Tufar
Date:
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