On 2017-09-12 00:19:48 -0700, Andres Freund wrote: > Hi, > > I've recently seen a benchmark in which pg_mbcliplen() showed up > prominently. Which it will basically in any benchmark with longer query > strings, but fast queries. That's not that uncommon. > > I wonder if we could avoid the cost of pg_mbcliplen() from within > pgstat_report_activity(), by moving some of the cost to the read > side. pgstat values are obviously read far less frequently in nearly all > cases that are performance relevant. > > Therefore I wonder if we couldn't just store a querystring that's > essentially just a memcpy()ed prefix, and do a pg_mbcliplen() on the > read side. I think that should work because all *server side* encodings > store character lengths in the *first* byte of a multibyte character > (at least one clientside encoding, gb18030, doesn't behave that way). > > That'd necessitate an added memory copy in pg_stat_get_activity(), but > that seems fairly harmless. > > Faults in my thinking?
Here's a patch that implements that idea. Seems to work well. I'm a bit loathe to add proper regression tests for this, seems awfully dependent on specific track_activity_query_size settings. I did confirm using gdb that I see incomplete characters before pgstat_clip_activity(), but not after. I've renamed st_activity to st_activity_raw to increase the likelihood that potential external users of st_activity notice and adapt. Increases the noise, but imo to a very bareable amount. Don't feel strongly though. Greetings, Andres Freund
>From 9c9503f0dfe1babb21e81c1955e996ad06c93608 Mon Sep 17 00:00:00 2001 From: Andres Freund <and...@anarazel.de> Date: Wed, 13 Sep 2017 19:25:34 -0700 Subject: [PATCH 1/8] 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..ccb528e627 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 allow to determine the length of a + * multi-byte character from it's first byte (not the case for client + * encodings, see GB18030). As st_activity always is 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..3690780236 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 trunctae 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
