Code review for pgstat_get_crashed_backend_activity patch.
authorTom Lane <tgl@sss.pgh.pa.us>
Fri, 21 Oct 2011 20:36:04 +0000 (16:36 -0400)
committerTom Lane <tgl@sss.pgh.pa.us>
Fri, 21 Oct 2011 20:36:04 +0000 (16:36 -0400)
Avoid possibly dumping core when pgstat_track_activity_query_size has a
less-than-default value; avoid uselessly searching for the query string
of a successfully-exited backend; don't bother putting out an ERRDETAIL if
we don't have a query to show; some other minor stylistic improvements.

src/backend/postmaster/pgstat.c
src/backend/postmaster/postmaster.c
src/backend/utils/adt/ascii.c
src/include/pgstat.h
src/include/utils/ascii.h

index ba64f2395c08bbb227eb58899e0d8d67a423667d..24582e304c0f1d0a1724900597c38a244e4a8ae5 100644 (file)
@@ -2760,27 +2760,33 @@ pgstat_get_backend_current_activity(int pid, bool checkUser)
  * pgstat_get_crashed_backend_activity() -
  *
  * Return a string representing the current activity of the backend with
- * the specified PID. Like the function above, but reads shared memory with
- * the expectation that it may be corrupt. Returns either a pointer to a
- * constant string, or writes into the 'buffer' argument and returns it.
+ * the specified PID.  Like the function above, but reads shared memory with
+ * the expectation that it may be corrupt.  On success, copy the string
+ * into the "buffer" argument and return that pointer.  On failure,
+ * return NULL.
  *
- * This function is only intended to be used by postmaster to report the
- * query that crashed the backend. In particular, no attempt is made to
+ * This function is only intended to be used by the postmaster to report the
+ * query that crashed a backend.  In particular, no attempt is made to
  * follow the correct concurrency protocol when accessing the
- * BackendStatusArray. But that's OK, in the worst case we'll return a
- * corrupted message. We also must take care not to trip on ereport(ERROR).
- *
- * Note: return strings for special cases match pg_stat_get_backend_activity.
+ * BackendStatusArray.  But that's OK, in the worst case we'll return a
+ * corrupted message.  We also must take care not to trip on ereport(ERROR).
  * ----------
  */
 const char *
-pgstat_get_crashed_backend_activity(int pid, char *buffer,
-                                   int len)
+pgstat_get_crashed_backend_activity(int pid, char *buffer, int buflen)
 {
    volatile PgBackendStatus *beentry;
    int         i;
 
    beentry = BackendStatusArray;
+
+   /*
+    * We probably shouldn't get here before shared memory has been set up,
+    * but be safe.
+    */
+   if (beentry == NULL || BackendActivityBuffer == NULL)
+       return NULL;
+
    for (i = 1; i <= MaxBackends; i++)
    {
        if (beentry->st_procpid == pid)
@@ -2790,26 +2796,29 @@ pgstat_get_crashed_backend_activity(int pid, char *buffer,
            const char *activity_last;
 
            /*
-            * We can't access activity pointer before we verify that it
-            * falls into BackendActivityBuffer. To make sure that the entire
-            * string including its ending is contained within the buffer,
-            * we subtract one activity length from it.
+            * We mustn't access activity string before we verify that it
+            * falls within the BackendActivityBuffer. To make sure that the
+            * entire string including its ending is contained within the
+            * buffer, subtract one activity length from the buffer size.
             */
            activity_last = BackendActivityBuffer + BackendActivityBufferSize
-                           - pgstat_track_activity_query_size;
+               - pgstat_track_activity_query_size;
 
            if (activity < BackendActivityBuffer ||
                activity > activity_last)
-               return "<command string corrupt>";
+               return NULL;
 
-           if (*(activity) == '\0')
-               return "<command string empty>";
+           /* If no string available, no point in a report */
+           if (activity[0] == '\0')
+               return NULL;
 
            /*
             * Copy only ASCII-safe characters so we don't run into encoding
-            * problems when reporting the message.
+            * problems when reporting the message; and be sure not to run
+            * off the end of memory.
             */
-           ascii_safe_strncpy(buffer, activity, len);
+           ascii_safe_strlcpy(buffer, activity,
+                              Min(buflen, pgstat_track_activity_query_size));
 
            return buffer;
        }
@@ -2818,9 +2827,10 @@ pgstat_get_crashed_backend_activity(int pid, char *buffer,
    }
 
    /* PID not found */
-   return "<backend information not available>";
+   return NULL;
 }
 
+
 /* ------------------------------------------------------------
  * Local support functions follow
  * ------------------------------------------------------------
index 5476975f765484717aa5474da538e14aa0517972..4bb519d46a4c61b9fc5da69e4a30334c6e1c1a4a 100644 (file)
@@ -2777,12 +2777,17 @@ HandleChildCrash(int pid, int exitstatus, const char *procname)
 static void
 LogChildExit(int lev, const char *procname, int pid, int exitstatus)
 {
-   char        activity_buffer[1024]; /* default track_activity_query_size */
-   const char *activity;
+   /*
+    * size of activity_buffer is arbitrary, but set equal to default
+    * track_activity_query_size
+    */
+   char        activity_buffer[1024];
+   const char *activity = NULL;
 
-   activity = pgstat_get_crashed_backend_activity(pid,
-                                                  activity_buffer,
-                                                  sizeof(activity_buffer));
+   if (!EXIT_STATUS_0(exitstatus))
+       activity = pgstat_get_crashed_backend_activity(pid,
+                                                      activity_buffer,
+                                                      sizeof(activity_buffer));
 
    if (WIFEXITED(exitstatus))
        ereport(lev,
@@ -2792,7 +2797,7 @@ LogChildExit(int lev, const char *procname, int pid, int exitstatus)
          "server process" */
                (errmsg("%s (PID %d) exited with exit code %d",
                        procname, pid, WEXITSTATUS(exitstatus)),
-                errdetail("Running query: %s", activity)));
+                activity ? errdetail("Failed process was running: %s", activity) : 0));
    else if (WIFSIGNALED(exitstatus))
 #if defined(WIN32)
        ereport(lev,
@@ -2803,7 +2808,7 @@ LogChildExit(int lev, const char *procname, int pid, int exitstatus)
                (errmsg("%s (PID %d) was terminated by exception 0x%X",
                        procname, pid, WTERMSIG(exitstatus)),
                 errhint("See C include file \"ntstatus.h\" for a description of the hexadecimal value."),
-                errdetail("Running query: %s", activity)));
+                activity ? errdetail("Failed process was running: %s", activity) : 0));
 #elif defined(HAVE_DECL_SYS_SIGLIST) && HAVE_DECL_SYS_SIGLIST
    ereport(lev,
 
@@ -2814,7 +2819,7 @@ LogChildExit(int lev, const char *procname, int pid, int exitstatus)
                    procname, pid, WTERMSIG(exitstatus),
                    WTERMSIG(exitstatus) < NSIG ?
                    sys_siglist[WTERMSIG(exitstatus)] : "(unknown)"),
