Обсуждение: Re: [pgsql-hackers-win32] [HACKERS] snprintf causes regression tests

Поиск
Список
Период
Сортировка

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

От
Bruce Momjian
Дата:
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

Re: [pgsql-hackers-win32] [HACKERS] snprintf causes regression tests to fail

От
Tom Lane
Дата:
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

От
Nicolai Tufar
Дата:
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

Re: [pgsql-hackers-win32] [HACKERS] snprintf causes regression tests to fail

От
Tom Lane
Дата:
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