Обсуждение: A GUC variable to replace PGBE_ACTIVITY_SIZE
Attached is a patch providing a new GUC variable called "track_activity_query_size", as previously discussed on pgsql-hackers here: http://archives.postgresql.org/pgsql-hackers/2008-06/msg00814.php This is my first patch for postgres, so I'm sure it may need some love. In particular: * Should it be possible to set this new variable via a command-line option ala shared_buffers? * I'm not exactly sure about the best way to add unit/regr tests for this change. * I'm also not sure what's required in the way of updates to the documentation. Any comments or suggestions would be most appreciated. Cheers, T diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c index f178fe3..912c1cf 100644 --- a/src/backend/postmaster/pgstat.c +++ b/src/backend/postmaster/pgstat.c @@ -101,6 +101,7 @@ bool pgstat_track_activities = false; bool pgstat_track_counts = false; int pgstat_track_functions = TRACK_FUNC_OFF; +int pgstat_track_activity_query_size = PGBE_DEFAULT_ACTIVITY_SIZE; /* * BgWriter global statistics counters (unused in other processes). @@ -2010,6 +2011,7 @@ pgstat_fetch_global(void) static PgBackendStatus *BackendStatusArray = NULL; static PgBackendStatus *MyBEEntry = NULL; +static char* BackendActivityBuffer = NULL; /* @@ -2025,7 +2027,26 @@ BackendStatusShmemSize(void) } /* - * Initialize the shared status array during postmaster startup. + * Ensures that every element of BackendStatusArray has a valid st_activity + * pointer. + */ +static void +pgstat_initialize_activity_pointers(void) +{ + Size i; + PgBackendStatus* beentry = BackendStatusArray; + char* buffer = BackendActivityBuffer; + + for (i = 0; i < MaxBackends; i++) { + beentry->st_activity = buffer; + buffer += pgstat_track_activity_query_size; + beentry++; + } +} + +/* + * Initialize the shared status array & activity buffer during postmaster + * startup. */ void CreateSharedBackendStatus(void) @@ -2044,6 +2065,17 @@ CreateSharedBackendStatus(void) */ MemSet(BackendStatusArray, 0, size); } + + size = mul_size(pgstat_track_activity_query_size, MaxBackends); + BackendActivityBuffer = (char*) + ShmemInitStruct("Backend Activity Buffer", size, &found); + + if (!found) + { + MemSet(BackendActivityBuffer, 0, size); + } + + pgstat_initialize_activity_pointers(); } @@ -2128,7 +2160,7 @@ pgstat_bestart(void) beentry->st_waiting = false; beentry->st_activity[0] = '\0'; /* Also make sure the last byte in the string area is always 0 */ - beentry->st_activity[PGBE_ACTIVITY_SIZE - 1] = '\0'; + beentry->st_activity[pgstat_track_activity_query_size - 1] = '\0'; beentry->st_changecount++; Assert((beentry->st_changecount & 1) == 0); @@ -2188,7 +2220,7 @@ pgstat_report_activity(const char *cmd_str) start_timestamp = GetCurrentStatementStartTimestamp(); len = strlen(cmd_str); - len = pg_mbcliplen(cmd_str, len, PGBE_ACTIVITY_SIZE - 1); + len = pg_mbcliplen(cmd_str, len, pgstat_track_activity_query_size - 1); /* * Update my status entry, following the protocol of bumping @@ -2267,6 +2299,7 @@ pgstat_read_current_status(void) volatile PgBackendStatus *beentry; PgBackendStatus *localtable; PgBackendStatus *localentry; + char *localactivity; int i; Assert(!pgStatRunningInCollector); @@ -2278,6 +2311,9 @@ pgstat_read_current_status(void) localtable = (PgBackendStatus *) MemoryContextAlloc(pgStatLocalContext, sizeof(PgBackendStatus) * MaxBackends); + localactivity = (char *) + MemoryContextAlloc(pgStatLocalContext, + pgstat_track_activity_query_size * MaxBackends); localNumBackends = 0; beentry = BackendStatusArray; @@ -2296,10 +2332,14 @@ pgstat_read_current_status(void) int save_changecount = beentry->st_changecount; /* - * XXX if PGBE_ACTIVITY_SIZE is really large, it might be best to - * use strcpy not memcpy for copying the activity string? + * XXX if pgstat_track_activity_query_size is really large, + * it might be best to use strcpy not memcpy for copying the + * activity string? */ memcpy(localentry, (char *) beentry, sizeof(PgBackendStatus)); + memcpy(localactivity, (char *) beentry->st_activity, + pgstat_track_activity_query_size); + localentry->st_activity = localactivity; if (save_changecount == beentry->st_changecount && (save_changecount & 1) == 0) @@ -2314,9 +2354,10 @@ pgstat_read_current_status(void) if (localentry->st_procpid > 0) { localentry++; + localactivity += pgstat_track_activity_query_size; localNumBackends++; } - } + } /* Set the pointer only after completion of a valid table */ localBackendStatusTable = localtable; diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index fa96437..b7f1302 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -1311,6 +1311,15 @@ static struct config_int ConfigureNamesInt[] = }, { + {"track_activity_query_size", PGC_POSTMASTER, RESOURCES_MEM, + gettext_noop("Sets the maximum number of characters that will be displayed for pg_stat_activity.current_query."), + NULL, + }, + &pgstat_track_activity_query_size, + PGBE_DEFAULT_ACTIVITY_SIZE, 100, 102400, NULL, NULL + }, + + { {"temp_buffers", PGC_USERSET, RESOURCES_MEM, gettext_noop("Sets the maximum number of temporary buffers used by each session."), NULL, diff --git a/src/include/pgstat.h b/src/include/pgstat.h index a9f5501..03b9465 100644 --- a/src/include/pgstat.h +++ b/src/include/pgstat.h @@ -509,8 +509,8 @@ typedef struct PgStat_GlobalStats * ---------- */ -/* Max length of st_activity string ... perhaps replace with a GUC var? */ -#define PGBE_ACTIVITY_SIZE 1024 +/* Default length of st_activity string (see backend_activity_size GUC) */ +#define PGBE_DEFAULT_ACTIVITY_SIZE 1024 /* ---------- * PgBackendStatus @@ -551,7 +551,7 @@ typedef struct PgBackendStatus bool st_waiting; /* current command string; MUST be null-terminated */ - char st_activity[PGBE_ACTIVITY_SIZE]; + char *st_activity; } PgBackendStatus; /* @@ -578,6 +578,7 @@ typedef struct PgStat_FunctionCallUsage extern bool pgstat_track_activities; extern bool pgstat_track_counts; extern int pgstat_track_functions; +extern int pgstat_track_activity_query_size; /* * BgWriter statistics counters are updated directly by bgwriter and bufmgr
On Mon, 2008-06-23 at 03:52 +1000, Thomas Lee wrote: > * Should it be possible to set this new variable via a command-line > option ala shared_buffers? I would say not: most parameters cannot be set by special command-line parameters, and this one is not important enough to warrant special treatment. Instead, "--track_activity_query_size=x" can be used as-is. > * I'm not exactly sure about the best way to add unit/regr tests for > this change. AFAICS there isn't an easy way. > * I'm also not sure what's required in the way of updates to the > documentation. postgresql.conf.sample and doc/src/sgml/config.sgml should be updated at a minimum. -Neil
I'd like to see some quick testing of this thing mentioned in the comments: > /* > * XXX if pgstat_track_activity_query_size is really large, > * it might be best to use strcpy not memcpy for copying the > * activity string? > */ If we make it a GUC, there will be people running with "really large" pgstat_track_activity_query_size settings. In fact I wouldn't be surprised if people routinely cranked it up to the 10-100 KB range, just in case. It's going to be platform-dependent, for sure, but some quick micro-benchmarks of where the break-even point between memcpy and strcpy lies would be nice. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
An updated patch for the track_activity_query_size GUC variable.
Made sure it's a context diff this time around, as per the wiki
documentation.
Cheers,
T
*** a/doc/src/sgml/config.sgml
--- b/doc/src/sgml/config.sgml
***************
*** 3331,3336 **** COPY postgres_log FROM '/full/path/to/logfile.csv' WITH csv;
--- 3331,3349 ----
</listitem>
</varlistentry>
+ <varlistentry id="guc-track-active-query-size" xreflabel="track_active_query_size">
+ <term><varname>track_active_query_size</varname> (<type>integer</type>)</term>
+ <indexterm>
+ <primary><varname>track_active_query_size</> configuration parameter</primary>
+ </indexterm>
+ <listitem>
+ Specifies the maximum number of characters used to track the currently
+ executing command for each active session. This parameter has a value
+ of 1024 by default.
+ Only superusers can change this setting.
+ </listitem>
+ </varlistentry>
+
<varlistentry id="guc-track-counts" xreflabel="track_counts">
<term><varname>track_counts</varname> (<type>boolean</type>)</term>
<indexterm>
*** a/src/backend/postmaster/pgstat.c
--- b/src/backend/postmaster/pgstat.c
***************
*** 101,106 ****
--- 101,107 ----
bool pgstat_track_activities = false;
bool pgstat_track_counts = false;
int pgstat_track_functions = TRACK_FUNC_OFF;
+ int pgstat_track_activity_query_size = PGBE_DEFAULT_ACTIVITY_SIZE;
/*
* BgWriter global statistics counters (unused in other processes).
***************
*** 2010,2015 **** pgstat_fetch_global(void)
--- 2011,2017 ----
static PgBackendStatus *BackendStatusArray = NULL;
static PgBackendStatus *MyBEEntry = NULL;
+ static char* BackendActivityBuffer = NULL;
/*
***************
*** 2025,2031 **** BackendStatusShmemSize(void)
}
/*
! * Initialize the shared status array during postmaster startup.
*/
void
CreateSharedBackendStatus(void)
--- 2027,2052 ----
}
/*
! * Ensures that every element of BackendStatusArray has a valid st_activity
! * pointer.
! */
! static void
! pgstat_initialize_activity_pointers(void)
! {
! Size i;
! PgBackendStatus* beentry = BackendStatusArray;
! char* buffer = BackendActivityBuffer;
!
! for (i = 0; i < MaxBackends; i++) {
! beentry->st_activity = buffer;
! buffer += pgstat_track_activity_query_size;
! beentry++;
! }
! }
!
! /*
! * Initialize the shared status array & activity buffer during postmaster
! * startup.
*/
void
CreateSharedBackendStatus(void)
***************
*** 2044,2049 **** CreateSharedBackendStatus(void)
--- 2065,2081 ----
*/
MemSet(BackendStatusArray, 0, size);
}
+
+ size = mul_size(pgstat_track_activity_query_size, MaxBackends);
+ BackendActivityBuffer = (char*)
+ ShmemInitStruct("Backend Activity Buffer", size, &found);
+
+ if (!found)
+ {
+ MemSet(BackendActivityBuffer, 0, size);
+ }
+
+ pgstat_initialize_activity_pointers();
}
***************
*** 2128,2134 **** pgstat_bestart(void)
beentry->st_waiting = false;
beentry->st_activity[0] = '\0';
/* Also make sure the last byte in the string area is always 0 */
! beentry->st_activity[PGBE_ACTIVITY_SIZE - 1] = '\0';
beentry->st_changecount++;
Assert((beentry->st_changecount & 1) == 0);
--- 2160,2166 ----
beentry->st_waiting = false;
beentry->st_activity[0] = '\0';
/* Also make sure the last byte in the string area is always 0 */
! beentry->st_activity[pgstat_track_activity_query_size - 1] = '\0';
beentry->st_changecount++;
Assert((beentry->st_changecount & 1) == 0);
***************
*** 2188,2194 **** pgstat_report_activity(const char *cmd_str)
start_timestamp = GetCurrentStatementStartTimestamp();
len = strlen(cmd_str);
! len = pg_mbcliplen(cmd_str, len, PGBE_ACTIVITY_SIZE - 1);
/*
* Update my status entry, following the protocol of bumping
--- 2220,2226 ----
start_timestamp = GetCurrentStatementStartTimestamp();
len = strlen(cmd_str);
! len = pg_mbcliplen(cmd_str, len, pgstat_track_activity_query_size - 1);
/*
* Update my status entry, following the protocol of bumping
***************
*** 2267,2272 **** pgstat_read_current_status(void)
--- 2299,2305 ----
volatile PgBackendStatus *beentry;
PgBackendStatus *localtable;
PgBackendStatus *localentry;
+ char *localactivity;
int i;
Assert(!pgStatRunningInCollector);
***************
*** 2278,2283 **** pgstat_read_current_status(void)
--- 2311,2319 ----
localtable = (PgBackendStatus *)
MemoryContextAlloc(pgStatLocalContext,
sizeof(PgBackendStatus) * MaxBackends);
+ localactivity = (char *)
+ MemoryContextAlloc(pgStatLocalContext,
+ pgstat_track_activity_query_size * MaxBackends);
localNumBackends = 0;
beentry = BackendStatusArray;
***************
*** 2296,2305 **** pgstat_read_current_status(void)
int save_changecount = beentry->st_changecount;
/*
! * XXX if PGBE_ACTIVITY_SIZE is really large, it might be best to
! * use strcpy not memcpy for copying the activity string?
*/
memcpy(localentry, (char *) beentry, sizeof(PgBackendStatus));
if (save_changecount == beentry->st_changecount &&
(save_changecount & 1) == 0)
--- 2332,2345 ----
int save_changecount = beentry->st_changecount;
/*
! * XXX if pgstat_track_activity_query_size is really large,
! * it might be best to use strcpy not memcpy for copying the
! * activity string?
*/
memcpy(localentry, (char *) beentry, sizeof(PgBackendStatus));
+ memcpy(localactivity, (char *) beentry->st_activity,
+ pgstat_track_activity_query_size);
+ localentry->st_activity = localactivity;
if (save_changecount == beentry->st_changecount &&
(save_changecount & 1) == 0)
***************
*** 2314,2322 **** pgstat_read_current_status(void)
if (localentry->st_procpid > 0)
{
localentry++;
localNumBackends++;
}
! }
/* Set the pointer only after completion of a valid table */
localBackendStatusTable = localtable;
--- 2354,2363 ----
if (localentry->st_procpid > 0)
{
localentry++;
+ localactivity += pgstat_track_activity_query_size;
localNumBackends++;
}
! }
/* Set the pointer only after completion of a valid table */
localBackendStatusTable = localtable;
*** a/src/backend/utils/misc/guc.c
--- b/src/backend/utils/misc/guc.c
***************
*** 1311,1316 **** static struct config_int ConfigureNamesInt[] =
--- 1311,1325 ----
},
{
+ {"track_activity_query_size", PGC_POSTMASTER, RESOURCES_MEM,
+ gettext_noop("Sets the maximum number of characters that will be displayed for
pg_stat_activity.current_query."),
+ NULL,
+ },
+ &pgstat_track_activity_query_size,
+ PGBE_DEFAULT_ACTIVITY_SIZE, 100, 102400, NULL, NULL
+ },
+
+ {
{"temp_buffers", PGC_USERSET, RESOURCES_MEM,
gettext_noop("Sets the maximum number of temporary buffers used by each session."),
NULL,
*** a/src/backend/utils/misc/postgresql.conf.sample
--- b/src/backend/utils/misc/postgresql.conf.sample
***************
*** 364,369 ****
--- 364,370 ----
#track_activities = on
#track_counts = on
#track_functions = none # none, pl, all
+ #track_active_query_size = 1024
#update_process_title = on
*** a/src/include/pgstat.h
--- b/src/include/pgstat.h
***************
*** 509,516 **** typedef struct PgStat_GlobalStats
* ----------
*/
! /* Max length of st_activity string ... perhaps replace with a GUC var? */
! #define PGBE_ACTIVITY_SIZE 1024
/* ----------
* PgBackendStatus
--- 509,516 ----
* ----------
*/
! /* Default length of st_activity string (see backend_activity_size GUC) */
! #define PGBE_DEFAULT_ACTIVITY_SIZE 1024
/* ----------
* PgBackendStatus
***************
*** 551,557 **** typedef struct PgBackendStatus
bool st_waiting;
/* current command string; MUST be null-terminated */
! char st_activity[PGBE_ACTIVITY_SIZE];
} PgBackendStatus;
/*
--- 551,557 ----
bool st_waiting;
/* current command string; MUST be null-terminated */
! char *st_activity;
} PgBackendStatus;
/*
***************
*** 578,583 **** typedef struct PgStat_FunctionCallUsage
--- 578,584 ----
extern bool pgstat_track_activities;
extern bool pgstat_track_counts;
extern int pgstat_track_functions;
+ extern int pgstat_track_activity_query_size;
/*
* BgWriter statistics counters are updated directly by bgwriter and bufmgr
Thomas Lee wrote: > An updated patch for the track_activity_query_size GUC variable. Thanks, committed with some changes. There was one bug: BackendStatusShmemSize() needs to report the memory needed for the activity string buffers; it's used for sizing the single fixed size shmem block on postmaster startup. I also fixed the documentation to refer to the setting with the right name, noted that it's a startup time option, and changed the wording a bit. I tested the performance between strcpy and memcpy a little bit. As expected, strcpy is a lot cheaper if the string is small. I chose strcpy, since it's very likely that the buffer is over-sized, and even if it's just the right size to hold typical queries, it's going to be "<IDLE>" for idle connections. Another simple optimization occurred to me while looking at this: we should skip the memcpy/strcpy altogether if the BackendActivity slot is not in use. That seems like a good bet, you usually don't try to max out max_connections. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
"Heikki Linnakangas" <heikki@enterprisedb.com> writes:
> Another simple optimization occurred to me while looking at this: we
> should skip the memcpy/strcpy altogether if the BackendActivity slot is
> not in use. That seems like a good bet, you usually don't try to max out
> max_connections.
Huh? How could we be assigning to a slot that is not in use?
regards, tom lane
Tom Lane wrote: > "Heikki Linnakangas" <heikki@enterprisedb.com> writes: >> Another simple optimization occurred to me while looking at this: we >> should skip the memcpy/strcpy altogether if the BackendActivity slot is >> not in use. That seems like a good bet, you usually don't try to max out >> max_connections. > > Huh? How could we be assigning to a slot that is not in use? Before the patch, we loop through the shared PgBackendStatus slots (there is MaxBackends of them), and issue a memcpy for each to copy it to our local slot. After that, we check if it was actually in use. After the patch, we still loop through the shared slots, but only issue the memcpy for slots that are in use. Hope this answers your question, I'm not sure what you meant... -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
"Heikki Linnakangas" <heikki@enterprisedb.com> writes:
> Tom Lane wrote:
>> Huh? How could we be assigning to a slot that is not in use?
> Before the patch, we loop through the shared PgBackendStatus slots
> (there is MaxBackends of them), and issue a memcpy for each to copy it
> to our local slot. After that, we check if it was actually in use.
> After the patch, we still loop through the shared slots, but only issue
> the memcpy for slots that are in use.
Oh, you're talking about *fetching* the slot contents, not *assigning*
to them. Sorry, probably should have read the patch instead of just
reacting to the comment ...
regards, tom lane