Re: [PATCHES] A GUC variable to replace PGBE_ACTIVITY_SIZE

2008-06-24 Thread Heikki Linnakangas

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

--
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches


Re: [PATCHES] A GUC variable to replace PGBE_ACTIVITY_SIZE

2008-06-23 Thread Neil Conway
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



-- 
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches


[PATCHES] A GUC variable to replace PGBE_ACTIVITY_SIZE

2008-06-22 Thread Thomas Lee
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,