Re: [pgsql-hackers-win32] [HACKERS] snprintf causes regression tests - Mailing list pgsql-patches

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