Re: BUG #13636: psql numericlocale adds comma where it ought not - Mailing list pgsql-bugs

Thomas Munro <thomas.munro@enterprisedb.com> writes:
> On Fri, Sep 25, 2015 at 11:37 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Indeed.  It looks like the author of format_numeric_locale() never
>> heard of e-format output.  There's some other pretty crummy code in
>> there, but that's the core problem ...

> Does this look reasonable?

I thought it needed a rather more thoroughgoing revision: there is no need
for it to assume so much about what is in the column, and good reason for
it not to.  (For instance, I note that psql will try to apply this code to
"money" columns, which may be a bad idea, but there can definitely be
stuff in there that doesn't look like a regular number.)  It should muck
with digits immediately following the sign, and nothing else.  There was
some other useless inefficiency too.  I came up with the attached.

I would have borrowed your regression test additions, except I'm afraid
they will fail if the prevailing locale isn't C.

            regards, tom lane

diff --git a/src/bin/psql/print.c b/src/bin/psql/print.c
index cab9e6e..f7343d1 100644
*** a/src/bin/psql/print.c
--- b/src/bin/psql/print.c
***************
*** 39,46 ****
   */
  volatile bool cancel_pressed = false;

  static char *decimal_point;
! static char *grouping;
  static char *thousands_sep;

  static char default_footer[100];
--- 39,47 ----
   */
  volatile bool cancel_pressed = false;

+ /* info for locale-aware numeric formatting; set up by setDecimalLocale() */
  static char *decimal_point;
! static int    groupdigits;
  static char *thousands_sep;

  static char default_footer[100];
*************** static void IsPagerNeeded(const printTab
*** 196,239 ****
  static void print_aligned_vertical(const printTableContent *cont, FILE *fout);


  static int
  integer_digits(const char *my_str)
  {
!     int            frac_len;
!
!     if (my_str[0] == '-')
          my_str++;
!
!     frac_len = strchr(my_str, '.') ? strlen(strchr(my_str, '.')) : 0;
!
!     return strlen(my_str) - frac_len;
  }

! /* Return additional length required for locale-aware numeric output */
  static int
  additional_numeric_locale_len(const char *my_str)
  {
      int            int_len = integer_digits(my_str),
                  len = 0;
-     int            groupdigits = atoi(grouping);

!     if (int_len > 0)
!         /* Don't count a leading separator */
!         len = (int_len / groupdigits - (int_len % groupdigits == 0)) *
!             strlen(thousands_sep);

      if (strchr(my_str, '.') != NULL)
!         len += strlen(decimal_point) - strlen(".");

      return len;
  }

- static int
- strlen_with_numeric_locale(const char *my_str)
- {
-     return strlen(my_str) + additional_numeric_locale_len(my_str);
- }
-
  /*
   * Returns the appropriately formatted string in a new allocated block,
   * caller must free
--- 197,231 ----
  static void print_aligned_vertical(const printTableContent *cont, FILE *fout);


+ /* Count number of digits in integral part of number */
  static int
  integer_digits(const char *my_str)
  {
!     /* ignoring any sign ... */
!     if (my_str[0] == '-' || my_str[0] == '+')
          my_str++;
!     /* ... count initial integral digits */
!     return strspn(my_str, "0123456789");
  }

! /* Compute additional length required for locale-aware numeric output */
  static int
  additional_numeric_locale_len(const char *my_str)
  {
      int            int_len = integer_digits(my_str),
                  len = 0;

!     /* Account for added thousands_sep instances */
!     if (int_len > groupdigits)
!         len += ((int_len - 1) / groupdigits) * strlen(thousands_sep);

+     /* Account for possible additional length of decimal_point */
      if (strchr(my_str, '.') != NULL)
!         len += strlen(decimal_point) - 1;

      return len;
  }

  /*
   * Returns the appropriately formatted string in a new allocated block,
   * caller must free
*************** strlen_with_numeric_locale(const char *m
*** 241,293 ****
  static char *
  format_numeric_locale(const char *my_str)
  {
      int            i,
-                 j,
-                 int_len = integer_digits(my_str),
                  leading_digits;
!     int            groupdigits = atoi(grouping);
!     int            new_str_start = 0;
!     char       *new_str = pg_malloc(strlen_with_numeric_locale(my_str) + 1);

!     leading_digits = (int_len % groupdigits != 0) ?
!         int_len % groupdigits : groupdigits;

!     if (my_str[0] == '-')        /* skip over sign, affects grouping
!                                  * calculations */
      {
!         new_str[0] = my_str[0];
          my_str++;
-         new_str_start = 1;
      }

!     for (i = 0, j = new_str_start;; i++, j++)
      {
!         /* Hit decimal point? */
!         if (my_str[i] == '.')
          {
!             strcpy(&new_str[j], decimal_point);
!             j += strlen(decimal_point);
!             /* add fractional part */
!             strcpy(&new_str[j], &my_str[i] + 1);
!             break;
          }

!         /* End of string? */
!         if (my_str[i] == '\0')
!         {
!             new_str[j] = '\0';
!             break;
!         }

!         /* Add separator? */
!         if (i != 0 && (i - leading_digits) % groupdigits == 0)
!         {
!             strcpy(&new_str[j], thousands_sep);
!             j += strlen(thousands_sep);
!         }

!         new_str[j] = my_str[i];
!     }

      return new_str;
  }
--- 233,283 ----
  static char *
  format_numeric_locale(const char *my_str)
  {
+     int            new_len = strlen(my_str) + additional_numeric_locale_len(my_str);
+     char       *new_str = pg_malloc(new_len + 1);
+     int            int_len = integer_digits(my_str);
      int            i,
                  leading_digits;
!     int            new_str_pos = 0;

!     /* number of digits in first thousands group */
!     leading_digits = int_len % groupdigits;
!     if (leading_digits == 0)
!         leading_digits = groupdigits;

!     /* process sign */
!     if (my_str[0] == '-' || my_str[0] == '+')
      {
!         new_str[new_str_pos++] = my_str[0];
          my_str++;
      }

!     /* process integer part of number */
!     for (i = 0; i < int_len; i++)
      {
!         /* Time to insert separator? */
!         if (i > 0 && --leading_digits == 0)
          {
!             strcpy(&new_str[new_str_pos], thousands_sep);
!             new_str_pos += strlen(thousands_sep);
!             leading_digits = groupdigits;
          }
+         new_str[new_str_pos++] = my_str[i];
+     }

!     /* handle decimal point if any */
!     if (my_str[i] == '.')
!     {
!         strcpy(&new_str[new_str_pos], decimal_point);
!         new_str_pos += strlen(decimal_point);
!         i++;
!     }

!     /* copy the rest (fractional digits and/or exponent, and \0 terminator) */
!     strcpy(&new_str[new_str_pos], &my_str[i]);

!     /* assert we didn't underestimate new_len (an overestimate is OK) */
!     assert(strlen(new_str) <= new_len);

      return new_str;
  }
*************** setDecimalLocale(void)
*** 3241,3250 ****
          decimal_point = pg_strdup(extlconv->decimal_point);
      else
          decimal_point = ".";    /* SQL output standard */
      if (*extlconv->grouping && atoi(extlconv->grouping) > 0)
!         grouping = pg_strdup(extlconv->grouping);
      else
!         grouping = "3";            /* most common */

      /* similar code exists in formatting.c */
      if (*extlconv->thousands_sep)
--- 3231,3241 ----
          decimal_point = pg_strdup(extlconv->decimal_point);
      else
          decimal_point = ".";    /* SQL output standard */
+
      if (*extlconv->grouping && atoi(extlconv->grouping) > 0)
!         groupdigits = atoi(extlconv->grouping);
      else
!         groupdigits = 3;        /* most common */

      /* similar code exists in formatting.c */
      if (*extlconv->thousands_sep)