8000 Code review for pgstat_get_crashed_backend_activity patch. · fschopp/postgres@f9c92a5 · GitHub
[go: up one dir, main page]
Skip to content
8000

Commit f9c92a5

Browse files
tglsfdctglsfdc
committed
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.
1 parent 5ac5980 commit f9c92a5

File tree

5 files changed

+66
-47
lines changed

5 files changed

+66
-47
lines changed

‎src/backend/postmaster/pgstat.c

Lines changed: 32 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -2760,27 +2760,33 @@ pgstat_get_backend_current_activity(int pid, bool checkUser)
27602760
* pgstat_get_crashed_backend_activity() -
27612761
*
27622762
* Return a string representing the current activity of the backend with
2763-
* the specified PID. Like the function above, but reads shared memory with
2764-
* the expectation that it may be corrupt. Returns either a pointer to a
2765-
* constant string, or writes into the 'buffer' argument and returns it.
2763+
* the specified PID. Like the function above, but reads shared memory with
2764+
* the expectation that it may be corrupt. On success, copy the string
2765+
* into the "buffer" argument and return that pointer. On failure,
2766+
* return NULL.
27662767
*
2767-
* This function is only intended to be used by postmaster to report the
2768-
* query that crashed the backend. In particular, no attempt is made to
2768+
* This function is only intended to be used by the postmaster to report the
2769+
* query that crashed a backend. In particular, no attempt is made to
27692770
* follow the correct concurrency protocol when accessing the
2770-
* BackendStatusArray. But that's OK, in the worst case we'll return a
2771-
* corrupted message. We also must take care not to trip on ereport(ERROR).
2772-
*
2773-
* Note: return strings for special cases match pg_stat_get_backend_activity.
2771+
* BackendStatusArray. But that's OK, in the worst case we'll return a
2772+
* corrupted message. We also must take care not to trip on ereport(ERROR).
27742773
* ----------
27752774
*/
27762775
const char *
2777-
pgstat_get_crashed_backend_activity(int pid, char *buffer,
2778-
int len)
2776+
pgstat_get_crashed_backend_activity(int pid, char *buffer, int buflen)
27792777
{
27802778
volatile PgBackendStatus *beentry;
27812779
int i;
27822780

27832781
beentry = BackendStatusArray;
2782+
2783+
/*
2784+
* We probably shouldn't get here before shared memory has been set up,
2785+
* but be safe.
2786+
*/
2787+
if (beentry == NULL || BackendActivityBuffer == NULL)
2788+
return NULL;
2789+
27842790
for (i = 1; i <= MaxBackends; i++)
27852791
{
27862792
if (beentry->st_procpid == pid)
@@ -2790,26 +2796,29 @@ pgstat_get_crashed_backend_activity(int pid, char *buffer,
27902796
const char *activity_last;
27912797

27922798
/*
2793-
* We can't access activity pointer before we verify that it
2794-
* falls into BackendActivityBuffer. To make sure that the entire
2795-
* string including its ending is contained within the buffer,
2796-
* we subtract one activity length from it.
2799+
* We mustn't access activity string before we verify that it
2800+
* falls within the BackendActivityBuffer. To make sure that the
2801+
* entire string including its ending is contained within the
2802+
* buffer, subtract one activity length from the buffer size.
27972803
*/
27982804
activity_last = BackendActivityBuffer + BackendActivityBufferSize
2799-
- pgstat_track_activity_query_size;
2805+
- pgstat_track_activity_query_size;
28002806

28012807
if (activity < BackendActivityBuffer ||
28022808
activity > activity_last)
2803-
return "<command string corrupt>";
2809+
return NULL;
28042810

2805-
if (*(activity) == '\0')
2806-
return "<command string empty>";
2811+
/* If no string available, no point in a report */
2812+
if (activity[0] == '\0')
2813+
return NULL;
28072814

28082815
/*
28092816
* Copy only ASCII-safe characters so we don't run into encoding
2810-
* problems when reporting the message.
2817+
* problems when reporting the message; and be sure not to run
2818+
* off the end of memory.
28112819
*/
2812-
ascii_safe_strncpy(buffer, activity, len);
2820+
ascii_safe_strlcpy(buffer, activity,
2821+
Min(buflen, pgstat_track_activity_query_size));
28132822

28142823
return buffer;
28152824
}
@@ -2818,9 +2827,10 @@ pgstat_get_crashed_backend_activity(int pid, char *buffer,
28182827
}
28192828

28202829
/* PID not found */
2821-
return "<backend information not available>";
2830+
return NULL;
28222831
}
28232832

2833+
28242834
/* ------------------------------------------------------------
28252835
* Local support functions follow
28262836
* ------------------------------------------------------------

‎src/backend/postmaster/postmaster.c

Lines changed: 15 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -2777,12 +2777,17 @@ HandleChildCrash(int pid, int exitstatus, const char *procname)
27772777
static void
27782778
LogChildExit(int lev, const char *procname, int pid, int exitstatus)
27792779
{
2780-
char activity_buffer[1024]; /* default track_activity_query_size */
2781-
const char *activity;
2780+
/*
2781+
* size of activity_buffer is arbitrary, but set equal to default
2782+
* track_activity_query_size
2783+
*/
2784+
char activity_buffer[1024];
2785+
const char *activity = NULL;
27822786

2783-
activity = pgstat_get_crashed_backend_activity(pid,
2784-
activity_buffer,
2785-
sizeof(activity_buffer));
2787+
if (!EXIT_STATUS_0(exitstatus))
2788+
activity = pgstat_get_crashed_backend_activity(pid,
2789+
activity_buffer,
2790+
sizeof(activity_buffer));
27862791

27872792
if (WIFEXITED(exitstatus))
27882793
ereport(lev,
@@ -2792,7 +2797,7 @@ LogChildExit(int lev, const char *procname, int pid, int exitstatus)
27922797
"server process" */
27932798
(errmsg("%s (PID %d) exited with exit code %d",
27942799
procname, pid, WEXITSTATUS(exitstatus)),
2795-
errdetail("Running query: %s", activity)));
2800+
activity ? errdetail("Failed process was running: %s", activity) : 0));
27962801
else if (WIFSIGNALED(exitstatus))
27972802
#if defined(WIN32)
27982803
ereport(lev,
@@ -2803,7 +2808,7 @@ LogChildExit(int lev, const char *procname, int pid, int exitstatus)
28032808
(errmsg("%s (PID %d) was terminated by exception 0x%X",
28042809
procname, pid, WTERMSIG(exitstatus)),
28052810
errhint("See C include file \"ntstatus.h\" for a description of the hexadecimal value."),
2806-
errdetail("Running query: %s", activity)));
2811+
activity ? errdetail("Failed process was running: %s", activity) : 0));
28072812
#elif defined(HAVE_DECL_SYS_SIGLIST) && HAVE_DECL_SYS_SIGLIST
28082813
ereport(lev,
28092814

@@ -2814,7 +2819,7 @@ LogChildExit(int lev, const char *procname, int pid, int exitstatus)
28142819
procname, pid, WTERMSIG(exitstatus),
28152820
WTERMSIG(exitstatus) < NSIG ?
28162821
sys_siglist[WTERMSIG(exitstatus)] : "(unknown)"),
2817-
errdetail("Running query: %s", activity)));
2822+
activity ? errdetail("Failed process was running: %s", activity) : 0));
28182823
#else
28192824
ereport(lev,
28202825

@@ -2823,7 +2828,7 @@ LogChildExit(int lev, const char *procname, int pid, int exitstatus)
28232828
"server process" */
28242829
(errmsg("%s (PID %d) was terminated by signal %d",
28252830
procname, pid, WTERMSIG(exitstatus)),
2826-
errdetail("Running query: %s", activity)));
2831+
activity ? errdetail("Failed process was running: %s", activity) : 0));
28272832
#endif
28282833
else
28292834
ereport(lev,
@@ -2833,7 +2838,7 @@ LogChildExit(int lev, const char *procname, int pid, int exitstatus)
28332838
"server process" */
28342839
(errmsg("%s (PID %d) exited with unrecognized status %d",
28352840
procname, pid, exitstatus),
2836-
errdetail("Running query: %s", activity)));
2841+
activity ? errdetail("Failed process was running: %s", activity) : 0));
28372842
}
28382843

