log_hostname and pg_stat_activity

Started by Peter Eisentrautabout 15 years ago16 messages
#1Peter Eisentraut
peter_e@gmx.net

Somehow I fantasized that log_hostname would also turn
pg_stat_activity.client_addr into names instead of IP addresses. It
doesn't, but perhaps it should? It would be nice to be able to
configure an IP-address free setup. Or would it be worth having a
separate configuration parameter for that?

#2Magnus Hagander
magnus@hagander.net
In reply to: Peter Eisentraut (#1)
Re: log_hostname and pg_stat_activity

On Thu, Dec 23, 2010 at 22:09, Peter Eisentraut <peter_e@gmx.net> wrote:

Somehow I fantasized that log_hostname would also turn
pg_stat_activity.client_addr into names instead of IP addresses.  It
doesn't, but perhaps it should?  It would be nice to be able to
configure an IP-address free setup.  Or would it be worth having a
separate configuration parameter for that?

It should certainly be renamed to something else if it does both, but
I don't see the point of having two separate parameters between them.
As long as you can use a cached version of the lookup, you're only
paying the price once, after all...

However, pg_stat_activity.client_addr is an inet field, not a text
string, so you'd have to invent a separate field for it I think...

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

#3Peter Eisentraut
peter_e@gmx.net
In reply to: Magnus Hagander (#2)
1 attachment(s)
Re: log_hostname and pg_stat_activity

On tor, 2010-12-23 at 22:21 +0100, Magnus Hagander wrote:

On Thu, Dec 23, 2010 at 22:09, Peter Eisentraut <peter_e@gmx.net> wrote:

Somehow I fantasized that log_hostname would also turn
pg_stat_activity.client_addr into names instead of IP addresses. It
doesn't, but perhaps it should? It would be nice to be able to
configure an IP-address free setup. Or would it be worth having a
separate configuration parameter for that?

It should certainly be renamed to something else if it does both, but
I don't see the point of having two separate parameters between them.
As long as you can use a cached version of the lookup, you're only
paying the price once, after all...

However, pg_stat_activity.client_addr is an inet field, not a text
string, so you'd have to invent a separate field for it I think...

Here is a patch that adds a client_hostname field to pg_stat_activity.
It takes the hostname if it is available either by having log_hostname
set or if the pg_hba.conf processing resolved it. So I think for most
people who would care about this thing, it would "just work".

I'm not so sure about the pgstat.c internals. Do we need the separate
buffer in shared memory, as in this patch, or could we just copy the
name directly into the PgBackendStatus struct? I guess not the latter,
since my compiler complains about copying a string into a volatile
pointer.

Attachments:

pg-stat-activity-client-hostname.patchtext/x-patch; charset=UTF-8; name=pg-stat-activity-client-hostname.patchDownload
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 718e996..dd92728 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -482,6 +482,7 @@ CREATE VIEW pg_stat_activity AS
             U.rolname AS usename,
             S.application_name,
             S.client_addr,
+            S.client_hostname,
             S.client_port,
             S.backend_start,
             S.xact_start,
@@ -499,6 +500,7 @@ CREATE VIEW pg_stat_replication AS
             U.rolname AS usename,
             S.application_name,
             S.client_addr,
+            S.client_hostname,
             S.client_port,
             S.backend_start,
             W.state,
diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index 301568f..3a00157 100644
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -2223,6 +2223,7 @@ pgstat_fetch_global(void)
 
 static PgBackendStatus *BackendStatusArray = NULL;
 static PgBackendStatus *MyBEEntry = NULL;
+static char *BackendClientHostnameBuffer = NULL;
 static char *BackendAppnameBuffer = NULL;
 static char *BackendActivityBuffer = NULL;
 
@@ -2244,7 +2245,7 @@ BackendStatusShmemSize(void)
 }
 
 /*
- * Initialize the shared status array and activity/appname string buffers
+ * Initialize the shared status array and several string buffers
  * during postmaster startup.
  */
 void
@@ -2286,6 +2287,24 @@ CreateSharedBackendStatus(void)
 		}
 	}
 
+	/* Create or attach to the shared client hostname buffer */
+	size = mul_size(NAMEDATALEN, MaxBackends);
+	BackendClientHostnameBuffer = (char *)
+		ShmemInitStruct("Backend Client Host Name Buffer", size, &found);
+
+	if (!found)
+	{
+		MemSet(BackendClientHostnameBuffer, 0, size);
+
+		/* Initialize st_clienthostname pointers. */
+		buffer = BackendClientHostnameBuffer;
+		for (i = 0; i < MaxBackends; i++)
+		{
+			BackendStatusArray[i].st_clienthostname = buffer;
+			buffer += NAMEDATALEN;
+		}
+	}
+
 	/* Create or attach to the shared activity buffer */
 	size = mul_size(pgstat_track_activity_query_size, MaxBackends);
 	BackendActivityBuffer = (char *)
@@ -2386,16 +2405,21 @@ pgstat_bestart(void)
 	beentry->st_databaseid = MyDatabaseId;
 	beentry->st_userid = userid;
 	beentry->st_clientaddr = clientaddr;
