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 commonly is
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.

Rename PgBackendStatus.st_activity to st_activity_raw so existing
extension users of the field break - their code has to be adjusted to
use pgstat_clip_activity().

Author: Andres Freund
Tested-By: Khuntal Ghosh
Reviewed-By: Robert Haas, Tom Lane
Discussion: https://postgr.es/m/20170912071948.pa7igbpkkkviecpz@alap3.anarazel.de

diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index accf302cf73f079eef239bad25d55ebf9f5b59fa..1ffdac5448d9c45aad021ebd54f01ab1f523aba5 100644 (file)
--- 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 20ce48b2d8272ffa04b4df4a071cc467c88cefb4..5a968e3758f6d28a9c85d2cac697931ac9e6d8ca 100644 (file)
--- 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 57ac5d41e4628f92c83cadd0aa09cfdd127feef3..52af0aa5415b1e615f3087c8627a728bc3a8d557 100644 (file)
--- 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() -
  *