pgstat SRF?

Started by Magnus Haganderover 17 years ago6 messages
#1Magnus Hagander
magnus@hagander.net

While looking over the statistics-for-functions patch
(http://archives.postgresql.org/pgsql-patches/2008-03/msg00300.php), I
came back to a thought I've had before - why do we keep one function
per column for pgstat functions, instead of using a set returning
function? Is there some actual reason for this, or is it just legacy
from a time when it was (much) harder to write SRFs?

If there's no actual reason, I think it would be a good idea to make at
least new views added based on SRFs instead....

//Magnus

#2Tom Lane
tgl@sss.pgh.pa.us
In reply to: Magnus Hagander (#1)
Re: pgstat SRF?

Magnus Hagander <magnus@hagander.net> writes:

While looking over the statistics-for-functions patch
(http://archives.postgresql.org/pgsql-patches/2008-03/msg00300.php), I
came back to a thought I've had before - why do we keep one function
per column for pgstat functions, instead of using a set returning
function? Is there some actual reason for this, or is it just legacy
from a time when it was (much) harder to write SRFs?

I think it's so that you can build your own stats views instead of being
compelled to select the data someone thought was good for you.

regards, tom lane

#3Magnus Hagander
magnus@hagander.net
In reply to: Tom Lane (#2)
Re: pgstat SRF?

Tom Lane wrote:

Magnus Hagander <magnus@hagander.net> writes:

While looking over the statistics-for-functions patch
(http://archives.postgresql.org/pgsql-patches/2008-03/msg00300.php),
I came back to a thought I've had before - why do we keep one
function per column for pgstat functions, instead of using a set
returning function? Is there some actual reason for this, or is it
just legacy from a time when it was (much) harder to write SRFs?

I think it's so that you can build your own stats views instead of
being compelled to select the data someone thought was good for you.

You can still do that if it's an SRF. You could even make the SRF take
an optional argument to either return a single value (filtered the same
way the individual functions are) or when this one is set to NULL,
return the whole table.

It would make the overhead a lot lower in the most common case ("SELECT
* FROM pg_stat_<somethingorother>"), while only adding a little in the
other cases, I think.

Though I'm not sure that overhead is big enough to care about in the
first place, but if you're VIEWs are longish it could be...

//Magnus

#4Decibel!
decibel@decibel.org
In reply to: Magnus Hagander (#1)
1 attachment(s)
Re: pgstat SRF?

On Apr 21, 2008, at 8:34 AM, Magnus Hagander wrote:

While looking over the statistics-for-functions patch
(http://archives.postgresql.org/pgsql-patches/2008-03/msg00300.php), I
came back to a thought I've had before - why do we keep one function
per column for pgstat functions, instead of using a set returning
function? Is there some actual reason for this, or is it just legacy
from a time when it was (much) harder to write SRFs?

If there's no actual reason, I think it would be a good idea to
make at
least new views added based on SRFs instead....

+1. I could probably use this for pgstats at some point.
-- 
Decibel!, aka Jim C. Nasby, Database Architect  decibel@decibel.org
Give your computer some brain candy! www.distributed.net Team #1828

Attachments:

smime.p7sapplication/pkcs7-signature; name=smime.p7sDownload
#5Magnus Hagander
magnus@hagander.net
In reply to: Magnus Hagander (#3)
1 attachment(s)
Re: pgstat SRF?

Magnus Hagander wrote:

Tom Lane wrote:

Magnus Hagander <magnus@hagander.net> writes:

While looking over the statistics-for-functions patch
(http://archives.postgresql.org/pgsql-patches/2008-03/msg00300.php),
I came back to a thought I've had before - why do we keep one
function per column for pgstat functions, instead of using a set
returning function? Is there some actual reason for this, or is it
just legacy from a time when it was (much) harder to write SRFs?

I think it's so that you can build your own stats views instead of
being compelled to select the data someone thought was good for you.

You can still do that if it's an SRF. You could even make the SRF take
an optional argument to either return a single value (filtered the
same way the individual functions are) or when this one is set to
NULL, return the whole table.

It would make the overhead a lot lower in the most common case
("SELECT
* FROM pg_stat_<somethingorother>"), while only adding a little in the
other cases, I think.

Though I'm not sure that overhead is big enough to care about in the
first place, but if you're VIEWs are longish it could be...

Actually, looking at this once more, the interface to the functions
sucked more than I thought. They're not actually accepting procpid as
parameters, but just an index into the current array in pgstats..
Basically, they're not supposed to be used in any other way than
accessing all the rows at once :-)

Attached is a version of the functions required for pg_stat_activity
implemented as a SRF instead of different functions. A quick benchmark
(grabbing the VIEW 10,000 times on a system with about 500 active
backends) shows it's about 20% faster than the function-per-value
approach, but the runtime per view is still very quick as it is today.
(And most of what overhead there is most likely comes from reading the
stats file)

However, it also implements the lookup-by-PID functionality that IMHO
makes a lot more sense than lookup-by-backend-array-index. This is
obviously a lot more performant than querying the VIEW for all rows -
something that might be a big win for monitoring apps that look for
info about a single backend.

Unsure if we want to go ahead and convert all functions, but I think we
can make a good argument for making *new* stats views (like the ones
about functions that in the queue) based on SRFs instead. It also has
the nice side-effect of less rows in the system tables ;)

Comments?

//Magnus

Attachments:

pgstat.difftext/x-patch; name=pgstat.diffDownload
Index: src/backend/utils/adt/pgstatfuncs.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/utils/adt/pgstatfuncs.c,v
retrieving revision 1.49
diff -c -r1.49 pgstatfuncs.c
*** src/backend/utils/adt/pgstatfuncs.c	25 Mar 2008 22:42:44 -0000	1.49
--- src/backend/utils/adt/pgstatfuncs.c	28 Apr 2008 13:36:57 -0000
***************
*** 17,22 ****
--- 17,24 ----
  #include "funcapi.h"
  #include "miscadmin.h"
  #include "pgstat.h"
+ #include "catalog/pg_type.h"
+ #include "access/heapam.h"
  #include "utils/builtins.h"
  #include "utils/inet.h"
  #include "libpq/ip.h"
***************
*** 39,44 ****
--- 41,47 ----
  extern Datum pg_stat_get_last_autoanalyze_time(PG_FUNCTION_ARGS);
  
  extern Datum pg_stat_get_backend_idset(PG_FUNCTION_ARGS);
+ extern Datum pg_stat_get_activity(PG_FUNCTION_ARGS);
  extern Datum pg_backend_pid(PG_FUNCTION_ARGS);
  extern Datum pg_stat_get_backend_pid(PG_FUNCTION_ARGS);
  extern Datum pg_stat_get_backend_dbid(PG_FUNCTION_ARGS);
***************
*** 363,368 ****
--- 366,590 ----
  	}
  }
  
+ Datum
+ pg_stat_get_activity(PG_FUNCTION_ARGS)
+ {
+ 	FuncCallContext *funcctx;
+ 
+ 	if (SRF_IS_FIRSTCALL())
+ 	{
+ 		MemoryContext oldcontext;
+ 		TupleDesc tupdesc;
+ 		
+ 		funcctx = SRF_FIRSTCALL_INIT();
+ 		
+ 		oldcontext = MemoryContextSwitchTo(funcctx->multi_call_memory_ctx);
+ 
+ 		tupdesc = CreateTemplateTupleDesc(10, 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);
+ 		TupleDescInitEntry(tupdesc, (AttrNumber) 4, "current_query", TEXTOID, -1, 0);
+ 		TupleDescInitEntry(tupdesc, (AttrNumber) 5, "waiting", BOOLOID, -1, 0);
+ 		TupleDescInitEntry(tupdesc, (AttrNumber) 6, "act_start", TIMESTAMPTZOID, -1, 0);
+ 		TupleDescInitEntry(tupdesc, (AttrNumber) 7, "query_start", TIMESTAMPTZOID, -1, 0);
+ 		TupleDescInitEntry(tupdesc, (AttrNumber) 8, "backend_start", TIMESTAMPTZOID, -1, 0);
+ 		TupleDescInitEntry(tupdesc, (AttrNumber) 9, "client_addr", INETOID, -1, 0);
+ 		TupleDescInitEntry(tupdesc, (AttrNumber) 10, "client_port", INT4OID, -1, 0);
+ 
+ 		funcctx->tuple_desc = BlessTupleDesc(tupdesc);
+ 
+ 		funcctx->user_fctx = palloc0(sizeof(int));
+ 		if (PG_ARGISNULL(0))
+ 		{
+ 			/* Get all backends */
+ 			funcctx->max_calls = pgstat_fetch_stat_numbackends();
+ 		}
+ 		else
+ 		{
+ 			/* 
+ 			 * Get one backend - locate by pid.
+ 			 *
+ 			 * We lookup the backend early, so we can return zero rows if it doesn't
+ 			 * exist, instead of returning a single row full of NULLs.
+ 			 */
+ 			int		pid = PG_GETARG_INT32(0);
+ 			int		i;
+ 			int		n = pgstat_fetch_stat_numbackends();
+ 			
+ 			for (i = 1; i <= n; i++)
+ 			{
+ 				PgBackendStatus *be = pgstat_fetch_stat_beentry(i);
+ 				if (be)
+ 				{
+ 					if (be->st_procpid == pid)
+ 					{
+ 						*(int *)(funcctx->user_fctx) = i;
+ 						break;
+ 					}
+ 				}
+ 			}
+ 
+ 			if (*(int *)(funcctx->user_fctx) == 0)
+ 				/* Pid not found, return zero rows */
+ 				funcctx->max_calls = 0;
+ 			else
+ 				funcctx->max_calls = 1;
+ 		}
+ 		
+ 		MemoryContextSwitchTo(oldcontext);
+ 	}
+ 
+ 	/* stuff done on every call of the function */
+ 	funcctx = SRF_PERCALL_SETUP();
+ 
+ 	if (funcctx->call_cntr < funcctx->max_calls)
+ 	{
+ 		/* for each row */
+ 		Datum			values[10];
+ 		bool			nulls[10];
+ 		HeapTuple		tuple;
+ 		PgBackendStatus *beentry;
+ 		SockAddr		zero_clientaddr;
+ 
+ 		MemSet(values, 0, sizeof(values));
+ 		MemSet(nulls, 0, sizeof(nulls));
+ 		
+ 		if (*(int *)(funcctx->user_fctx) > 0)
+ 			/* Get specific pid slot */
+ 			beentry = pgstat_fetch_stat_beentry(*(int *)(funcctx->user_fctx));
+ 		else
+ 			/* Get the next one in the list */
+ 			beentry = pgstat_fetch_stat_beentry(funcctx->call_cntr+1); /* 1-based index */
+ 		if (!beentry)
+ 		{
+ 			int i;
+ 
+ 			for (i = 0; i < sizeof(nulls)/sizeof(nulls[0]); i++)
+ 				nulls[i] = true;
+ 
+ 			nulls[3] = false;
+ 			values[3] = CStringGetTextDatum("<backend information not available>");
+ 
+ 			tuple = heap_form_tuple(funcctx->tuple_desc, values, nulls);
+ 			SRF_RETURN_NEXT(funcctx, HeapTupleGetDatum(tuple));
+ 		}
+ 
+ 		/* Values available to all callers */
+ 		values[0] = ObjectIdGetDatum(beentry->st_databaseid);
+ 		values[1] = Int32GetDatum(beentry->st_procpid);
+ 		values[2] = ObjectIdGetDatum(beentry->st_userid);
+ 
+ 		/* Values only available to same user or superuser */
+ 		if (superuser() || beentry->st_userid == GetUserId())
+ 		{
+ 			if (*(beentry->st_activity) == '\0')
+ 			{
+ 				values[3] = CStringGetTextDatum("<command string not enabled>");
+ 			}
+ 			else
+ 			{
+ 				values[3] = CStringGetTextDatum(beentry->st_activity);
+ 			}
+ 
+ 			values[4] = BoolGetDatum(beentry->st_waiting);
+ 
+ 			if (beentry->st_xact_start_timestamp != 0)
+ 				values[5] = TimestampTzGetDatum(beentry->st_xact_start_timestamp);
+ 			else
+ 				nulls[5] = true;
+ 
+ 			if (beentry->st_activity_start_timestamp != 0)
+ 				values[6] = TimestampTzGetDatum(beentry->st_activity_start_timestamp);
+ 			else
+ 				nulls[6] = true;
+ 
+ 			if (beentry->st_proc_start_timestamp != 0)
+ 				values[7] = TimestampTzGetDatum(beentry->st_proc_start_timestamp);
+ 			else
+ 				nulls[7] = true;
+ 
+ 			/* A zeroed client addr means we don't know */
+ 			memset(&zero_clientaddr, 0, sizeof(zero_clientaddr));
+ 			if (memcmp(&(beentry->st_clientaddr), &zero_clientaddr,
+ 									  sizeof(zero_clientaddr) == 0))
+ 			{
+ 				nulls[8] = true;
+ 				nulls[9] = true;
+ 			}
+ 			else
+ 			{
+ 				if (beentry->st_clientaddr.addr.ss_family == AF_INET
+ #ifdef HAVE_IPV6
+ 					|| beentry->st_clientaddr.addr.ss_family == AF_INET6
+ #endif
+ 				   )
+ 				{
+ 					char        remote_host[NI_MAXHOST];
+ 					char		remote_port[NI_MAXSERV];
+ 					int			ret;
+ 
+ 					remote_host[0] = '\0';
+ 					remote_port[0] = '\0';
+ 					ret = pg_getnameinfo_all(&beentry->st_clientaddr.addr,
+ 											 beentry->st_clientaddr.salen,
+ 											 remote_host, sizeof(remote_host),
+ 											 remote_port, sizeof(remote_port),
+ 											 NI_NUMERICHOST | NI_NUMERICSERV);
+ 					if (ret)
+ 					{
+ 						nulls[8] = true;
+ 						nulls[9] = true;
+ 					}
+ 					else
+ 					{
+ 						clean_ipv6_addr(beentry->st_clientaddr.addr.ss_family, remote_host);
+ 						values[8] = DirectFunctionCall1(inet_in,
+ 													   CStringGetDatum(remote_host));
+ 						values[9] = Int32GetDatum(atoi(remote_port));
+ 					}
+ 				}
+ 				else if (beentry->st_clientaddr.addr.ss_family == AF_UNIX)
+ 				{
+ 					/*
+ 					 * Unix sockets always reports NULL for host and -1 for port, so it's
+ 					 * possible to tell the difference to connections we have no
+ 					 * permissions to view, or with errors.
+ 					 */
+ 					nulls[8] = true;
+ 					values[9] = DatumGetInt32(-1);
+ 				}
+ 				else
+ 				{
+ 					/* Unknown address type, should never happen */
+ 					nulls[8] = true;
+ 					nulls[9] = true;
+ 				}
+ 			}
+ 		}
+ 		else
+ 		{
+ 			/* No permissions to view data about this session */
+ 			values[3] = CStringGetTextDatum("<insufficient privilege>");
+ 			nulls[4] = true;
+ 			nulls[5] = true;
+ 			nulls[6] = true;
+ 			nulls[7] = true;
+ 			nulls[8] = true;
+ 			nulls[9] = true;
+ 		}
+ 
+ 		tuple = heap_form_tuple(funcctx->tuple_desc, values, nulls);
+ 
+ 		SRF_RETURN_NEXT(funcctx, HeapTupleGetDatum(tuple));
+ 	}
+ 	else
+ 	{
+ 		/* nothing left */
+ 		SRF_RETURN_DONE(funcctx);
+ 	}
+ }
+ 
  
  Datum
  pg_backend_pid(PG_FUNCTION_ARGS)
Index: src/include/catalog/pg_proc.h
===================================================================
RCS file: /cvsroot/pgsql/src/include/catalog/pg_proc.h,v
retrieving revision 1.492
diff -c -r1.492 pg_proc.h
*** src/include/catalog/pg_proc.h	17 Apr 2008 20:56:41 -0000	1.492
--- src/include/catalog/pg_proc.h	28 Apr 2008 13:36:57 -0000
***************
*** 2899,2904 ****
--- 2899,2906 ----
  DESCR("statistics: last auto analyze time for a table");
  DATA(insert OID = 1936 (  pg_stat_get_backend_idset		PGNSP PGUID 12 1 100 f f t t s 0 23 "" _null_ _null_ _null_ pg_stat_get_backend_idset - _null_ _null_ ));
  DESCR("statistics: currently active backend IDs");
+ DATA(insert OID = 2022 (  pg_stat_get_activity			PGNSP PGUID 12 1 100 f f f t s 1 2249 "23" _null_ _null_ _null_ pg_stat_get_activity - _null_ _null_ ));
+ DESCR("statistics: information about currently active backends");
  DATA(insert OID = 2026 (  pg_backend_pid				PGNSP PGUID 12 1 0 f f t f s 0 23 "" _null_ _null_ _null_ pg_backend_pid - _null_ _null_ ));
  DESCR("statistics: current backend PID");
  DATA(insert OID = 1937 (  pg_stat_get_backend_pid		PGNSP PGUID 12 1 0 f f t f s 1 23 "23" _null_ _null_ _null_ pg_stat_get_backend_pid - _null_ _null_ ));
#6Magnus Hagander
magnus@hagander.net
In reply to: Magnus Hagander (#5)
Re: pgstat SRF?

Magnus Hagander wrote:

Magnus Hagander wrote:

Tom Lane wrote:

Magnus Hagander <magnus@hagander.net> writes:

While looking over the statistics-for-functions patch
(http://archives.postgresql.org/pgsql-patches/2008-03/msg00300.php),
I came back to a thought I've had before - why do we keep one
function per column for pgstat functions, instead of using a set
returning function? Is there some actual reason for this, or is
it just legacy from a time when it was (much) harder to write
SRFs?

I think it's so that you can build your own stats views instead of
being compelled to select the data someone thought was good for
you.

You can still do that if it's an SRF. You could even make the SRF
take an optional argument to either return a single value (filtered
the same way the individual functions are) or when this one is set
to NULL, return the whole table.

It would make the overhead a lot lower in the most common case
("SELECT
* FROM pg_stat_<somethingorother>"), while only adding a little in
the other cases, I think.

Though I'm not sure that overhead is big enough to care about in the
first place, but if you're VIEWs are longish it could be...

Actually, looking at this once more, the interface to the functions
sucked more than I thought. They're not actually accepting procpid as
parameters, but just an index into the current array in pgstats..
Basically, they're not supposed to be used in any other way than
accessing all the rows at once :-)

Attached is a version of the functions required for pg_stat_activity
implemented as a SRF instead of different functions. A quick benchmark
(grabbing the VIEW 10,000 times on a system with about 500 active
backends) shows it's about 20% faster than the function-per-value
approach, but the runtime per view is still very quick as it is today.
(And most of what overhead there is most likely comes from reading the
stats file)

However, it also implements the lookup-by-PID functionality that IMHO
makes a lot more sense than lookup-by-backend-array-index. This is
obviously a lot more performant than querying the VIEW for all rows -
something that might be a big win for monitoring apps that look for
info about a single backend.

Unsure if we want to go ahead and convert all functions, but I think
we can make a good argument for making *new* stats views (like the
ones about functions that in the queue) based on SRFs instead. It
also has the nice side-effect of less rows in the system tables ;)

Comments?

Unless there are any objections I'll complete this patch along with the
proper documentation, and apply it for now. I will look at converting
some further pgstats stuff into SRFs as well, assuming they show
similar results..

//Magnus