Re: BUG #5355: locale incorrectly comma-separates "(null)" - Mailing list pgsql-bugs

Heikki Linnakangas wrote:
> Andy Lester wrote:
>> The following bug has been logged online:
>>
>> Bug reference:      5355
>> Logged by:          Andy Lester
>> Email address:      andy@petdance.com
>> PostgreSQL version: 8.3.6
>> Operating system:   Linux
>> Description:        locale incorrectly comma-separates "(null)"
>> Details:
>>
>> In my .psqlrc I have the following (among others)
>>
>> \pset null '(null)'
>> \pset numericlocale on
>>
>> When I select numeric columns that have NULL values, they show as "(nu,ll)".
>
> Hmm, some of the other formats seem to handle numericlocale even worse :-( :
>
> postgres=# \pset numericlocale on
> Showing locale-adjusted numeric output.
> postgres=# \pset format latex
> Output format is latex.
> postgres=# SELECT 1234::numeric, 'foobar' ;
> \begin{tabular}{r | l}
> \textit{numeric} & \textit{?column?} \\
> \hline
> 1,234 & foo,bar \\
> \end{tabular}
>
> \noindent (1 row) \\
>
> (note how 'foobar' got a comma in the middle.)

Ok, here's a patch to fix both of those bugs. It moves the
responsibility of applying numeric locale up from the print_* functions
to the same place where the null string is applied. Less room for bugs
of omission like the latex format bug that way, and it's no longer
applied to the null string. This also makes the print_* code simpler.

I'll port this to back-branches and commit. Thanks for the report!

--
  Heikki Linnakangas
  EnterpriseDB   http://www.enterprisedb.com
? src/bin/psql/.deps
? src/bin/psql/psql
? src/bin/psql/po/cs.mo
? src/bin/psql/po/de.mo
? src/bin/psql/po/es.mo
? src/bin/psql/po/fa.mo
? src/bin/psql/po/fr.mo
? src/bin/psql/po/hu.mo
? src/bin/psql/po/it.mo
? src/bin/psql/po/ko.mo
? src/bin/psql/po/nb.mo
? src/bin/psql/po/pt_BR.mo
? src/bin/psql/po/ro.mo
? src/bin/psql/po/ru.mo
? src/bin/psql/po/sk.mo
? src/bin/psql/po/sl.mo
? src/bin/psql/po/sv.mo
? src/bin/psql/po/tr.mo
? src/bin/psql/po/zh_CN.mo
? src/bin/psql/po/zh_TW.mo
Index: src/bin/psql/print.c
===================================================================
RCS file: /cvsroot/pgsql/src/bin/psql/print.c,v
retrieving revision 1.96
diff -c -r1.96 print.c
*** src/bin/psql/print.c    1 Jan 2008 19:45:56 -0000    1.96
--- src/bin/psql/print.c    1 Mar 2010 19:12:51 -0000
***************
*** 181,187 ****
      const char *opt_fieldsep = opt->fieldSep;
      const char *opt_recordsep = opt->recordSep;
      bool        opt_tuples_only = opt->tuples_only;
-     bool        opt_numeric_locale = opt->numericLocale;
      unsigned int col_count = 0;
      unsigned int i;
      const char *const * ptr;
--- 181,186 ----
***************
*** 231,245 ****
              if (cancel_pressed)
                  break;
          }
!         if (opt_align[i % col_count] == 'r' && opt_numeric_locale)
!         {
!             char       *my_cell = format_numeric_locale(*ptr);
!
!             fputs(my_cell, fout);
!             free(my_cell);
!         }
!         else
!             fputs(*ptr, fout);

          if ((i + 1) % col_count)
              fputs(opt_fieldsep, fout);
--- 230,236 ----
              if (cancel_pressed)
                  break;
          }
!         fputs(*ptr, fout);

          if ((i + 1) % col_count)
              fputs(opt_fieldsep, fout);
***************
*** 278,284 ****
      const char *opt_fieldsep = opt->fieldSep;
      const char *opt_recordsep = opt->recordSep;
      bool        opt_tuples_only = opt->tuples_only;
-     bool        opt_numeric_locale = opt->numericLocale;
      unsigned int col_count = 0;
      unsigned int i;
      const char *const * ptr;
--- 269,274 ----
***************
*** 324,338 ****

          fputs(headers[i % col_count], fout);
          fputs(opt_fieldsep, fout);
!         if (opt_align[i % col_count] == 'r' && opt_numeric_locale)
!         {
!             char       *my_cell = format_numeric_locale(*ptr);
!
!             fputs(my_cell, fout);
!             free(my_cell);
!         }
!         else
!             fputs(*ptr, fout);

          if ((i + 1) % col_count)
              fputs(opt_recordsep, fout);
