Re: PG_DIAG_SEVERITY and a possible bug in pq_parse_errornotice() - Mailing list pgsql-hackers

I wrote:
> After sleeping on it, I think the right answer is to introduce the new
> error-message field (and not worry about 9.5).  Will work on a patch
> for that, unless I hear objections pretty soon.

So far as I can find, the attached is all we need to do to introduce a
new message field.  (This patch doesn't address the memory-context
questions, but it does fix the localization-driven failure demonstrated
upthread.)

Any objections?  Anyone want to bikeshed the field name?  I considered
PG_DIAG_SEVERITY_NONLOCALIZED and PG_DIAG_SEVERITY_ENGLISH before settling
on PG_DIAG_SEVERITY_ASCII, but I can't say I'm in love with that.

            regards, tom lane

diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index f22e3da..40ae0ff 100644
*** a/doc/src/sgml/libpq.sgml
--- b/doc/src/sgml/libpq.sgml
*************** char *PQresultErrorField(const PGresult
*** 2767,2772 ****
--- 2767,2788 ----
            </listitem>
           </varlistentry>

+          <varlistentry id="libpq-pg-diag-severity-ascii">
+           <term><symbol>PG_DIAG_SEVERITY_ASCII</></term>
+           <listitem>
+            <para>
+             The severity; the field contents are <literal>ERROR</>,
+             <literal>FATAL</>, or <literal>PANIC</> (in an error message),
+             or <literal>WARNING</>, <literal>NOTICE</>, <literal>DEBUG</>,
+             <literal>INFO</>, or <literal>LOG</> (in a notice message).
+             This is identical to the <symbol>PG_DIAG_SEVERITY</> field except
+             that the contents are never localized.  This is present only in
+             reports generated by <productname>PostgreSQL</> versions 9.6
+             and later.
+            </para>
+           </listitem>
+          </varlistentry>
+
           <varlistentry id="libpq-pg-diag-sqlstate">
            <term>
             <symbol>PG_DIAG_SQLSTATE</>
diff --git a/doc/src/sgml/protocol.sgml b/doc/src/sgml/protocol.sgml
index 9c96d8f..9bddb19 100644
*** a/doc/src/sgml/protocol.sgml
--- b/doc/src/sgml/protocol.sgml
*************** message.
*** 4884,4889 ****
--- 4884,4908 ----

  <varlistentry>
  <term>
+ <literal>A</>
+ </term>
+ <listitem>
+ <para>
+         Severity: the field contents are
+         <literal>ERROR</>, <literal>FATAL</>, or
+         <literal>PANIC</> (in an error message), or
+         <literal>WARNING</>, <literal>NOTICE</>, <literal>DEBUG</>,
+         <literal>INFO</>, or <literal>LOG</> (in a notice message).
+         This is identical to the <literal>S</> field except
+         that the contents are never localized.  This is present only in
+         messages generated by <productname>PostgreSQL</> versions 9.6
+         and later.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
+ <term>
  <literal>C</>
  </term>
  <listitem>
diff --git a/src/backend/libpq/pqmq.c b/src/backend/libpq/pqmq.c
index 921242f..b04a7ef 100644
*** a/src/backend/libpq/pqmq.c
--- b/src/backend/libpq/pqmq.c
*************** pq_parse_errornotice(StringInfo msg, Err
*** 237,246 ****
          switch (code)
          {
              case PG_DIAG_SEVERITY:
                  if (strcmp(value, "DEBUG") == 0)
!                     edata->elevel = DEBUG1;        /* or some other DEBUG level */
                  else if (strcmp(value, "LOG") == 0)
!                     edata->elevel = LOG;        /* can't be COMMERROR */
                  else if (strcmp(value, "INFO") == 0)
                      edata->elevel = INFO;
                  else if (strcmp(value, "NOTICE") == 0)
--- 237,262 ----
          switch (code)
          {
              case PG_DIAG_SEVERITY:
+                 /* ignore, trusting we'll get a nonlocalized version */
+                 break;
+             case PG_DIAG_SEVERITY_ASCII:
                  if (strcmp(value, "DEBUG") == 0)
!                 {
!                     /*
!                      * We can't reconstruct the exact DEBUG level, but
!                      * presumably it was >= client_min_messages, so select
!                      * DEBUG1 to ensure we'll pass it on to the client.
!                      */
!                     edata->elevel = DEBUG1;
!                 }
                  else if (strcmp(value, "LOG") == 0)
!                 {
!                     /*
!                      * It can't be LOG_SERVER_ONLY, or the worker wouldn't
!                      * have sent it to us; so LOG is the correct value.
!                      */
!                     edata->elevel = LOG;
!                 }
                  else if (strcmp(value, "INFO") == 0)
                      edata->elevel = INFO;
                  else if (strcmp(value, "NOTICE") == 0)
*************** pq_parse_errornotice(StringInfo msg, Err
*** 254,264 ****
                  else if (strcmp(value, "PANIC") == 0)
                      edata->elevel = PANIC;
                  else
!                     elog(ERROR, "unknown error severity");
                  break;
              case PG_DIAG_SQLSTATE:
                  if (strlen(value) != 5)
!                     elog(ERROR, "malformed sql state");
                  edata->sqlerrcode = MAKE_SQLSTATE(value[0], value[1], value[2],
                                                    value[3], value[4]);
                  break;
--- 270,280 ----
                  else if (strcmp(value, "PANIC") == 0)
                      edata->elevel = PANIC;
                  else
!                     elog(ERROR, "unrecognized error severity: \"%s\"", value);
                  break;
              case PG_DIAG_SQLSTATE:
                  if (strlen(value) != 5)
!                     elog(ERROR, "invalid SQLSTATE: \"%s\"", value);
                  edata->sqlerrcode = MAKE_SQLSTATE(value[0], value[1], value[2],
                                                    value[3], value[4]);
                  break;
*************** pq_parse_errornotice(StringInfo msg, Err
*** 308,314 ****
                  edata->funcname = pstrdup(value);
                  break;
              default:
!                 elog(ERROR, "unknown error field: %d", (int) code);
                  break;
          }
      }
--- 324,330 ----
                  edata->funcname = pstrdup(value);
                  break;
              default:
!                 elog(ERROR, "unrecognized error field code: %d", (int) code);
                  break;
          }
      }
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index 78d441d..ab71b50 100644
*** a/src/backend/utils/error/elog.c
--- b/src/backend/utils/error/elog.c
*************** write_csvlog(ErrorData *edata)
*** 2744,2750 ****
      appendStringInfoChar(&buf, ',');

      /* Error severity */
!     appendStringInfoString(&buf, error_severity(edata->elevel));
      appendStringInfoChar(&buf, ',');

      /* SQL state code */
--- 2744,2750 ----
      appendStringInfoChar(&buf, ',');

      /* Error severity */
!     appendStringInfoString(&buf, gettext(error_severity(edata->elevel)));
      appendStringInfoChar(&buf, ',');

      /* SQL state code */
*************** send_message_to_server_log(ErrorData *ed
*** 2861,2867 ****
      formatted_log_time[0] = '\0';

      log_line_prefix(&buf, edata);
!     appendStringInfo(&buf, "%s:  ", error_severity(edata->elevel));

      if (Log_error_verbosity >= PGERROR_VERBOSE)
          appendStringInfo(&buf, "%s: ", unpack_sql_state(edata->sqlerrcode));
--- 2861,2867 ----
      formatted_log_time[0] = '\0';

      log_line_prefix(&buf, edata);
!     appendStringInfo(&buf, "%s:  ", gettext(error_severity(edata->elevel)));

      if (Log_error_verbosity >= PGERROR_VERBOSE)
          appendStringInfo(&buf, "%s: ", unpack_sql_state(edata->sqlerrcode));
*************** send_message_to_frontend(ErrorData *edat
*** 3144,3155 ****
      if (PG_PROTOCOL_MAJOR(FrontendProtocol) >= 3)
      {
          /* New style with separate fields */
          char        tbuf[12];
          int            ssval;
          int            i;

          pq_sendbyte(&msgbuf, PG_DIAG_SEVERITY);
!         err_sendstring(&msgbuf, error_severity(edata->elevel));

          /* unpack MAKE_SQLSTATE code */
          ssval = edata->sqlerrcode;
--- 3144,3159 ----
      if (PG_PROTOCOL_MAJOR(FrontendProtocol) >= 3)
      {
          /* New style with separate fields */
+         const char *sev;
          char        tbuf[12];
          int            ssval;
          int            i;

+         sev = error_severity(edata->elevel);
          pq_sendbyte(&msgbuf, PG_DIAG_SEVERITY);
!         err_sendstring(&msgbuf, gettext(sev));
!         pq_sendbyte(&msgbuf, PG_DIAG_SEVERITY_ASCII);
!         err_sendstring(&msgbuf, sev);

          /* unpack MAKE_SQLSTATE code */
          ssval = edata->sqlerrcode;
*************** send_message_to_frontend(ErrorData *edat
*** 3268,3274 ****

          initStringInfo(&buf);

!         appendStringInfo(&buf, "%s:  ", error_severity(edata->elevel));

          if (edata->show_funcname && edata->funcname)
              appendStringInfo(&buf, "%s: ", edata->funcname);
--- 3272,3278 ----

          initStringInfo(&buf);

!         appendStringInfo(&buf, "%s:  ", gettext(error_severity(edata->elevel)));

          if (edata->show_funcname && edata->funcname)
              appendStringInfo(&buf, "%s: ", edata->funcname);
*************** get_errno_symbol(int errnum)
*** 3578,3584 ****


  /*
!  * error_severity --- get localized string representing elevel
   */
  static const char *
  error_severity(int elevel)
--- 3582,3591 ----


  /*
!  * error_severity --- get string representing elevel
!  *
!  * The string is not localized here, but we mark the strings for translation
!  * so that callers can invoke gettext on the result.
   */
  static const char *
  error_severity(int elevel)
*************** error_severity(int elevel)
*** 3592,3620 ****
          case DEBUG3:
          case DEBUG4:
          case DEBUG5:
!             prefix = _("DEBUG");
              break;
          case LOG:
          case LOG_SERVER_ONLY:
!             prefix = _("LOG");
              break;
          case INFO:
!             prefix = _("INFO");
              break;
          case NOTICE:
!             prefix = _("NOTICE");
              break;
          case WARNING:
!             prefix = _("WARNING");
              break;
          case ERROR:
!             prefix = _("ERROR");
              break;
          case FATAL:
!             prefix = _("FATAL");
              break;
          case PANIC:
!             prefix = _("PANIC");
              break;
          default:
              prefix = "???";
--- 3599,3627 ----
          case DEBUG3:
          case DEBUG4:
          case DEBUG5:
!             prefix = gettext_noop("DEBUG");
              break;
          case LOG:
          case LOG_SERVER_ONLY:
!             prefix = gettext_noop("LOG");
              break;
          case INFO:
!             prefix = gettext_noop("INFO");
              break;
          case NOTICE:
!             prefix = gettext_noop("NOTICE");
              break;
          case WARNING:
!             prefix = gettext_noop("WARNING");
              break;
          case ERROR:
!             prefix = gettext_noop("ERROR");
              break;
          case FATAL:
!             prefix = gettext_noop("FATAL");
              break;
          case PANIC:
!             prefix = gettext_noop("PANIC");
              break;
          default:
              prefix = "???";
diff --git a/src/include/postgres_ext.h b/src/include/postgres_ext.h
index 74c344c..e318e18 100644
*** a/src/include/postgres_ext.h
--- b/src/include/postgres_ext.h
*************** typedef PG_INT64_TYPE pg_int64;
*** 49,54 ****
--- 49,55 ----
   * applications.
   */
  #define PG_DIAG_SEVERITY        'S'
+ #define PG_DIAG_SEVERITY_ASCII    'A'
  #define PG_DIAG_SQLSTATE        'C'
  #define PG_DIAG_MESSAGE_PRIMARY 'M'
  #define PG_DIAG_MESSAGE_DETAIL    'D'
diff --git a/src/interfaces/libpq/fe-exec.c b/src/interfaces/libpq/fe-exec.c
index d1b91c8..58c8cab 100644
*** a/src/interfaces/libpq/fe-exec.c
--- b/src/interfaces/libpq/fe-exec.c
*************** pqInternalNotice(const PGNoticeHooks *ho
*** 824,829 ****
--- 824,830 ----
       */
      pqSaveMessageField(res, PG_DIAG_MESSAGE_PRIMARY, msgBuf);
      pqSaveMessageField(res, PG_DIAG_SEVERITY, libpq_gettext("NOTICE"));
+     pqSaveMessageField(res, PG_DIAG_SEVERITY_ASCII, "NOTICE");
      /* XXX should provide a SQLSTATE too? */

      /*