-            errdetail("Running query: %s", activity)));
+            activity ? errdetail("Failed process was running: %s", activity) : 0));
 #else
        ereport(lev,
 
@@ -2823,7 +2828,7 @@ LogChildExit(int lev, const char *procname, int pid, int exitstatus)
          "server process" */
                (errmsg("%s (PID %d) was terminated by signal %d",
                        procname, pid, WTERMSIG(exitstatus)),
-                errdetail("Running query: %s", activity)));
+                activity ? errdetail("Failed process was running: %s", activity) : 0));
 #endif
    else
        ereport(lev,
@@ -2833,7 +2838,7 @@ LogChildExit(int lev, const char *procname, int pid, int exitstatus)
          "server process" */
                (errmsg("%s (PID %d) exited with unrecognized status %d",
                        procname, pid, exitstatus),
-                errdetail("Running query: %s", activity)));
+                activity ? errdetail("Failed process was running: %s", activity) : 0));
 }
 
 /*
index 7c9e96a04f60cd9819957762d9b326082730e254..95719a6f911ccc3c8aa63806ca985cd862196bf6 100644 (file)
@@ -160,35 +160,38 @@ to_ascii_default(PG_FUNCTION_ARGS)
 }
 
 /* ----------
- * "Escape" a string in unknown encoding to a valid ASCII string.
- * Replace non-ASCII bytes with '?'
- * This must not trigger ereport(ERROR), as it is called from postmaster.
+ * Copy a string in an arbitrary backend-safe encoding, converting it to a
+ * valid ASCII string by replacing non-ASCII bytes with '?'.  Otherwise the
+ * behavior is identical to strlcpy(), except that we don't bother with a
+ * return value.
  *
- * Unlike C strncpy(), the result is always terminated with exactly one null
- * byte.
+ * This must not trigger ereport(ERROR), as it is called in postmaster.
  * ----------
  */
 void
-ascii_safe_strncpy(char *dest, const char *src, int len)
+ascii_safe_strlcpy(char *dest, const char *src, size_t destsiz)
 {
-   int         i;
+   if (destsiz == 0)           /* corner case: no room for trailing nul */
+       return;
 
-   for (i = 0; i < (len - 1); i++)
+   while (--destsiz > 0)
    {
-       unsigned char ch = src[i];  /* use unsigned char here to avoid compiler warning */
+       /* use unsigned char here to avoid compiler warning */
+       unsigned char ch = *src++;
 
        if (ch == '\0')
            break;
        /* Keep printable ASCII characters */
        if (32 <= ch && ch <= 127)
-           dest[i] = ch;
+           *dest = ch;
        /* White-space is also OK */
        else if (ch == '\n' || ch == '\r' || ch == '\t')
-           dest[i] = ch;
+           *dest = ch;
        /* Everything else is replaced with '?' */
        else
-           dest[i] = '?';
+           *dest = '?';
+       dest++;
    }
 
-   dest[i] = '\0';
+   *dest = '\0';
 }
index d0f1927efb21218ab60e64667f2e4cfca43bcac3..7b2bd4edc0e1d01c27442ca09b9d9031584af84f 100644 (file)
@@ -721,7 +721,7 @@ extern void pgstat_report_xact_timestamp(TimestampTz tstamp);
 extern void pgstat_report_waiting(bool waiting);
 extern const char *pgstat_get_backend_current_activity(int pid, bool checkUser);
 extern const char *pgstat_get_crashed_backend_activity(int pid, char *buffer,
-                                   int len);
+                                   int buflen);
 
 extern PgStat_TableStatus *find_tabstat_entry(Oid rel_id);
 extern PgStat_BackendFunctionEntry *find_funcstat_entry(Oid func_id);
index adeddb5626fa9469b9ce091426e18e9003d04118..b57713b3eb1a44e2b2dd8cc750070b09abc2df5b 100644 (file)
@@ -16,6 +16,7 @@
 extern Datum to_ascii_encname(PG_FUNCTION_ARGS);
 extern Datum to_ascii_enc(PG_FUNCTION_ARGS);
 extern Datum to_ascii_default(PG_FUNCTION_ARGS);
-extern void ascii_safe_strncpy(char *dest, const char *src, int len);
+
+extern void ascii_safe_strlcpy(char *dest, const char *src, size_t destsiz);
 
 #endif   /* _ASCII_H_ */