]> granicus.if.org Git - postgresql/commitdiff
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_ */