On 2017-09-15 08:25:09 -0400, Robert Haas wrote: > On Thu, Sep 14, 2017 at 2:03 AM, Andres Freund <and...@anarazel.de> wrote: > > Here's a patch that implements that idea. > > From the department of cosmetic nitpicking: > > + * All supported server-encodings allow to determine the length of a > > make it possible to determine > > + * multi-byte character from it's first byte (not the case for client > > its > > this is not the case > > + * encodings, see GB18030). As st_activity always is stored using server > > is always stored using a server > > + * pgstat_clip_activity() to trunctae correctly.
Version correcting these is attached. Thanks! Greetings, Andres Freund
>From a2325a2649da403dc562b8e93df972839823d924 Mon Sep 17 00:00:00 2001 From: Andres Freund <and...@anarazel.de> Date: Wed, 13 Sep 2017 19:25:34 -0700 Subject: [PATCH] Speedup pgstat_report_activity by moving mb-aware truncation to read side. Previously multi-byte aware truncation was done on every pgstat_report_activity() call - proving to be a bottleneck for workloads with long query strings that execute quickly. Instead move the truncation to the read side, which is commonly executed far less frequently. That's possible because all server encodings allow to determine the length of a multi-byte string from the first byte. Author: Andres Freund Discussion: https://postgr.es/m/20170912071948.pa7igbpkkkvie...@alap3.anarazel.de --- src/backend/postmaster/pgstat.c | 63 ++++++++++++++++++++++++++++--------- src/backend/utils/adt/pgstatfuncs.c | 17 +++++++--- src/include/pgstat.h | 12 +++++-- 3 files changed, 72 insertions(+), 20 deletions(-) diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c index accf302cf7..1ffdac5448 100644 --- a/src/backend/postmaster/pgstat.c +++ b/src/backend/postmaster/pgstat.c @@ -2701,7 +2701,7 @@ CreateSharedBackendStatus(void) buffer = BackendActivityBuffer; for (i = 0; i < NumBackendStatSlots; i++) { - BackendStatusArray[i].st_activity = buffer; + BackendStatusArray[i].st_activity_raw = buffer; buffer += pgstat_track_activity_query_size; } } @@ -2922,11 +2922,11 @@ pgstat_bestart(void) #endif beentry->st_state = STATE_UNDEFINED; beentry->st_appname[0] = '\0'; - beentry->st_activity[0] = '\0'; + beentry->st_activity_raw[0] = '\0'; /* Also make sure the last byte in each string area is always 0 */ beentry->st_clienthostname[NAMEDATALEN - 1] = '\0'; beentry->st_appname[NAMEDATALEN - 1] = '\0'; - beentry->st_activity[pgstat_track_activity_query_size - 1] = '\0'; + beentry->st_activity_raw[pgstat_track_activity_query_size - 1] = '\0'; beentry->st_progress_command = PROGRESS_COMMAND_INVALID; beentry->st_progress_command_target = InvalidOid; @@ -3017,7 +3017,7 @@ pgstat_report_activity(BackendState state, const char *cmd_str) pgstat_increment_changecount_before(beentry); beentry->st_state = STATE_DISABLED; beentry->st_state_start_timestamp = 0; - beentry->st_activity[0] = '\0'; + beentry->st_activity_raw[0] = '\0'; beentry->st_activity_start_timestamp = 0; /* st_xact_start_timestamp and wait_event_info are also disabled */ beentry->st_xact_start_timestamp = 0; @@ -3034,8 +3034,12 @@ pgstat_report_activity(BackendState state, const char *cmd_str) start_timestamp = GetCurrentStatementStartTimestamp(); if (cmd_str != NULL) { - len = pg_mbcliplen(cmd_str, strlen(cmd_str), - pgstat_track_activity_query_size - 1); + /* + * Compute length of to-be-stored string unaware of multi-byte + * characters. For speed reasons that'll get corrected on read, rather + * than computed every write. + */ + len = Min(strlen(cmd_str), pgstat_track_activity_query_size - 1); } current_timestamp = GetCurrentTimestamp(); @@ -3049,8 +3053,8 @@ pgstat_report_activity(BackendState state, const char *cmd_str) if (cmd_str != NULL) { - memcpy((char *) beentry->st_activity, cmd_str, len); - beentry->st_activity[len] = '\0'; + memcpy((char *) beentry->st_activity_raw, cmd_str, len); + beentry->st_activity_raw[len] = '\0'; beentry->st_activity_start_timestamp = start_timestamp; } @@ -3278,8 +3282,8 @@ pgstat_read_current_status(void) */ strcpy(localappname, (char *) beentry->st_appname); localentry->backendStatus.st_appname = localappname; - strcpy(localactivity, (char *) beentry->st_activity); - localentry->backendStatus.st_activity = localactivity; + strcpy(localactivity, (char *) beentry->st_activity_raw); + localentry->backendStatus.st_activity_raw = localactivity; localentry->backendStatus.st_ssl = beentry->st_ssl; #ifdef USE_SSL if (beentry->st_ssl) @@ -3945,10 +3949,13 @@ pgstat_get_backend_current_activity(int pid, bool checkUser) /* Now it is safe to use the non-volatile pointer */ if (checkUser && !superuser() && beentry->st_userid != GetUserId()) return "<insufficient privilege>"; - else if (*(beentry->st_activity) == '\0') + else if (*(beentry->st_activity_raw) == '\0') return "<command string not enabled>"; else - return beentry->st_activity; + { + /* this'll leak a bit of memory, but that seems acceptable */ + return pgstat_clip_activity(beentry->st_activity_raw); + } } beentry++; @@ -3994,7 +4001,7 @@ pgstat_get_crashed_backend_activity(int pid, char *buffer, int buflen) if (beentry->st_procpid == pid) { /* Read pointer just once, so it can't change after validation */ - const char *activity = beentry->st_activity; + const char *activity = beentry->st_activity_raw; const char *activity_last; /* @@ -4017,7 +4024,8 @@ pgstat_get_crashed_backend_activity(int pid, char *buffer, int buflen) /* * Copy only ASCII-safe characters so we don't run into encoding * problems when reporting the message; and be sure not to run off - * the end of memory. + * the end of memory. As only ASCII characters are reported, it + * doesn't seem necessary to perform multibyte aware clipping. */ ascii_safe_strlcpy(buffer, activity, Min(buflen, pgstat_track_activity_query_size)); @@ -6270,3 +6278,30 @@ pgstat_db_requested(Oid databaseid) return false; } + +/* + * Convert a potentially unsafely truncated activity string (see + * PgBackendStatus.st_activity_raw's documentation) into a correctly truncated + * one. + * + * The returned string is allocated in the caller's memory context and may be + * freed. + */ +char * +pgstat_clip_activity(const char *activity) +{ + int rawlen = strnlen(activity, pgstat_track_activity_query_size - 1); + int cliplen; + + /* + * All supported server-encodings make it possible to determine the length + * of a multi-byte character from its first byte (this is not the case for + * client encodings, see GB18030). As st_activity is always stored using + * server encoding, this allows us to perform multi-byte aware truncation, + * even if the string earlier was truncated in the middle of a multi-byte + * character. + */ + cliplen = pg_mbcliplen(activity, rawlen, + pgstat_track_activity_query_size - 1); + return pnstrdup(activity, cliplen); +} diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c index 20ce48b2d8..5a968e3758 100644 --- a/src/backend/utils/adt/pgstatfuncs.c +++ b/src/backend/utils/adt/pgstatfuncs.c @@ -664,6 +664,7 @@ pg_stat_get_activity(PG_FUNCTION_ARGS) is_member_of_role(GetUserId(), DEFAULT_ROLE_READ_ALL_STATS)) { SockAddr zero_clientaddr; + char *clipped_activity; switch (beentry->st_state) { @@ -690,7 +691,9 @@ pg_stat_get_activity(PG_FUNCTION_ARGS) break; } - values[5] = CStringGetTextDatum(beentry->st_activity); + clipped_activity = pgstat_clip_activity(beentry->st_activity_raw); + values[5] = CStringGetTextDatum(clipped_activity); + pfree(clipped_activity); proc = BackendPidGetProc(beentry->st_procpid); if (proc != NULL) @@ -906,17 +909,23 @@ pg_stat_get_backend_activity(PG_FUNCTION_ARGS) int32 beid = PG_GETARG_INT32(0); PgBackendStatus *beentry; const char *activity; + char *clipped_activity; + text *ret; if ((beentry = pgstat_fetch_stat_beentry(beid)) == NULL) activity = "<backend information not available>"; else if (!has_privs_of_role(GetUserId(), beentry->st_userid)) activity = "<insufficient privilege>"; - else if (*(beentry->st_activity) == '\0') + else if (*(beentry->st_activity_raw) == '\0') activity = "<command string not enabled>"; else - activity = beentry->st_activity; + activity = beentry->st_activity_raw; - PG_RETURN_TEXT_P(cstring_to_text(activity)); + clipped_activity = pgstat_clip_activity(activity); + ret = cstring_to_text(activity); + pfree(clipped_activity); + + PG_RETURN_TEXT_P(ret); } Datum diff --git a/src/include/pgstat.h b/src/include/pgstat.h index 57ac5d41e4..52af0aa541 100644 --- a/src/include/pgstat.h +++ b/src/include/pgstat.h @@ -1003,8 +1003,14 @@ typedef struct PgBackendStatus /* application name; MUST be null-terminated */ char *st_appname; - /* current command string; MUST be null-terminated */ - char *st_activity; + /* + * Current command string; MUST be null-terminated. Note that this string + * possibly is truncated in the middle of a multi-byte character. As + * activity strings are stored more frequently than read, that allows to + * move the cost of correct truncation to the display side. Use + * pgstat_clip_activity() to truncate correctly. + */ + char *st_activity_raw; /* * Command progress reporting. Any command which wishes can advertise @@ -1193,6 +1199,8 @@ extern PgStat_BackendFunctionEntry *find_funcstat_entry(Oid func_id); extern void pgstat_initstats(Relation rel); +extern char *pgstat_clip_activity(const char *activity); + /* ---------- * pgstat_report_wait_start() - * -- 2.14.1.536.g6867272d5b.dirty
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