+	beentry->st_clienthostname[0] = '\0';
 	beentry->st_waiting = false;
 	beentry->st_appname[0] = '\0';
 	beentry->st_activity[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_changecount++;
 	Assert((beentry->st_changecount & 1) == 0);
 
+	if (MyProcPort && MyProcPort->remote_hostname)
+		strlcpy(beentry->st_clienthostname, MyProcPort->remote_hostname, NAMEDATALEN);
+
 	/* Update app name to current GUC setting */
 	if (application_name)
 		pgstat_report_appname(application_name);
diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c
index a95ba8b..a608495 100644
--- a/src/backend/utils/adt/pgstatfuncs.c
+++ b/src/backend/utils/adt/pgstatfuncs.c
@@ -504,7 +504,7 @@ pg_stat_get_activity(PG_FUNCTION_ARGS)
 
 		oldcontext = MemoryContextSwitchTo(funcctx->multi_call_memory_ctx);
 
-		tupdesc = CreateTemplateTupleDesc(11, false);
+		tupdesc = CreateTemplateTupleDesc(12, false);
 		TupleDescInitEntry(tupdesc, (AttrNumber) 1, "datid", OIDOID, -1, 0);
 		TupleDescInitEntry(tupdesc, (AttrNumber) 2, "procpid", INT4OID, -1, 0);
 		TupleDescInitEntry(tupdesc, (AttrNumber) 3, "usesysid", OIDOID, -1, 0);
@@ -515,7 +515,8 @@ pg_stat_get_activity(PG_FUNCTION_ARGS)
 		TupleDescInitEntry(tupdesc, (AttrNumber) 8, "query_start", TIMESTAMPTZOID, -1, 0);
 		TupleDescInitEntry(tupdesc, (AttrNumber) 9, "backend_start", TIMESTAMPTZOID, -1, 0);
 		TupleDescInitEntry(tupdesc, (AttrNumber) 10, "client_addr", INETOID, -1, 0);
-		TupleDescInitEntry(tupdesc, (AttrNumber) 11, "client_port", INT4OID, -1, 0);
+		TupleDescInitEntry(tupdesc, (AttrNumber) 11, "client_hostname", TEXTOID, -1, 0);
+		TupleDescInitEntry(tupdesc, (AttrNumber) 12, "client_port", INT4OID, -1, 0);
 
 		funcctx->tuple_desc = BlessTupleDesc(tupdesc);
 
@@ -567,8 +568,8 @@ pg_stat_get_activity(PG_FUNCTION_ARGS)
 	if (funcctx->call_cntr < funcctx->max_calls)
 	{
 		/* for each row */
-		Datum		values[11];
-		bool		nulls[11];
+		Datum		values[12];
+		bool		nulls[12];
 		HeapTuple	tuple;
 		PgBackendStatus *beentry;
 		SockAddr	zero_clientaddr;
@@ -645,6 +646,7 @@ pg_stat_get_activity(PG_FUNCTION_ARGS)
 			{
 				nulls[9] = true;
 				nulls[10] = true;
+				nulls[11] = true;
 			}
 			else
 			{
@@ -669,13 +671,18 @@ pg_stat_get_activity(PG_FUNCTION_ARGS)
 					{
 						nulls[9] = true;
 						nulls[10] = true;
+						nulls[11] = true;
 					}
 					else
 					{
 						clean_ipv6_addr(beentry->st_clientaddr.addr.ss_family, remote_host);
 						values[9] = DirectFunctionCall1(inet_in,
 											   CStringGetDatum(remote_host));
-						values[10] = Int32GetDatum(atoi(remote_port));
+						if (beentry->st_clienthostname)
+							values[10] = CStringGetTextDatum(beentry->st_clienthostname);
+						else
+							nulls[10] = true;
+						values[11] = Int32GetDatum(atoi(remote_port));
 					}
 				}
 				else if (beentry->st_clientaddr.addr.ss_family == AF_UNIX)
@@ -687,13 +694,15 @@ pg_stat_get_activity(PG_FUNCTION_ARGS)
 					 * errors.
 					 */
 					nulls[9] = true;
-					values[10] = DatumGetInt32(-1);
+					nulls[10] = true;
+					values[11] = DatumGetInt32(-1);
 				}
 				else
 				{
 					/* Unknown address type, should never happen */
 					nulls[9] = true;
 					nulls[10] = true;
+					nulls[11] = true;
 				}
 			}
 		}
@@ -707,6 +716,7 @@ pg_stat_get_activity(PG_FUNCTION_ARGS)
 			nulls[8] = true;
 			nulls[9] = true;
 			nulls[10] = true;
+			nulls[11] = true;
 		}
 
 		tuple = heap_form_tuple(funcctx->tuple_desc, values, nulls);
diff --git a/src/include/catalog/catversion.h b/src/include/catalog/catversion.h
index df3c95b..f1fa958 100644
--- a/src/include/catalog/catversion.h
+++ b/src/include/catalog/catversion.h
@@ -53,6 +53,6 @@
  */
 
 /*							yyyymmddN */
-#define CATALOG_VERSION_NO	201101111
+#define CATALOG_VERSION_NO	201101152
 
 #endif
