Thread: Re: [PATCHES] COPYable logs

[redirected to -hackers]

Tom Lane wrote:
> Another thing that needs to be looked at carefully is how much memory
> write_csvlog() eats.  I'm a little bit concerned about whether it will
> result in infinite recursion when our backs are against the wall and
> we only have the original 8K in ErrorContext to work in.  (We could
> increase that figure if need be, but we need to know by how much.)
>
>
>   


I think we can make a saving by rearranging the end of 
send_message_to_server_log() so we can call pfree(buf.data) before we 
call write_csvlog(). We can probably make a further saving by changing 
how we put the CSV-escaped error message on the end of the buffer we 
send down the pipe. I will look into those.

If these prove difficult, I'd say 24K would put us in an equivalent 
position (two extra copies of the error message plus change). Even so, 
I'm inclined to say that 8K is very tight. It might be enough for very 
small startup messages, but is the context size likely to stay at 8K for 
non-trivial uses? A single logged complex query or function body would 
easily blow it.

cheers

andrew

Andrew Dunstan <andrew@dunslane.net> writes:
> If these prove difficult, I'd say 24K would put us in an equivalent 
> position (two extra copies of the error message plus change). Even so, 
> I'm inclined to say that 8K is very tight.

We really only care about being able to deliver an "out of memory during
error recovery" message within that space.  So yes, you can assume the
message text isn't huge and there isn't any add-on context info to worry
about.  But there could be localization and/or encoding translation costs.
        regards, tom lane

Tom Lane wrote:
> Andrew Dunstan <andrew@dunslane.net> writes:
>
>> If these prove difficult, I'd say 24K would put us in an equivalent
>> position (two extra copies of the error message plus change). Even so,
>> I'm inclined to say that 8K is very tight.
>>
>
> We really only care about being able to deliver an "out of memory during
> error recovery" message within that space.  So yes, you can assume the
> message text isn't huge and there isn't any add-on context info to worry
> about.  But there could be localization and/or encoding translation costs.
>
>
>

Well, the attached small patch should reduce the memory impact
substantially. Not sure whether you think that's sufficient, or you want
to take it further.

cheers

andrew
Index: src/backend/utils/error/elog.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/utils/error/elog.c,v
retrieving revision 1.194
diff -c -r1.194 elog.c
*** src/backend/utils/error/elog.c    19 Aug 2007 01:41:25 -0000    1.194
--- src/backend/utils/error/elog.c    20 Aug 2007 01:34:38 -0000
***************
*** 133,139 ****
  static void append_with_tabs(StringInfo buf, const char *str);
  static bool is_log_level_output(int elevel, int log_min_level);
  static void write_pipe_chunks(char *data, int len, int dest);
! static void get_error_message(StringInfo buf, ErrorData *edata);

  /*
   * errstart --- begin an error-reporting cycle
--- 133,140 ----
  static void append_with_tabs(StringInfo buf, const char *str);
  static bool is_log_level_output(int elevel, int log_min_level);
  static void write_pipe_chunks(char *data, int len, int dest);
! static void get_csv_error_message(StringInfo buf, ErrorData *edata);
! static void write_csvlog(ErrorData *edata);

  /*
   * errstart --- begin an error-reporting cycle
***************
*** 1809,1817 ****
      appendStringInfoChar(&buf, ',');

      /* Error message and cursor position if any */
!     get_error_message(&msgbuf, edata);
!
!     appendCSVLiteral(&buf, msgbuf.data);

      appendStringInfoChar(&buf, '\n');

--- 1810,1816 ----
      appendStringInfoChar(&buf, ',');

      /* Error message and cursor position if any */
!     get_csv_error_message(&buf, edata);

      appendStringInfoChar(&buf, '\n');

***************
*** 1826,1840 ****
  }

  /*
!  * Appends the buffer with the error message and the cursor position.
   */
  static void
! get_error_message(StringInfo buf, ErrorData *edata)
  {
!     if (edata->message)
!         appendStringInfo(buf, "%s", edata->message);
!     else
!         appendStringInfo(buf, "%s", _("missing error text"));

      if (edata->cursorpos > 0)
          appendStringInfo(buf, _(" at character %d"),
--- 1825,1847 ----
  }

  /*
!  * Appends the buffer with the error message and the cursor position, all
!  * CSV escaped.
   */
  static void
! get_csv_error_message(StringInfo buf, ErrorData *edata)
  {
!     char *msg = edata->message ? edata-> message : _("missing error text");
!     char c;
!
!     appendStringInfoCharMacro(buf, '"');
!
!     while ( (c = *msg++) != '\0' )
!     {
!       if (c == '"')
!           appendStringInfoCharMacro(buf, '"');
!       appendStringInfoCharMacro(buf, c);
!     }

      if (edata->cursorpos > 0)
          appendStringInfo(buf, _(" at character %d"),
***************
*** 1842,1847 ****
--- 1849,1856 ----
      else if (edata->internalpos > 0)
          appendStringInfo(buf, _(" at character %d"),
                           edata->internalpos);
+
+     appendStringInfoCharMacro(buf, '"');
  }

  /*
***************
*** 2032,2044 ****
              write(fileno(stderr), buf.data, buf.len);
      }

      if (Log_destination & LOG_DESTINATION_CSVLOG)
      {
          if (redirection_done || am_syslogger)
          {
              /* send CSV data if it's safe to do so (syslogger doesn't need
!              * the pipe)
               */
              write_csvlog(edata);
          }
          else
--- 2041,2059 ----
              write(fileno(stderr), buf.data, buf.len);
      }

+     /* If in the syslogger process, try to write messages direct to file */
+     if (am_syslogger)
+         write_syslogger_file(buf.data, buf.len, LOG_DESTINATION_STDERR);
+
+     /* Write to CSV log if enabled */
      if (Log_destination & LOG_DESTINATION_CSVLOG)
      {
          if (redirection_done || am_syslogger)
          {
              /* send CSV data if it's safe to do so (syslogger doesn't need
!              * the pipe). First get back the space in the message buffer.
               */
+             pfree(buf.data);
              write_csvlog(edata);
          }
          else
***************
*** 2051,2064 ****
                  /* write message to stderr unless we just sent it above */
                  write(fileno(stderr), buf.data, buf.len);
              }
          }
      }
!
!     /* If in the syslogger process, try to write messages direct to file */
!     if (am_syslogger)
!         write_syslogger_file(buf.data, buf.len, LOG_DESTINATION_STDERR);
!
!     pfree(buf.data);
  }

  /*
--- 2066,2078 ----
                  /* write message to stderr unless we just sent it above */
                  write(fileno(stderr), buf.data, buf.len);
              }
+             pfree(buf.data);
          }
      }
!     else
!     {
!         pfree(buf.data);
!     }
  }

  /*