28392844
/*

‎src/backend/utils/adt/ascii.c

Lines changed: 16 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -160,35 +160,38 @@ to_ascii_default(PG_FUNCTION_ARGS)
160160
}
161161

162162
/* ----------
163-
* "Escape" a string in unknown encoding to a valid ASCII string.
164-
* Replace non-ASCII bytes with '?'
165-
* This must not trigger ereport(ERROR), as it is called from postmaster.
163+
* Copy a string in an arbitrary backend-safe encoding, converting it to a
164+
* valid ASCII string by replacing non-ASCII bytes with '?'. Otherwise the
165+
* behavior is identical to strlcpy(), except that we don't bother with a
166+
* return value.
166167
*
167-
* Unlike C strncpy(), the result is always terminated with exactly one null
168-
* byte.
168+
* This must not trigger ereport(ERROR), as it is called in postmaster.
169169
* ----------
170170
*/
171171
void
172-
ascii_safe_strncpy(char *dest, const char *src, int len)
172+
ascii_safe_strlcpy(char *dest, const char *src, size_t destsiz)
173173
{
174-
int i;
174+
if (destsiz == 0) /* corner case: no room for trailing nul */
175+
return;
175176

176-
for (i = 0; i < (len - 1); i++)
177+
while (--destsiz > 0)
177178
{
178-
unsigned char ch = src[i]; /* use unsigned char here to avoid compiler warning */
179+
/* use unsigned char here to avoid compiler warning */
180+
unsigned char ch = *src++;
179181

180182
if (ch == '\0')
181183
break;
182184
/* Keep printable ASCII characters */
183185
if (32 <= ch && ch <= 127)
184-
dest[i] = ch;
186+
*dest = ch;
185187
/* White-space is also OK */
186188
else if (ch == '\n' || ch == '\r' || ch == '\t')
187-
dest[i] = ch;
189+
*dest = ch;
188190
/* Everything else is replaced with '?' */
189191
else
190-
dest[i] = '?';
192+
*dest = '?';
193+
dest++;
191194
}
192195

193-
dest[i] = '\0';
196+
*dest = '\0';
194197
}

‎src/include/pgstat.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -721,7 +721,7 @@ extern void pgstat_report_xact_timestamp(TimestampTz tstamp);
721721
extern void pgstat_report_waiting(bool waiting);
722722
extern const char *pgstat_get_backend_current_activity(int pid, bool checkUser);
723723
extern const char *pgstat_get_crashed_backend_activity(int pid, char *buffer,
724-
int len);
724+
int buflen);
725725

726726
extern PgStat_TableStatus *find_tabstat_entry(Oid rel_id);
727727
extern PgStat_BackendFunctionEntry *find_funcstat_entry(Oid func_id);

‎src/include/utils/ascii.h

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
extern Datum to_ascii_encname(PG_FUNCTION_ARGS);
1717
extern Datum to_ascii_enc(PG_FUNCTION_ARGS);
1818
extern Datum to_ascii_default(PG_FUNCTION_ARGS);
19-
extern void ascii_safe_strncpy(char *dest, const char *src, int len);
19+
20+
extern void ascii_safe_strlcpy(char *dest, const char *src, size_t destsiz);
2021

2122
#endif /* _ASCII_H_ */

0 commit comments

Comments
 (0)
0