--- 314,320 ----

          fputs(headers[i % col_count], fout);
          fputs(opt_fieldsep, fout);
!         fputs(*ptr, fout);

          if ((i + 1) % col_count)
              fputs(opt_recordsep, fout);
***************
*** 405,411 ****
                     FILE *fout)
  {
      bool        opt_tuples_only = opt->tuples_only;
-     bool        opt_numeric_locale = opt->numericLocale;
      unsigned short int opt_border = opt->border;
      int            encoding = opt->encoding;
      unsigned int col_count = 0;
--- 387,392 ----
***************
*** 482,491 ****
          int            height,
                      space;

!         if (opt_align[i % col_count] == 'r' && opt_numeric_locale)
!             numeric_locale_len = additional_numeric_locale_len(*ptr);
!         else
!             numeric_locale_len = 0;

          /* Get width, ignore height */
          pg_wcssize((unsigned char *) *ptr, strlen(*ptr), encoding, &tmp, &height, &space);
--- 463,469 ----
          int            height,
                      space;

!         numeric_locale_len = 0;

          /* Get width, ignore height */
          pg_wcssize((unsigned char *) *ptr, strlen(*ptr), encoding, &tmp, &height, &space);
***************
*** 655,679 ****
                      /* content */
                      if (opt_align[j] == 'r')
                      {
!                         if (opt_numeric_locale)
!                         {
!                             /*
!                              * Assumption: This code used only on strings
!                              * without multibyte characters, otherwise
!                              * this_line->width < strlen(this_ptr) and we get
!                              * an overflow
!                              */
!                             char       *my_cell = format_numeric_locale((char *) this_line->ptr);
!
!                             fprintf(fout, "%*s%s",
!                                     (int) (widths[i % col_count] - strlen(my_cell)), "",
!                                     my_cell);
!                             free(my_cell);
!                         }
!                         else
!                             fprintf(fout, "%*s%s",
!                                     widths[j] - this_line->width, "",
!                                     this_line->ptr);
                      }
                      else
                          fprintf(fout, "%-s%*s", this_line->ptr,
--- 633,641 ----
                      /* content */
                      if (opt_align[j] == 'r')
                      {
!                         fprintf(fout, "%*s%s",
!                                 widths[j] - this_line->width, "",
!                                 this_line->ptr);
                      }
                      else
                          fprintf(fout, "%-s%*s", this_line->ptr,
***************
*** 743,749 ****
                         FILE *fout)
  {
      bool        opt_tuples_only = opt->tuples_only;
-     bool        opt_numeric_locale = opt->numericLocale;
      unsigned short int opt_border = opt->border;
      int            encoding = opt->encoding;
      unsigned int col_count = 0;
--- 705,710 ----
***************
*** 804,813 ****
          int            height,
                      fs;

!         if (opt_align[i % col_count] == 'r' && opt_numeric_locale)
!             numeric_locale_len = additional_numeric_locale_len(*ptr);
!         else
!             numeric_locale_len = 0;

          pg_wcssize((unsigned char *) *ptr, strlen(*ptr), encoding, &tmp, &height, &fs);
          tmp += numeric_locale_len;
--- 765,771 ----
          int            height,
                      fs;

!         numeric_locale_len = 0;

          pg_wcssize((unsigned char *) *ptr, strlen(*ptr), encoding, &tmp, &height, &fs);
          tmp += numeric_locale_len;
***************
*** 925,949 ****

              if (!dcomplete)
              {
!                 if (opt_align[i % col_count] == 'r' && opt_numeric_locale)
!                 {
!                     char       *my_cell = format_numeric_locale((char *) dlineptr[line_count].ptr);
!
!                     if (opt_border < 2)
!                         fprintf(fout, "%s\n", my_cell);
!                     else
!                         fprintf(fout, "%-s%*s |\n", my_cell,
!                                 (int) (dwidth - strlen(my_cell)), "");
!                     free(my_cell);
!                 }
                  else
!                 {
!                     if (opt_border < 2)
!                         fprintf(fout, "%s\n", dlineptr[line_count].ptr);
!                     else
!                         fprintf(fout, "%-s%*s |\n", dlineptr[line_count].ptr,
!                                 dwidth - dlineptr[line_count].width, "");
!                 }

                  if (line_count == dheight - 1 || !dlineptr[line_count + 1].ptr)
                      dcomplete = 1;
--- 883,893 ----

              if (!dcomplete)
              {
!                 if (opt_border < 2)
!                     fprintf(fout, "%s\n", dlineptr[line_count].ptr);
                  else
!                     fprintf(fout, "%-s%*s |\n", dlineptr[line_count].ptr,
!                             dwidth - dlineptr[line_count].width, "");

                  if (line_count == dheight - 1 || !dlineptr[line_count + 1].ptr)
                      dcomplete = 1;
***************
*** 1037,1043 ****
                  FILE *fout)
  {
      bool        opt_tuples_only = opt->tuples_only;
-     bool        opt_numeric_locale = opt->numericLocale;
      unsigned short int opt_border = opt->border;
      const char *opt_table_attr = opt->tableAttr;
      unsigned int col_count = 0;
--- 981,986 ----
***************
*** 1094,1108 ****
          /* is string only whitespace? */
          if ((*ptr)[strspn(*ptr, " \t")] == '\0')
              fputs("  ", fout);
!         else if (opt_align[i % col_count] == 'r' && opt_numeric_locale)
!         {
!             char       *my_cell = format_numeric_locale(*ptr);
!
!             html_escaped_print(my_cell, fout);
!             free(my_cell);
!         }
!         else
!             html_escaped_print(*ptr, fout);

          fputs("</td>\n", fout);

--- 1037,1043 ----
          /* is string only whitespace? */
          if ((*ptr)[strspn(*ptr, " \t")] == '\0')
              fputs("  ", fout);
!         html_escaped_print(*ptr, fout);

          fputs("</td>\n", fout);

***************
*** 1138,1144 ****
                      FILE *fout)
  {
      bool        opt_tuples_only = opt->tuples_only;
-     bool        opt_numeric_locale = opt->numericLocale;
      unsigned short int opt_border = opt->border;
      const char *opt_table_attr = opt->tableAttr;
      unsigned int col_count = 0;
--- 1073,1078 ----
***************
*** 1192,1206 ****
          /* is string only whitespace? */
          if ((*ptr)[strspn(*ptr, " \t")] == '\0')
              fputs("  ", fout);
!         else if (opt_align[i % col_count] == 'r' && opt_numeric_locale)
!         {
!             char       *my_cell = format_numeric_locale(*ptr);
!
!             html_escaped_print(my_cell, fout);
!             free(my_cell);
!         }
!         else
!             html_escaped_print(*ptr, fout);

          fputs("</td>\n  </tr>\n", fout);
      }
--- 1126,1132 ----
          /* is string only whitespace? */
          if ((*ptr)[strspn(*ptr, " \t")] == '\0')
              fputs("  ", fout);
!         html_escaped_print(*ptr, fout);

          fputs("</td>\n  </tr>\n", fout);
      }
***************
*** 1276,1282 ****
                   FILE *fout)
  {
      bool        opt_tuples_only = opt->tuples_only;
-     bool        opt_numeric_locale = opt->numericLocale;
      unsigned short int opt_border = opt->border;
      unsigned int col_count = 0;
      unsigned int i;
--- 1202,1207 ----
***************
*** 1340,1354 ****
      /* print cells */
      for (i = 0, ptr = cells; *ptr; i++, ptr++)
      {
!         if (opt_numeric_locale)
!         {
!             char       *my_cell = format_numeric_locale(*ptr);
!
!             latex_escaped_print(my_cell, fout);
!             free(my_cell);
!         }
!         else
!             latex_escaped_print(*ptr, fout);

          if ((i + 1) % col_count == 0)
          {
--- 1265,1271 ----
      /* print cells */
      for (i = 0, ptr = cells; *ptr; i++, ptr++)
      {
!         latex_escaped_print(*ptr, fout);

          if ((i + 1) % col_count == 0)
          {
***************
*** 1389,1395 ****
                       FILE *fout)
  {
      bool        opt_tuples_only = opt->tuples_only;
-     bool        opt_numeric_locale = opt->numericLocale;
      unsigned short int opt_border = opt->border;
      unsigned int col_count = 0;
      unsigned long record = opt->prior_records + 1;
--- 1306,1311 ----
***************
*** 1469,1483 ****
          {
              for (ptr = footers; *ptr; ptr++)
              {
!                 if (opt_numeric_locale)
!                 {
!                     char       *my_cell = format_numeric_locale(*ptr);
!
!                     latex_escaped_print(my_cell, fout);
!                     free(my_cell);
!                 }
!                 else
!                     latex_escaped_print(*ptr, fout);
                  fputs(" \\\\\n", fout);
              }
          }
--- 1385,1391 ----
          {
              for (ptr = footers; *ptr; ptr++)
              {
!                 latex_escaped_print(*ptr, fout);
                  fputs(" \\\\\n", fout);
              }
          }
***************
*** 1516,1522 ****
                      FILE *fout)
  {
      bool        opt_tuples_only = opt->tuples_only;
-     bool        opt_numeric_locale = opt->numericLocale;
      unsigned short int opt_border = opt->border;
      unsigned int col_count = 0;
      unsigned int i;
--- 1424,1429 ----
***************
*** 1575,1589 ****
      /* print cells */
      for (i = 0, ptr = cells; *ptr; i++, ptr++)
      {
!         if (opt_numeric_locale)
!         {
!             char       *my_cell = format_numeric_locale(*ptr);
!
!             troff_ms_escaped_print(my_cell, fout);
!             free(my_cell);
!         }
!         else
!             troff_ms_escaped_print(*ptr, fout);

          if ((i + 1) % col_count == 0)
          {
--- 1482,1488 ----
      /* print cells */
      for (i = 0, ptr = cells; *ptr; i++, ptr++)
      {
!         troff_ms_escaped_print(*ptr, fout);

          if ((i + 1) % col_count == 0)
          {
***************
*** 1619,1625 ****
                          FILE *fout)
  {
      bool        opt_tuples_only = opt->tuples_only;
-     bool        opt_numeric_locale = opt->numericLocale;
      unsigned short int opt_border = opt->border;
      unsigned int col_count = 0;
      unsigned long record = opt->prior_records + 1;
--- 1518,1523 ----
***************
*** 1704,1718 ****

          troff_ms_escaped_print(headers[i % col_count], fout);
          fputc('\t', fout);
!         if (opt_numeric_locale)
!         {
!             char       *my_cell = format_numeric_locale(*ptr);
!
!             troff_ms_escaped_print(my_cell, fout);
!             free(my_cell);
!         }
!         else
!             troff_ms_escaped_print(*ptr, fout);

          fputc('\n', fout);
      }
--- 1602,1608 ----

          troff_ms_escaped_print(headers[i % col_count], fout);
          fputc('\t', fout);
!         troff_ms_escaped_print(*ptr, fout);

          fputc('\n', fout);
      }
***************
*** 1936,1941 ****
--- 1826,1832 ----
      int            i,
                  r,
                  c;
+     bool       *cellmustfree = NULL;

      if (cancel_pressed)
          return;
***************
*** 1956,1961 ****
--- 1847,1879 ----
  #endif
      }

+     /* set alignment */
+     align = pg_local_calloc(nfields + 1, sizeof(*align));
+
+     for (i = 0; i < nfields; i++)
+     {
+         Oid            ftype = PQftype(result, i);
+
+         switch (ftype)
+         {
+             case INT2OID:
+             case INT4OID:
+             case INT8OID:
+             case FLOAT4OID:
+             case FLOAT8OID:
+             case NUMERICOID:
+             case OIDOID:
+             case XIDOID:
+             case CIDOID:
+             case CASHOID:
+                 align[i] = 'r';
+                 break;
+             default:
+                 align[i] = 'l';
+                 break;
+         }
+     }
+
      /* set cells */
      ncells = ntuples * nfields;
      cells = pg_local_calloc(ncells + 1, sizeof(*cells));
***************
*** 1976,1981 ****
--- 1894,1913 ----
                  if (opt->trans_columns && opt->trans_columns[c])
                      cells[i] = _(cells[i]);
  #endif
+
+                 if (align[c] == 'r' && opt->topt.numericLocale)
+                 {
+                     cells[i] = format_numeric_locale(cells[i]);
+
+                     /*
+                      * format_numeric_locale() returns a malloc'd string,
+                      * remember that it needs to be freed.
+                      */
+                     if (cellmustfree == NULL)
+                         cellmustfree = pg_local_calloc(ncells + 1, sizeof(bool));
+
+                     cellmustfree[i] = true;
+                 }
              }
              i++;
          }
***************
*** 2000,2038 ****
      else
          footers = NULL;

-     /* set alignment */
-     align = pg_local_calloc(nfields + 1, sizeof(*align));
-
-     for (i = 0; i < nfields; i++)
-     {
-         Oid            ftype = PQftype(result, i);
-
-         switch (ftype)
-         {
-             case INT2OID:
-             case INT4OID:
-             case INT8OID:
-             case FLOAT4OID:
-             case FLOAT8OID:
-             case NUMERICOID:
-             case OIDOID:
-             case XIDOID:
-             case CIDOID:
-             case CASHOID:
-                 align[i] = 'r';
-                 break;
-             default:
-                 align[i] = 'l';
-                 break;
-         }
-     }
-
      /* call table printer */
      printTable(opt->title, headers, cells,
                 (const char *const *) footers,
                 align, &opt->topt, fout, flog);

      free(headers);
      free(cells);
      if (footers && !opt->footers)
      {
--- 1932,1952 ----
      else
          footers = NULL;

      /* call table printer */
      printTable(opt->title, headers, cells,
                 (const char *const *) footers,
                 align, &opt->topt, fout, flog);

      free(headers);
+     if (cellmustfree)
+     {
+         for (i = 0; i < ncells; i++)
+         {
+             if (cellmustfree[i])
+                 free((char *) cells[i]);
+         }
+         free(cellmustfree);
+     }
      free(cells);
      if (footers && !opt->footers)
      {