From: Tom Lane Date: Fri, 21 Oct 2011 20:36:04 +0000 (-0400) Subject: Code review for pgstat_get_crashed_backend_activity patch. X-Git-Tag: REL9_2_BETA1~946 X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=f9c92a5a3ead738c7de0dffa203a92b4d2fec413;p=postgresql Code review for pgstat_get_crashed_backend_activity patch. 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. --- diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c index ba64f2395c..24582e304c 100644 --- a/src/backend/postmaster/pgstat.c +++ b/src/backend/postmaster/pgstat.c @@ -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 ""; + return NULL; - if (*(activity) == '\0') - return ""; + /* 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 ""; + return NULL; } + /* ------------------------------------------------------------ * Local support functions follow * ------------------------------------------------------------ diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c index 5476975f76..4bb519d46a 100644 --- a/src/backend/postmaster/postmaster.c +++ b/src/backend/postmaster/postmaster.c @@ -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)); } /* diff --git a/src/backend/utils/adt/ascii.c b/src/backend/utils/adt/ascii.c index 7c9e96a04f..95719a6f91 100644 --- a/src/backend/utils/adt/ascii.c +++ b/src/backend/utils/adt/ascii.c @@ -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'; } diff --git a/src/include/pgstat.h b/src/include/pgstat.h index d0f1927efb..7b2bd4edc0 100644 --- a/src/include/pgstat.h +++ b/src/include/pgstat.h @@ -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); diff --git a/src/include/utils/ascii.h b/src/include/utils/ascii.h index adeddb5626..b57713b3eb 100644 --- a/src/include/utils/ascii.h +++ b/src/include/utils/ascii.h @@ -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_ */