diff --git a/src/include/catalog/pg_proc.h b/src/include/catalog/pg_proc.h
index f8b5d4d..7543b26 100644
--- a/src/include/catalog/pg_proc.h
+++ b/src/include/catalog/pg_proc.h
@@ -3073,7 +3073,7 @@ DATA(insert OID = 3057 ( pg_stat_get_autoanalyze_count PGNSP PGUID 12 1 0 0 f f
 DESCR("statistics: number of auto analyzes for a table");
 DATA(insert OID = 1936 (  pg_stat_get_backend_idset		PGNSP PGUID 12 1 100 0 f f f t t s 0 0 23 "" _null_ _null_ _null_ _null_ pg_stat_get_backend_idset _null_ _null_ _null_ ));
 DESCR("statistics: currently active backend IDs");
-DATA(insert OID = 2022 (  pg_stat_get_activity			PGNSP PGUID 12 1 100 0 f f f f t s 1 0 2249 "23" "{23,26,23,26,25,25,16,1184,1184,1184,869,23}" "{i,o,o,o,o,o,o,o,o,o,o,o}" "{pid,datid,procpid,usesysid,application_name,current_query,waiting,xact_start,query_start,backend_start,client_addr,client_port}" _null_ pg_stat_get_activity _null_ _null_ _null_ ));
+DATA(insert OID = 2022 (  pg_stat_get_activity			PGNSP PGUID 12 1 100 0 f f f f t s 1 0 2249 "23" "{23,26,23,26,25,25,16,1184,1184,1184,869,25,23}" "{i,o,o,o,o,o,o,o,o,o,o,o,o}" "{pid,datid,procpid,usesysid,application_name,current_query,waiting,xact_start,query_start,backend_start,client_addr,client_hostname,client_port}" _null_ pg_stat_get_activity _null_ _null_ _null_ ));
 DESCR("statistics: information about currently active backends");
 DATA(insert OID = 3099 (  pg_stat_get_wal_senders	PGNSP PGUID 12 1 10 0 f f f f t s 0 0 2249 "" "{23,25,25}" "{o,o,o}" "{procpid,state,sent_location}" _null_ pg_stat_get_wal_senders _null_ _null_ _null_ ));
 DESCR("statistics: information about currently active replication");
diff --git a/src/include/pgstat.h b/src/include/pgstat.h
index 9f4e0ca..7c5de34 100644
--- a/src/include/pgstat.h
+++ b/src/include/pgstat.h
@@ -626,6 +626,7 @@ typedef struct PgBackendStatus
 	Oid			st_databaseid;
 	Oid			st_userid;
 	SockAddr	st_clientaddr;
+	char	   *st_clienthostname; /* MUST be null-terminated */
 
 	/* Is backend currently waiting on an lmgr lock? */
 	bool		st_waiting;
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index 72e5630..57dc679 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -1290,13 +1290,13 @@ SELECT viewname, definition FROM pg_views WHERE schemaname <> 'information_schem
  pg_seclabels                | (((((SELECT l.objoid, l.classoid, l.objsubid, CASE WHEN (rel.relkind = 'r'::"char") THEN 'table'::text WHEN (rel.relkind = 'v'::"char") THEN 'view'::text WHEN (rel.relkind = 'S'::"char") THEN 'sequence'::text WHEN (rel.relkind = 'f'::"char") THEN 'foreign table'::text ELSE NULL::text END AS objtype, rel.relnamespace AS objnamespace, CASE WHEN pg_table_is_visible(rel.oid) THEN quote_ident((rel.relname)::text) ELSE ((quote_ident((nsp.nspname)::text) || '.'::text) || quote_ident((rel.relname)::text)) END AS objname, l.provider, l.label FROM ((pg_seclabel l JOIN pg_class rel ON (((l.classoid = rel.tableoid) AND (l.objoid = rel.oid)))) JOIN pg_namespace nsp ON ((rel.relnamespace = nsp.oid))) WHERE (l.objsubid = 0) UNION ALL SELECT l.objoid, l.classoid, l.objsubid, 'column'::text AS objtype, rel.relnamespace AS objnamespace, ((CASE WHEN pg_table_is_visible(rel.oid) THEN quote_ident((rel.relname)::text) ELSE ((quote_ident((nsp.nspname)::text) || '.'::text) || quote_ident((rel.relname)::text)) END || '.'::text) || (att.attname)::text) AS objname, l.provider, l.label FROM (((pg_seclabel l JOIN pg_class rel ON (((l.classoid = rel.tableoid) AND (l.objoid = rel.oid)))) JOIN pg_attribute att ON (((rel.oid = att.attrelid) AND (l.objsubid = att.attnum)))) JOIN pg_namespace nsp ON ((rel.relnamespace = nsp.oid))) WHERE (l.objsubid <> 0)) UNION ALL SELECT l.objoid, l.classoid, l.objsubid, CASE WHEN (pro.proisagg = true) THEN 'aggregate'::text WHEN (pro.proisagg = false) THEN 'function'::text ELSE NULL::text END AS objtype, pro.pronamespace AS objnamespace, (((CASE WHEN pg_function_is_visible(pro.oid) THEN quote_ident((pro.proname)::text) ELSE ((quote_ident((nsp.nspname)::text) || '.'::text) || quote_ident((pro.proname)::text)) END || '('::text) || pg_get_function_arguments(pro.oid)) || ')'::text) AS objname, l.provider, l.label FROM ((pg_seclabel l JOIN pg_proc pro ON (((l.classoid = pro.tableoid) AND (l.objoid = pro.oid)))) JOIN pg_namespace nsp ON ((pro.pronamespace = nsp.oid))) WHERE (l.objsubid = 0)) UNION ALL SELECT l.objoid, l.classoid, l.objsubid, CASE WHEN (typ.typtype = 'd'::"char") THEN 'domain'::text ELSE 'type'::text END AS objtype, typ.typnamespace AS objnamespace, CASE WHEN pg_type_is_visible(typ.oid) THEN quote_ident((typ.typname)::text) ELSE ((quote_ident((nsp.nspname)::text) || '.'::text) || quote_ident((typ.typname)::text)) END AS objname, l.provider, l.label FROM ((pg_seclabel l JOIN pg_type typ ON (((l.classoid = typ.tableoid) AND (l.objoid = typ.oid)))) JOIN pg_namespace nsp ON ((typ.typnamespace = nsp.oid))) WHERE (l.objsubid = 0)) UNION ALL SELECT l.objoid, l.classoid, l.objsubid, 'large object'::text AS objtype, NULL::oid AS objnamespace, (l.objoid)::text AS objname, l.provider, l.label FROM (pg_seclabel l JOIN pg_largeobject_metadata lom ON ((l.objoid = lom.oid))) WHERE ((l.classoid = ('pg_largeobject'::regclass)::oid) AND (l.objsubid = 0))) UNION ALL SELECT l.objoid, l.classoid, l.objsubid, 'language'::text AS objtype, NULL::oid AS objnamespace, quote_ident((lan.lanname)::text) AS objname, l.provider, l.label FROM (pg_seclabel l JOIN pg_language lan ON (((l.classoid = lan.tableoid) AND (l.objoid = lan.oid)))) WHERE (l.objsubid = 0)) UNION ALL SELECT l.objoid, l.classoid, l.objsubid, 'schema'::text AS objtype, nsp.oid AS objnamespace, quote_ident((nsp.nspname)::text) AS objname, l.provider, l.label FROM (pg_seclabel l JOIN pg_namespace nsp ON (((l.classoid = nsp.tableoid) AND (l.objoid = nsp.oid)))) WHERE (l.objsubid = 0);
  pg_settings                 | SELECT a.name, a.setting, a.unit, a.category, a.short_desc, a.extra_desc, a.context, a.vartype, a.source, a.min_val, a.max_val, a.enumvals, a.boot_val, a.reset_val, a.sourcefile, a.sourceline FROM pg_show_all_settings() a(name, setting, unit, category, short_desc, extra_desc, context, vartype, source, min_val, max_val, enumvals, boot_val, reset_val, sourcefile, sourceline);
  pg_shadow                   | SELECT pg_authid.rolname AS usename, pg_authid.oid AS usesysid, pg_authid.rolcreatedb AS usecreatedb, pg_authid.rolsuper AS usesuper, pg_authid.rolcatupdate AS usecatupd, pg_authid.rolreplication AS userepl, pg_authid.rolpassword AS passwd, (pg_authid.rolvaliduntil)::abstime AS valuntil, s.setconfig AS useconfig FROM (pg_authid LEFT JOIN pg_db_role_setting s ON (((pg_authid.oid = s.setrole) AND (s.setdatabase = (0)::oid)))) WHERE pg_authid.rolcanlogin;
- pg_stat_activity            | SELECT s.datid, d.datname, s.procpid, s.usesysid, u.rolname AS usename, s.application_name, s.client_addr, s.client_port, s.backend_start, s.xact_start, s.query_start, s.waiting, s.current_query FROM pg_database d, pg_stat_get_activity(NULL::integer) s(datid, procpid, usesysid, application_name, current_query, waiting, xact_start, query_start, backend_start, client_addr, client_port), pg_authid u WHERE ((s.datid = d.oid) AND (s.usesysid = u.oid));
+ pg_stat_activity            | SELECT s.datid, d.datname, s.procpid, s.usesysid, u.rolname AS usename, s.application_name, s.client_addr, s.client_hostname, s.client_port, s.backend_start, s.xact_start, s.query_start, s.waiting, s.current_query FROM pg_database d, pg_stat_get_activity(NULL::integer) s(datid, procpid, usesysid, application_name, current_query, waiting, xact_start, query_start, backend_start, client_addr, client_hostname, client_port), pg_authid u WHERE ((s.datid = d.oid) AND (s.usesysid = u.oid));
  pg_stat_all_indexes         | SELECT c.oid AS relid, i.oid AS indexrelid, n.nspname AS schemaname, c.relname, i.relname AS indexrelname, pg_stat_get_numscans(i.oid) AS idx_scan, pg_stat_get_tuples_returned(i.oid) AS idx_tup_read, pg_stat_get_tuples_fetched(i.oid) AS idx_tup_fetch FROM (((pg_class c JOIN pg_index x ON ((c.oid = x.indrelid))) JOIN pg_class i ON ((i.oid = x.indexrelid))) LEFT JOIN pg_namespace n ON ((n.oid = c.relnamespace))) WHERE (c.relkind = ANY (ARRAY['r'::"char", 't'::"char"]));
  pg_stat_all_tables          | SELECT c.oid AS relid, n.nspname AS schemaname, c.relname, pg_stat_get_numscans(c.oid) AS seq_scan, pg_stat_get_tuples_returned(c.oid) AS seq_tup_read, (sum(pg_stat_get_numscans(i.indexrelid)))::bigint AS idx_scan, ((sum(pg_stat_get_tuples_fetched(i.indexrelid)))::bigint + pg_stat_get_tuples_fetched(c.oid)) AS idx_tup_fetch, pg_stat_get_tuples_inserted(c.oid) AS n_tup_ins, pg_stat_get_tuples_updated(c.oid) AS n_tup_upd, pg_stat_get_tuples_deleted(c.oid) AS n_tup_del, pg_stat_get_tuples_hot_updated(c.oid) AS n_tup_hot_upd, pg_stat_get_live_tuples(c.oid) AS n_live_tup, pg_stat_get_dead_tuples(c.oid) AS n_dead_tup, pg_stat_get_last_vacuum_time(c.oid) AS last_vacuum, pg_stat_get_last_autovacuum_time(c.oid) AS last_autovacuum, pg_stat_get_last_analyze_time(c.oid) AS last_analyze, pg_stat_get_last_autoanalyze_time(c.oid) AS last_autoanalyze, pg_stat_get_vacuum_count(c.oid) AS vacuum_count, pg_stat_get_autovacuum_count(c.oid) AS autovacuum_count, pg_stat_get_analyze_count(c.oid) AS analyze_count, pg_stat_get_autoanalyze_count(c.oid) AS autoanalyze_count FROM ((pg_class c LEFT JOIN pg_index i ON ((c.oid = i.indrelid))) LEFT JOIN pg_namespace n ON ((n.oid = c.relnamespace))) WHERE (c.relkind = ANY (ARRAY['r'::"char", 't'::"char"])) GROUP BY c.oid, n.nspname, c.relname;
  pg_stat_bgwriter            | SELECT pg_stat_get_bgwriter_timed_checkpoints() AS checkpoints_timed, pg_stat_get_bgwriter_requested_checkpoints() AS checkpoints_req, pg_stat_get_bgwriter_buf_written_checkpoints() AS buffers_checkpoint, pg_stat_get_bgwriter_buf_written_clean() AS buffers_clean, pg_stat_get_bgwriter_maxwritten_clean() AS maxwritten_clean, pg_stat_get_buf_written_backend() AS buffers_backend, pg_stat_get_buf_fsync_backend() AS buffers_backend_fsync, pg_stat_get_buf_alloc() AS buffers_alloc;
  pg_stat_database            | SELECT d.oid AS datid, d.datname, pg_stat_get_db_numbackends(d.oid) AS numbackends, pg_stat_get_db_xact_commit(d.oid) AS xact_commit, pg_stat_get_db_xact_rollback(d.oid) AS xact_rollback, (pg_stat_get_db_blocks_fetched(d.oid) - pg_stat_get_db_blocks_hit(d.oid)) AS blks_read, pg_stat_get_db_blocks_hit(d.oid) AS blks_hit, pg_stat_get_db_tuples_returned(d.oid) AS tup_returned, pg_stat_get_db_tuples_fetched(d.oid) AS tup_fetched, pg_stat_get_db_tuples_inserted(d.oid) AS tup_inserted, pg_stat_get_db_tuples_updated(d.oid) AS tup_updated, pg_stat_get_db_tuples_deleted(d.oid) AS tup_deleted, pg_stat_get_db_conflict_all(d.oid) AS conflicts FROM pg_database d;
  pg_stat_database_conflicts  | SELECT d.oid AS datid, d.datname, pg_stat_get_db_conflict_tablespace(d.oid) AS confl_tablespace, pg_stat_get_db_conflict_lock(d.oid) AS confl_lock, pg_stat_get_db_conflict_snapshot(d.oid) AS confl_snapshot, pg_stat_get_db_conflict_bufferpin(d.oid) AS confl_bufferpin, pg_stat_get_db_conflict_startup_deadlock(d.oid) AS confl_deadlock FROM pg_database d;
- pg_stat_replication         | SELECT s.procpid, s.usesysid, u.rolname AS usename, s.application_name, s.client_addr, s.client_port, s.backend_start, w.state, w.sent_location FROM pg_stat_get_activity(NULL::integer) s(datid, procpid, usesysid, application_name, current_query, waiting, xact_start, query_start, backend_start, client_addr, client_port), pg_authid u, pg_stat_get_wal_senders() w(procpid, state, sent_location) WHERE ((s.usesysid = u.oid) AND (s.procpid = w.procpid));
+ pg_stat_replication         | SELECT s.procpid, s.usesysid, u.rolname AS usename, s.application_name, s.client_addr, s.client_hostname, s.client_port, s.backend_start, w.state, w.sent_location FROM pg_stat_get_activity(NULL::integer) s(datid, procpid, usesysid, application_name, current_query, waiting, xact_start, query_start, backend_start, client_addr, client_hostname, client_port), pg_authid u, pg_stat_get_wal_senders() w(procpid, state, sent_location) WHERE ((s.usesysid = u.oid) AND (s.procpid = w.procpid));
  pg_stat_sys_indexes         | SELECT pg_stat_all_indexes.relid, pg_stat_all_indexes.indexrelid, pg_stat_all_indexes.schemaname, pg_stat_all_indexes.relname, pg_stat_all_indexes.indexrelname, pg_stat_all_indexes.idx_scan, pg_stat_all_indexes.idx_tup_read, pg_stat_all_indexes.idx_tup_fetch FROM pg_stat_all_indexes WHERE ((pg_stat_all_indexes.schemaname = ANY (ARRAY['pg_catalog'::name, 'information_schema'::name])) OR (pg_stat_all_indexes.schemaname ~ '^pg_toast'::text));
  pg_stat_sys_tables          | SELECT pg_stat_all_tables.relid, pg_stat_all_tables.schemaname, pg_stat_all_tables.relname, pg_stat_all_tables.seq_scan, pg_stat_all_tables.seq_tup_read, pg_stat_all_tables.idx_scan, pg_stat_all_tables.idx_tup_fetch, pg_stat_all_tables.n_tup_ins, pg_stat_all_tables.n_tup_upd, pg_stat_all_tables.n_tup_del, pg_stat_all_tables.n_tup_hot_upd, pg_stat_all_tables.n_live_tup, pg_stat_all_tables.n_dead_tup, pg_stat_all_tables.last_vacuum, pg_stat_all_tables.last_autovacuum, pg_stat_all_tables.last_analyze, pg_stat_all_tables.last_autoanalyze, pg_stat_all_tables.vacuum_count, pg_stat_all_tables.autovacuum_count, pg_stat_all_tables.analyze_count, pg_stat_all_tables.autoanalyze_count FROM pg_stat_all_tables WHERE ((pg_stat_all_tables.schemaname = ANY (ARRAY['pg_catalog'::name, 'information_schema'::name])) OR (pg_stat_all_tables.schemaname ~ '^pg_toast'::text));
  pg_stat_user_functions      | SELECT p.oid AS funcid, n.nspname AS schemaname, p.proname AS funcname, pg_stat_get_function_calls(p.oid) AS calls, (pg_stat_get_function_time(p.oid) / 1000) AS total_time, (pg_stat_get_function_self_time(p.oid) / 1000) AS self_time FROM (pg_proc p LEFT JOIN pg_namespace n ON ((n.oid = p.pronamespace))) WHERE ((p.prolang <> (12)::oid) AND (pg_stat_get_function_calls(p.oid) IS NOT NULL));
#4Alvaro Herrera
alvherre@commandprompt.com
In reply to: Peter Eisentraut (#3)
Re: log_hostname and pg_stat_activity

Excerpts from Peter Eisentraut's message of sáb ene 15 13:15:45 -0300 2011:

Here is a patch that adds a client_hostname field to pg_stat_activity.
It takes the hostname if it is available either by having log_hostname
set or if the pg_hba.conf processing resolved it. So I think for most
people who would care about this thing, it would "just work".

I'm not so sure about the pgstat.c internals. Do we need the separate
buffer in shared memory, as in this patch, or could we just copy the
name directly into the PgBackendStatus struct? I guess not the latter,
since my compiler complains about copying a string into a volatile
pointer.

I think you should do it like clientaddr is handled: fill a string with
the value from MyProcPort, then make the PgBackendStatus point to that.

BTW you need to touch BackendStatusShmemSize if you change the routine
below that (but AFAIU that should be taken out).

...

I'm confused ... why isn't this code broken? I don't understand why
isn't clientaddr clobbered the next time someone uses the stack space,
given that it's not allocated anywhere.

--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

#5Steve Singer
ssinger_pg@sympatico.ca
In reply to: Peter Eisentraut (#3)
Re: log_hostname and pg_stat_activity

Here is my review for this patch

Submission Review
----------------
-Patch applies cleanly

-Patch does not include documentation changes. At a minimum: update the
table that lists what pg_stat_activity and pg_stat_replication includes
in monitoring.sgml but I propose more below.

-No tests are included but writing unit tests that depend on produce the
same output involving the hostname of the client is not possible.

Usability review
----------------

See my comments below in the testing section. The patch does do what it
says but the log_hostname issue is a usability issue (it not being
obvious why you get only null owhen log_hostname is turned off).
Documenting it might be fine. If log_hostname were new to 9.1 I would
encourage renaming it to something that implies it does more than just
control logging output but I don't see this as a good enough reason to
rename a parameter.

I think being able to see the hostnames connections in pg_stat_activity
come from is useful and it is a feature we don't already have.

Feature Test
----------------

If my connection is authorized through a line in pg_hba that uses
client_hostname then the column shows what I expect even with
log_hostname set to off.

However if I connect with a line in pg_hba that matches on an IP network
then my client_hostname is always null unless log_hostname is set to
true. This is consistent with the behavior you describe but I think the
average user will find it a bit confusing. Having a column that is
always null unless a GUC is set is less than ideal but I understand why
log_hostname isn't on by default.

I would be inclined to add an entry for these views in catalogs.sgml
that where we can then give users a pointer that they need to set
log_hostname to get anything out of this column.

If I connect through unix sockets (say psql -h /tmp --port 5433)
I was also expecting to see "[local]" in client_hostname but I am just
getting NULL. This this lookup is static I don't see why it should be
dependent on log_hostname (even with log_hostname set I'm not seeing
[local])

I have not tested this with ipv6

Performance Review
------------------
The lookup is done when the connection is established not each time the
view is queried. I don't see any performance issues

Coding Review
-------------

As Alvaro pointed out BackendStatusShmemSize should be updated.

To answer his question about why clientaddr works: clientaddr is a
SockAddr which is a structure not a pointer so the data gets copied by
value to beentry. That won't work for strings, I have no issues with
how your allocating space in beentry and then strlcpy'ing the values.

I see no issues with the implementation

I'm marking this as 'Waiting for Author' pending documentation changes
and maybe a fix the behaviour with unix sockets

Steve

#6Alvaro Herrera
alvherre@commandprompt.com
In reply to: Steve Singer (#5)
Re: log_hostname and pg_stat_activity

Excerpts from Steve Singer's message of mar ene 18 21:24:25 -0300 2011:

Coding Review
-------------

As Alvaro pointed out BackendStatusShmemSize should be updated.

To answer his question about why clientaddr works: clientaddr is a
SockAddr which is a structure not a pointer so the data gets copied by
value to beentry. That won't work for strings, I have no issues with
how your allocating space in beentry and then strlcpy'ing the values.

Doh, of course. Thanks.

--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

#7Robert Haas
robertmhaas@gmail.com
In reply to: Steve Singer (#5)
Re: log_hostname and pg_stat_activity

On Tue, Jan 18, 2011 at 7:24 PM, Steve Singer <ssinger_pg@sympatico.ca> wrote:

If my connection is authorized through a line in pg_hba that uses
client_hostname then the column shows what I expect even with log_hostname
set to off.

However if I connect with a line in pg_hba that matches on an IP network
then my client_hostname is always null unless log_hostname is set to true.
 This is consistent with the behavior you describe but I think the average
user will find it a bit confusing.

I agree. I'm not sure there's enough value to this feature to warrant
the amount of user confusion this is likely to produce.

But if we're going to do it anyway we at least need to do this:

I would be inclined to add an entry for these views in catalogs.sgml that
where we can then give users a pointer that they need to set log_hostname to
get anything out of this column.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

#8Peter Eisentraut
peter_e@gmx.net
In reply to: Robert Haas (#7)
Re: log_hostname and pg_stat_activity

On sön, 2011-01-30 at 15:03 -0500, Robert Haas wrote:

On Tue, Jan 18, 2011 at 7:24 PM, Steve Singer
<ssinger_pg@sympatico.ca> wrote:

If my connection is authorized through a line in pg_hba that uses
client_hostname then the column shows what I expect even with

log_hostname

set to off.

However if I connect with a line in pg_hba that matches on an IP

network

then my client_hostname is always null unless log_hostname is set to

true.

This is consistent with the behavior you describe but I think the

average

user will find it a bit confusing.

I agree. I'm not sure there's enough value to this feature to warrant
the amount of user confusion this is likely to produce.

What alternative behavior would you suggest?

#9Robert Haas
robertmhaas@gmail.com
In reply to: Peter Eisentraut (#8)
Re: log_hostname and pg_stat_activity

On Sun, Jan 30, 2011 at 4:30 PM, Peter Eisentraut <peter_e@gmx.net> wrote:

On sön, 2011-01-30 at 15:03 -0500, Robert Haas wrote:

On Tue, Jan 18, 2011 at 7:24 PM, Steve Singer
<ssinger_pg@sympatico.ca> wrote:

If my connection is authorized through a line in pg_hba that uses
client_hostname then the column shows what I expect even with

log_hostname

set to off.

However if I connect with a line in pg_hba that matches on an IP

network

then my client_hostname is always null unless log_hostname is set to

true.

 This is consistent with the behavior you describe but I think the

average

user will find it a bit confusing.

I agree.  I'm not sure there's enough value to this feature to warrant
the amount of user confusion this is likely to produce.

What alternative behavior would you suggest?

I don't know. As I said in my previous email, I'd either (a) forget
the whole thing or (b) make sure that the documentation is *extremely*
explicit about what the behavior is.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

#10Peter Eisentraut
peter_e@gmx.net
In reply to: Steve Singer (#5)
Re: log_hostname and pg_stat_activity

On tis, 2011-01-18 at 19:24 -0500, Steve Singer wrote:

However if I connect with a line in pg_hba that matches on an IP
network then my client_hostname is always null unless log_hostname is
set to true. This is consistent with the behavior you describe but I
think the average user will find it a bit confusing. Having a column
that is always null unless a GUC is set is less than ideal but I
understand why log_hostname isn't on by default.

Well, we have all these track_* variables, which also control what
appears in the statistics views.

After thinking about this some more, I think it might be better to be
less cute and forget about the interaction with the pg_hba.conf hostname
behavior. That is, the host name is set if and only if log_hostname is
on. Otherwise you will for example have an inconsistency between the
statistics views and the server log, unless you want to argue that we
can override the log_hostname setting based on what happens in
pg_hba.conf. That's just getting too weird.

#11Robert Haas
robertmhaas@gmail.com
In reply to: Peter Eisentraut (#10)
Re: log_hostname and pg_stat_activity

On Tue, Feb 1, 2011 at 1:09 PM, Peter Eisentraut <peter_e@gmx.net> wrote:

On tis, 2011-01-18 at 19:24 -0500, Steve Singer wrote:

However if I connect with a line in pg_hba that matches on an IP
network then my client_hostname is always null unless log_hostname is
set to true.  This is consistent with the behavior you describe but I
think the average user will find it a bit confusing.  Having a column
that is always null unless a GUC is set is less than ideal but I
understand why log_hostname isn't on by default.

Well, we have all these track_* variables, which also control what
appears in the statistics views.

After thinking about this some more, I think it might be better to be
less cute and forget about the interaction with the pg_hba.conf hostname
behavior.  That is, the host name is set if and only if log_hostname is
on.

+1 for doing it that way.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

#12Robert Haas
robertmhaas@gmail.com
In reply to: Robert Haas (#11)
Re: log_hostname and pg_stat_activity

On Tue, Feb 1, 2011 at 1:33 PM, Robert Haas <robertmhaas@gmail.com> wrote:

On Tue, Feb 1, 2011 at 1:09 PM, Peter Eisentraut <peter_e@gmx.net> wrote:

On tis, 2011-01-18 at 19:24 -0500, Steve Singer wrote:

However if I connect with a line in pg_hba that matches on an IP
network then my client_hostname is always null unless log_hostname is
set to true.  This is consistent with the behavior you describe but I
think the average user will find it a bit confusing.  Having a column
that is always null unless a GUC is set is less than ideal but I
understand why log_hostname isn't on by default.

Well, we have all these track_* variables, which also control what
appears in the statistics views.

After thinking about this some more, I think it might be better to be
less cute and forget about the interaction with the pg_hba.conf hostname
behavior.  That is, the host name is set if and only if log_hostname is
on.

+1 for doing it that way.

I think there are no outstanding issues with this patch of any
significance, so I'm marking it Ready for Committer.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

#13Steve Singer
ssinger_pg@sympatico.ca
In reply to: Robert Haas (#12)
Re: log_hostname and pg_stat_activity

On 11-02-10 10:13 AM, Robert Haas wrote:

On Tue, Feb 1, 2011 at 1:33 PM, Robert Haas<robertmhaas@gmail.com> wrote:

On Tue, Feb 1, 2011 at 1:09 PM, Peter Eisentraut<peter_e@gmx.net> wrote:

On tis, 2011-01-18 at 19:24 -0500, Steve Singer wrote:

However if I connect with a line in pg_hba that matches on an IP
network then my client_hostname is always null unless log_hostname is
set to true. This is consistent with the behavior you describe but I
think the average user will find it a bit confusing. Having a column
that is always null unless a GUC is set is less than ideal but I
understand why log_hostname isn't on by default.

Well, we have all these track_* variables, which also control what
appears in the statistics views.

After thinking about this some more, I think it might be better to be
less cute and forget about the interaction with the pg_hba.conf hostname
behavior. That is, the host name is set if and only if log_hostname is
on.

+1 for doing it that way.

I think there are no outstanding issues with this patch of any
significance, so I'm marking it Ready for Committer.

Was there an uodated version of this patch I missed?

The original patch needed some sort of documentation saying that having
something showup in the new pg_stat_activity columns is controlled by
log_hostname.

Above Peter and you seem to agree that having the having the line
matched in pg_hba being a controlling factor should be removed but I
haven't seen an updated patch that implements that.

#14Robert Haas
robertmhaas@gmail.com
In reply to: Steve Singer (#13)
Re: log_hostname and pg_stat_activity

On Thu, Feb 10, 2011 at 10:22 AM, Steve Singer <ssinger_pg@sympatico.ca> wrote:

On 11-02-10 10:13 AM, Robert Haas wrote:

On Tue, Feb 1, 2011 at 1:33 PM, Robert Haas<robertmhaas@gmail.com>  wrote:

On Tue, Feb 1, 2011 at 1:09 PM, Peter Eisentraut<peter_e@gmx.net>  wrote:

On tis, 2011-01-18 at 19:24 -0500, Steve Singer wrote:

However if I connect with a line in pg_hba that matches on an IP
network then my client_hostname is always null unless log_hostname is
set to true.  This is consistent with the behavior you describe but I
think the average user will find it a bit confusing.  Having a column
that is always null unless a GUC is set is less than ideal but I
understand why log_hostname isn't on by default.

Well, we have all these track_* variables, which also control what
appears in the statistics views.

After thinking about this some more, I think it might be better to be
less cute and forget about the interaction with the pg_hba.conf hostname
behavior.  That is, the host name is set if and only if log_hostname is
on.

+1 for doing it that way.

I think there are no outstanding issues with this patch of any
significance, so I'm marking it Ready for Committer.

Was there an uodated version of this patch I missed?

The original patch needed some sort of documentation saying that having
something showup in the new pg_stat_activity columns is controlled by
log_hostname.

Above Peter and you seem to agree that having the having the line matched in
pg_hba being a controlling factor should be removed but I haven't seen an
updated patch that implements that.

I was assuming those changes were sufficiently trivial that they could
be made at commit-time, especially if Peter is committing it himself.
Of course if he'd like a re-review, he can always post an updated
patch, but I just thought that was overly pedantic in this particular
case.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

#15Steve Singer
ssinger_pg@sympatico.ca
In reply to: Robert Haas (#14)
Re: log_hostname and pg_stat_activity

On 11-02-10 10:32 AM, Robert Haas wrote:

I was assuming those changes were sufficiently trivial that they could
be made at commit-time, especially if Peter is committing it himself.
Of course if he'd like a re-review, he can always post an updated
patch, but I just thought that was overly pedantic in this particular
case.

Sounds reasonable.

#16Robert Haas
robertmhaas@gmail.com
In reply to: Steve Singer (#15)
Re: log_hostname and pg_stat_activity

On Thu, Feb 10, 2011 at 10:40 AM, Steve Singer <ssinger_pg@sympatico.ca> wrote:

On 11-02-10 10:32 AM, Robert Haas wrote:

I was assuming those changes were sufficiently trivial that they could
be made at commit-time, especially if Peter is committing it himself.
Of course if he'd like a re-review, he can always post an updated
patch, but I just thought that was overly pedantic in this particular
case.

Sounds reasonable.

I rebased this patch, wrote documentation, and fixed
BackendStatusShmemSize. PFA.

On further review, I'm inclined to go with Peter's original approach
to displaying the hostname: show it if we have it, and don't if we
don't. It's not that hard to document the relevant criteria, and it
seems silly to suppress the information if we have it.

I had a thought of declaring st_clienthostname as char
st_clienthostname[NAMEDATALEN] rather than char *st_clienthostname.
That would simplify the initialization code. But I believe that the
protocol used for updating this data structure is unsafe unless the
whole thing fits into a single cache line. I'm not positive that's
going to be true on every architecture even as things stand. On my
Mac, with this patch, it's 208 bytes (which means that it's presumably
200 without the patch, and that it would be 264 with the alternate
approach proposed above). According to that font of knowledge,
Wikipedia, the size of a cache line can vary from 8 to 512 bytes
[citation needed].

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company