Re: pg_stat_statements: calls under-estimation propagation

Re: pg_stat_statements: calls under-estimation propagation

Re: pg_stat_statements: calls under-estimation propagation

Re: pg_stat_statements: calls under-estimation propagation

Re: pg_stat_statements: calls under-estimation propagation

Re: pg_stat_statements: calls under-estimation propagation

Re: pg_stat_statements: calls under-estimation propagation

Re: pg_stat_statements: calls under-estimation propagation

Re: pg_stat_statements: calls under-estimation propagation

Re: pg_stat_statements: calls under-estimation propagation

Re: pg_stat_statements: calls under-estimation propagation

Re: pg_stat_statements: calls under-estimation propagation

Re: pg_stat_statements: calls under-estimation propagation

Re: pg_stat_statements: calls under-estimation propagation

Re: pg_stat_statements: calls under-estimation propagation

Re: pg_stat_statements: calls under-estimation propagation

Re: pg_stat_statements: calls under-estimation propagation

Re: pg_stat_statements: calls under-estimation propagation

Re: pg_stat_statements: calls under-estimation propagation

Re: pg_stat_statements: calls under-estimation propagation

Re: pg_stat_statements: calls under-estimation propagation

On Sat, Dec 29, 2012 at 7:16 PM, Daniel Farina  wrote:
> On Sat, Dec 29, 2012 at 7:12 PM, Peter Geoghegan  wrote:
>> On 30 December 2012 02:45, Daniel Farina  wrote:
>>> As I recall, the gist of this objection had to do with a false sense
>>> of stability of the hash value, and the desire to enforce the ability
>>> to alter it.  Here's an option: xor the hash value with the
>>> 'statistics session id', so it's *known* to be unstable between
>>> sessions.  That gets you continuity in the common case and sound
>>> deprecation in the less-common cases (crashes, format upgrades, stat
>>> resetting).
>>
>> Hmm. I like the idea, but a concern there would be that you'd
>> introduce additional scope for collisions in the third-party utility
>> building time-series data from snapshots. I currently put the
>> probability of a collision within pg_stat_statements as 1% in the
>> event of a pg_stat_statements.max of 10,000.
>
> We can use a longer session key and duplicate the queryid (effectively
> padding) a couple of times to complete the XOR.  I think that makes
> the cases of collisions introduced by this astronomically low, as an
> increase over the base collision rate.

A version implementing that is attached, except I generate an
additional 64-bit session not exposed to the client to prevent even
casual de-leaking of the session state.  That may seem absurd, until
someone writes a tool that de-xors things and relies on it and then
nobody feels inclined to break it.  It also keeps the public session
number short.

I also opted to save the underestimate since I'm adding a handful of
fixed width fields to the file format anyway.

--
fdr

Re: pg_stat_statements: calls under-estimation propagation

Re: pg_stat_statements: calls under-estimation propagation

Re: pg_stat_statements: calls under-estimation propagation

Re: pg_stat_statements: calls under-estimation propagation

Re: pg_stat_statements: calls under-estimation propagation

Hello,
Please find attached pg_stat_statements-identification-v9.patch.
I have tried to address the following review comments
1. Use version PGSS_TUP_V1_2
2.Fixed total time being zero
3. Remove 'session_start' from the view and use point release number
to generate queryid
4. Hide only queryid and query text and not all fields from unauthorized user
5. Removed introduced field from view and code as statistics session
concept is not being used
6. Removed struct Instrumentation usage
7. Updated sgml to reflect changes made. Removed all references to
statistics session, and introduced fields.

regards
Sameer

Re: pg_stat_statements: calls under-estimation propagation

Re: pg_stat_statements: calls under-estimation propagation

Hello,
Please find v10 of patch attached. This patch addresses following
review comments
1. Removed errcode and used elogs for error "pg_stat_statements schema
is not supported by its binary"
2. Removed comments and other code formatting not directly relevant to
patch functionality
3. changed position of query_id in view to userid,dbid,query_id..
4 cleaned the patch some more to avoid unnecessary  whitespaces, newlines.

I assume the usage of PGSS_TUP_LATEST after explanation given.
Also the mixing of PG_VERSION_NUM with query_id is ok after after
explanation given.

regards
Sameer

Re: pg_stat_statements: calls under-estimation propagation

Re: pg_stat_statements: calls under-estimation propagation

Re: pg_stat_statements: calls under-estimation propagation

Re: pg_stat_statements: calls under-estimation propagation

Re: pg_stat_statements: calls under-estimation propagation

On Wed, Oct 2, 2013 at 6:40 PM, Sameer Thakur  wrote:
>>
>> Looks pretty good. Do you want to package up the patch with your
>> change and do the honors and re-submit it? Thanks for helping out so
>> much!
> Sure, will do. Need to add a bit of documentation explaining
> statistics session as well.
> I did some more basic testing around pg_stat_statements.max, now that
> we have clarity from Peter about its value being legitimate below 100.
> Seems to work fine, with pg_stat_statements =4 the max unique queries
> in the view are 4. On the 5th query the view holds just the latest
> unique query discarding the previous 4. Fujii had reported a
> segmentation fault in this scenario.
> Thank you for the patch

Please find the patch attached

regards
Sameer

Re: pg_stat_statements: calls under-estimation propagation

Re: pg_stat_statements: calls under-estimation propagation

Re: pg_stat_statements: calls under-estimation propagation

Re: pg_stat_statements: calls under-estimation propagation

Re: pg_stat_statements: calls under-estimation propagation

Re: pg_stat_statements: calls under-estimation propagation

Re: pg_stat_statements: calls under-estimation propagation

Re: pg_stat_statements: calls under-estimation propagation

Re: pg_stat_statements: calls under-estimation propagation

Re: pg_stat_statements: calls under-estimation propagation

Re: pg_stat_statements: calls under-estimation propagation

Please find patch attached which adds documentation for session_start
and introduced fields and corrects documentation for queryid to be
query_id. session_start remains in the view as agreed.
regards
Sameer

Re: pg_stat_statements: calls under-estimation propagation

On Fri, Sep 20, 2013 at 1:11 AM, Daniel Farina  wrote:
> I think the n-call underestimation propagation may not be quite precise for
> various detailed reasons (having to do with 'sticky' queries) and to make it
> precise is probably more work than it's worth.  And, on more reflection, I'm
> also having a hard time imaging people intuiting that value usefully.  So,
> here's a version removing that.

I forgot about removal of the relevant SGML, amended here in v6.

Re: pg_stat_statements: calls under-estimation propagation

Re: pg_stat_statements: calls under-estimation propagation

Attached is a cumulative patch attempting to address the below.  One
can see the deltas to get there at https://github.com/fdr/postgres.git
error-prop-pg_stat_statements-v2.

On Fri, Dec 28, 2012 at 9:58 AM, Peter Geoghegan  wrote:
> However, with this approach, calls_underest values might appear to the
> user in what might be considered an astonishing order. Now, I'm not
> suggesting that that's a real problem - just that they may not be the
> semantics we want, particularly as we can reasonably defer assigning a
> calls_underest until a sticky entry is "unstuck", and an entry becomes
> user-visible, within pgss_store().

Fix for this as I understand it:

*** a/contrib/pg_stat_statements/pg_stat_statements.c
--- b/contrib/pg_stat_statements/pg_stat_statements.c
***************
*** 1036,1041 **** pgss_store(const char *query, uint32 queryId,
--- 1036,1042 ----
  			e->counters.usage = USAGE_INIT;

  		e->counters.calls += 1;
+ 		e->counters.calls_underest = pgss->calls_max_underest;
  		e->counters.total_time += total_time;
  		e->counters.rows += rows;
  		e->counters.shared_blks_hit += bufusage->shared_blks_hit;
***************
*** 1264,1272 **** entry_alloc(pgssHashKey *key, const char *query,
int query_len, bool sticky)
  		/* set the appropriate initial usage count */
  		entry->counters.usage = sticky ? pgss->cur_median_usage : USAGE_INIT;

- 		/* propagate calls under-estimation bound */
- 		entry->counters.calls_underest = pgss->calls_max_underest;
-
  		/* re-initialize the mutex each time ... we assume no one using it */
  		SpinLockInit(&entry->mutex);
  		/* ... and don't forget the query text */
--- 1265,1270 ----

> Also, it seems like you should initialise pgss->calls_max_underest,
> within pgss_shmem_startup().

Easy enough. Somehow I wrongly thought zero-initialization was a thing
for the shmem functions.

*** a/contrib/pg_stat_statements/pg_stat_statements.c
--- b/contrib/pg_stat_statements/pg_stat_statements.c
***************
*** 426,431 **** pgss_shmem_startup(void)
--- 426,432 ----
  	{
  		/* First time through ... */
  		pgss->lock = LWLockAssign();
+ 		pgss->calls_max_underest = 0;
  		pgss->query_size = pgstat_track_activity_query_size;
  		pgss->cur_median_usage = ASSUMED_MEDIAN_INIT;
  	}

> You should probably serialise the value
> to disk, and initialise it to 0 if there is no such value to begin
> with.

I prefer different approach here: just compute it while loading the
entries from disk, since the calls + underestimation can be used to
find a new pessimum underestimation global value.

*** a/contrib/pg_stat_statements/pg_stat_statements.c
--- b/contrib/pg_stat_statements/pg_stat_statements.c
***************
*** 419,424 **** pgss_shmem_startup(void)
--- 419,425 ----
  	int			query_size;
  	int			buffer_size;
  	char	   *buffer = NULL;
+ 	int64		calls_max_underest = 0;

  	if (prev_shmem_startup_hook)
  		prev_shmem_startup_hook();
***************
*** 440,446 **** pgss_shmem_startup(void)
  	{
  		/* First time through ... */
  		pgss->lock = LWLockAssign();
! 		pgss->calls_max_underest = 0;
  		pgss->query_size = pgstat_track_activity_query_size;
  		pgss->cur_median_usage = ASSUMED_MEDIAN_INIT;
  	}
--- 441,447 ----
  	{
  		/* First time through ... */
  		pgss->lock = LWLockAssign();
! 		pgss->calls_max_underest = calls_max_underest;
  		pgss->query_size = pgstat_track_activity_query_size;
  		pgss->cur_median_usage = ASSUMED_MEDIAN_INIT;
  	}
***************
*** 528,533 **** pgss_shmem_startup(void)
--- 529,545 ----
  												   temp.query_len,
  												   query_size - 1);

+ 		/*
+ 		 * Compute maxima of under-estimation over the read entries
+ 		 * for reinitializing pgss->calls_max_underest.
+ 		 */
+ 		{
+ 			int64 cur_underest;
+ 				
+ 			cur_underest = temp.calls + temp.calls_underest;
+ 			calls_max_underest = Max(calls_max_underest, cur_underest);
+ 		}
+
  		/* make the hashtable entry (discards old entries if too many) */
  		entry = entry_alloc(&temp.key, buffer, temp.query_len, false);

***************
*** 535,540 **** pgss_shmem_startup(void)
--- 547,559 ----
  		entry->counters = temp.counters;
  	}

+ 	/*
+ 	 * Reinitialize global under-estimation information from the
+ 	 * computed maxima, if any.  Otherwise, calls_max_underest should
+ 	 * be zero.
+ 	 */
+ 	pgss->calls_max_underest = calls_max_underest;
+
  	pfree(buffer);
  	FreeFile(file);


> I think you probably should have created a
> PG_STAT_STATEMENTS_COLS_V1_1 macro, since that version of the module
> is now legacy, like *V1_0 is in HEAD.

Indeed.  Prepare to scroll, this change is not very complex but a bit
more bloated looking:

--- b/contrib/pg_stat_statements/pg_stat_statements.c
***************
*** 96,101 **** typedef struct pgssHashKey
--- 96,115 ----
  } pgssHashKey;

  /*
+  * Identifies the tuple format detected by pg_stat_statements.
+  *
+  * Used to identify features of newer formats and enable smooth
+  * upgrades: one can install a new pg_stat_statements binary while
+  * running with the old SQL function definitions.
+  */
+ typedef enum pgssTupVersion
+ {
+ 	PGSS_TUP_V1_0 = 1,
+ 	PGSS_TUP_V1_1,
+ 	PGSS_TUP_LATEST
+ } pgssTupVersion;
+
+ /*
   * The actual stats counters kept within pgssEntry.
   */
  typedef struct Counters
***************
*** 1078,1083 **** pg_stat_statements_reset(PG_FUNCTION_ARGS)
--- 1092,1098 ----
  }

  #define PG_STAT_STATEMENTS_COLS_V1_0	14
+ #define PG_STAT_STATEMENTS_COLS_V1_1	18
  #define PG_STAT_STATEMENTS_COLS			19

  /*
***************
*** 1095,1101 **** pg_stat_statements(PG_FUNCTION_ARGS)
  	bool		is_superuser = superuser();
  	HASH_SEQ_STATUS hash_seq;
  	pgssEntry  *entry;
! 	bool		sql_supports_v1_1_counters = true;

  	if (!pgss || !pgss_hash)
  		ereport(ERROR,
--- 1110,1116 ----
  	bool		is_superuser = superuser();
  	HASH_SEQ_STATUS hash_seq;
  	pgssEntry  *entry;
! 	pgssTupVersion detected_version;

  	if (!pgss || !pgss_hash)
  		ereport(ERROR,
***************
*** 1116,1123 **** pg_stat_statements(PG_FUNCTION_ARGS)
  	/* Build a tuple descriptor for our result type */
  	if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE)
  		elog(ERROR, "return type must be a row type");
  	if (tupdesc->natts == PG_STAT_STATEMENTS_COLS_V1_0)
! 		sql_supports_v1_1_counters = false;

  	per_query_ctx = rsinfo->econtext->ecxt_per_query_memory;
  	oldcontext = MemoryContextSwitchTo(per_query_ctx);
--- 1131,1158 ----
  	/* Build a tuple descriptor for our result type */
  	if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE)
  		elog(ERROR, "return type must be a row type");
+
+ 	/* Perform version detection */
  	if (tupdesc->natts == PG_STAT_STATEMENTS_COLS_V1_0)
! 		detected_version = PGSS_TUP_V1_0;
! 	else if (tupdesc->natts == PG_STAT_STATEMENTS_COLS_V1_1)
! 		detected_version = PGSS_TUP_V1_1;
! 	else if (tupdesc->natts == PG_STAT_STATEMENTS_COLS)
! 		detected_version = PGSS_TUP_LATEST;
! 	else
! 	{
! 		/*
! 		 * Couldn't identify the tuple format.  Raise error.
! 		 *
! 		 * This is an exceptional case that may only happen in bizarre
! 		 * situations, since it is thought that every released version
! 		 * of pg_stat_statements has a matching schema.
! 		 */
! 		ereport(ERROR,
! 				(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
! 				 errmsg("pg_stat_statements schema is not supported "
! 						"by its installed binary")));
! 	}

  	per_query_ctx = rsinfo->econtext->ecxt_per_query_memory;
  	oldcontext = MemoryContextSwitchTo(per_query_ctx);
***************
*** 1175,1196 **** pg_stat_statements(PG_FUNCTION_ARGS)
  			continue;

  		values[i++] = Int64GetDatumFast(tmp.calls);
! 		values[i++] = Int64GetDatumFast(tmp.calls_underest);
  		values[i++] = Float8GetDatumFast(tmp.total_time);
  		values[i++] = Int64GetDatumFast(tmp.rows);
  		values[i++] = Int64GetDatumFast(tmp.shared_blks_hit);
  		values[i++] = Int64GetDatumFast(tmp.shared_blks_read);
! 		if (sql_supports_v1_1_counters)
  			values[i++] = Int64GetDatumFast(tmp.shared_blks_dirtied);
  		values[i++] = Int64GetDatumFast(tmp.shared_blks_written);
  		values[i++] = Int64GetDatumFast(tmp.local_blks_hit);
  		values[i++] = Int64GetDatumFast(tmp.local_blks_read);
! 		if (sql_supports_v1_1_counters)
  			values[i++] = Int64GetDatumFast(tmp.local_blks_dirtied);
  		values[i++] = Int64GetDatumFast(tmp.local_blks_written);
  		values[i++] = Int64GetDatumFast(tmp.temp_blks_read);
  		values[i++] = Int64GetDatumFast(tmp.temp_blks_written);
! 		if (sql_supports_v1_1_counters)
  		{
  			values[i++] = Float8GetDatumFast(tmp.blk_read_time);
  			values[i++] = Float8GetDatumFast(tmp.blk_write_time);
--- 1210,1232 ----
  			continue;

  		values[i++] = Int64GetDatumFast(tmp.calls);
! 		if (detected_version >= PGSS_TUP_LATEST)
! 			values[i++] = Int64GetDatumFast(tmp.calls_underest);
  		values[i++] = Float8GetDatumFast(tmp.total_time);
  		values[i++] = Int64GetDatumFast(tmp.rows);
  		values[i++] = Int64GetDatumFast(tmp.shared_blks_hit);
  		values[i++] = Int64GetDatumFast(tmp.shared_blks_read);
! 		if (detected_version >= PGSS_TUP_V1_1)
  			values[i++] = Int64GetDatumFast(tmp.shared_blks_dirtied);
  		values[i++] = Int64GetDatumFast(tmp.shared_blks_written);
  		values[i++] = Int64GetDatumFast(tmp.local_blks_hit);
  		values[i++] = Int64GetDatumFast(tmp.local_blks_read);
! 		if (detected_version >= PGSS_TUP_V1_1)
  			values[i++] = Int64GetDatumFast(tmp.local_blks_dirtied);
  		values[i++] = Int64GetDatumFast(tmp.local_blks_written);
  		values[i++] = Int64GetDatumFast(tmp.temp_blks_read);
  		values[i++] = Int64GetDatumFast(tmp.temp_blks_written);
! 		if (detected_version >= PGSS_TUP_V1_1)
  		{
  			values[i++] = Float8GetDatumFast(tmp.blk_read_time);
  			values[i++] = Float8GetDatumFast(tmp.blk_write_time);

> I wonder if the way that pg_stat_statements throws its hands up when
> it comes to crash safety (i.e. the contents of the hash table are
> completely lost) could be a concern here. In other words, a program
> tasked with tracking execution costs over time and graphing
> time-series data from snapshots has to take responsibility for
> ensuring that there hasn't been a crash (or, indeed, a reset).

> Another issue is that I don't think that what you've done here solves
> the problem of uniquely identify each entry over time, in the same way
> that simply exposing the hash value would. I'm concerned with the
> related problem to the problem solved here - simply identifying the
> entry uniquely. As we've already discussed, the query string is an
> imperfect proxy for each entry, even with constants swapped with '?'
> characters (and even when combined with the userid and dbid values -
> it's still not the same as the hashtable key, in a way that is likely
> to bother people that use pg_stat_statements for long enough).

These were not express goals of the patch, but so long as you are
inviting features, attached is a bonus patch that exposes the queryid
and also the notion of a "statistics session" that is re-rolled
whenever the stats file could not be read or the stats are reset, able
to fully explain all obvious causes of retrograde motion in
statistics.  It too is cumulative, so it includes the under-estimation
field.  Notably, I also opted to nullify extra pg_stat_statements
fields when they'd also show "insufficient privileges" (that one is
spared from this censorship), because I feel as though a bit too much
information leaks from pg_stat_statement's statistics to ignore,
especially after adding the query id.  Since the common theme here is
identifying queries, I have called it
"pg_stat_statements-identification", and it can be found in the git
repo above under the same name (...-v1).

-- 
fdr

Re: pg_stat_statements: calls under-estimation propagation

On Sat, Dec 29, 2012 at 4:21 AM, Daniel Farina  wrote:
> These were not express goals of the patch, but so long as you are
> inviting features, attached is a bonus patch that exposes the queryid
> and also the notion of a "statistics session" that is re-rolled
> whenever the stats file could not be read or the stats are reset, able
> to fully explain all obvious causes of retrograde motion in
> statistics.  It too is cumulative, so it includes the under-estimation
> field.  Notably, I also opted to nullify extra pg_stat_statements
> fields when they'd also show "insufficient privileges" (that one is
> spared from this censorship), because I feel as though a bit too much
> information leaks from pg_stat_statement's statistics to ignore,
> especially after adding the query id.  Since the common theme here is
> identifying queries, I have called it
> "pg_stat_statements-identification", and it can be found in the git
> repo above under the same name (...-v1).

A small amendment that doesn't really change the spirit of the
narrative is attached.

--
fdr

pg_stat_statements: calls under-estimation propagation

Hello,

After long delay (sorry) here's a patch implementing what was
hand-waved at in
http://archives.postgresql.org/pgsql-hackers/2012-10/msg00176.php

I am still something at a loss at how to test it besides prodding it
by hand; it seems like it's going to involve infrastructure or
introducing hooks into pg_stat_statements for the express purpose.

The patch can also be sourced from:

  https://github.com/fdr/postgres.git error-prop-pg_stat_statements

Without further ado, the cover letter taken from the top of the patch:

This tries to establish a maximum under-estimate of the number of
calls for a given pg_stat_statements entry.  That means the number of
calls to the canonical form of the query is between 'calls' and 'calls
+ calls_underest'.

This is useful to determine when accumulating statistics if a given
record is bouncing in and out of the pg_stat_statements table, having
its ncalls reset all the time, but also having calls_underest grow
very rapidly.

Records that always stay in pg_stat_statements will have a
calls_underest that do not change at all.

An interesting case is when a query that usually is called is not
called for a while, and falls out of pg_stat_statements.  The result
can be that the query with the most 'calls' can also have more
uncertainty than the query with the second most calls, which is also
exactly the truth in reality.

Unceremoniously bundled into this patch is a reduction in the minimum
table size for pg_stat_statements, from 100 to 1.  Using tiny values
is not likely to be seen in production, but makes testing the patch a
lot easier in some situations.

I will add this to the commitfest.

--
fdr

Re: pg_stat_statements: calls under-estimation propagation

Re: pg_stat_statements: calls under-estimation propagation

Re: pg_stat_statements: calls under-estimation propagation

Re: pg_stat_statements: calls under-estimation propagation

Re: pg_stat_statements: calls under-estimation propagation

Re: pg_stat_statements: calls under-estimation propagation

Re: pg_stat_statements: calls under-estimation propagation

Re: pg_stat_statements: calls under-estimation propagation

Re: pg_stat_statements: calls under-estimation propagation

Re: pg_stat_statements: calls under-estimation propagation

Re: pg_stat_statements: calls under-estimation propagation

Re: pg_stat_statements: calls under-estimation propagation

Re: pg_stat_statements: calls under-estimation propagation

Re: pg_stat_statements: calls under-estimation propagation

Re: pg_stat_statements: calls under-estimation propagation

Re: pg_stat_statements: calls under-estimation propagation

Re: pg_stat_statements: calls under-estimation propagation

Re: pg_stat_statements: calls under-estimation propagation

On Mon, Nov 18, 2013 at 1:54 AM, Sameer Thakur  wrote:
> Please find v10 of patch attached. This patch addresses following
> review comments

I've cleaned this up - revision attached - and marked it "ready for committer".

I decided that queryid should be of type oid, not bigint. This is
arguably a slight abuse of notation, but since ultimately Oids are
just abstract object identifiers (so say the docs), but also because
there is no other convenient, minimal way of representing unsigned
32-bit integers in the view that I'm aware of, I'm inclined to think
that it's appropriate.

In passing, I've made pg_stat_statements invalidate serialized entries
if there is a change in major version. This seems desirable as a
catch-all invalidator of entries.

I note that Tom has objected to exposing the queryid in the past, on
numerous occasions. I'm more confident than ever that it's actually
the right thing to do. I've had people I don't know walk up to me at
conferences and ask me what we don't already expose this at least
twice now. There are very strong indications that many people want
this, and given that I've documented the caveats, I think that we
should trust those calling for this. At the very least, it allows
people to GROUP BY queryid, when they don't want things broken out by
userid.

We're self-evidently already effectively relying on the queryid to be
as stable as it is documented to be in this patch. The hash function
cannot really change in minor releases, because to do so would at the
very least necessitate re-indexing hash indexes, and would of course
invalidate internally managed pg_stat_statements entries, both of
which are undesirable outcomes (and therefore, for these reasons and
more, unlikely). Arguments for not documenting hash_any() do not apply
here -- we're already suffering the full consequences of whatever
queryid instability may exist.

Quite apart from all of that, I think we need to have a way of
identifying particular entries for the purposes of supporting
per-entry "settings". Recent discussion about min/max query time, or
somehow characterizing the distribution of each entry's historic
execution time (or whatever) have not considered one important
questoin: What are you supposed to do when you find out that there is
an outlier (whatever an outlier is)?

I won't go into the details, because there is little point, but I'm
reasonably confident that it will be virtually impossible for
pg_stat_statements itself to usefully classify particular query
executions as outliers (I'm not even sure that we could do it if we
assumed a normal distribution, which would be bogus, and certainly
made very noisy by caching effects and so on. Furthermore, who are we
to say that an outlier is an execution time two sigmas to the right?
Seems useless).

Outliers are typically caused by things like bad plans, or problematic
constant values that appear in the most common values list (and are
therefore just inherently far more expensive to query against), or
lock contention. In all of those cases, with a min/max or something we
probably won't even get to know what the problematic constant values
were when response time suddenly suffers, because of course
pg_stat_statements doesn't help with that. So have we gained much?
Even with detective work, the trail might have gone cold by the time
the outlier is examined. And, monitoring is only one concern -- what
about alerting?

The bigger point is that having this will facilitate being able to
mark entries as "SLA queries" or something like that, where if their
execution exceeds a time (specified by the DBA per entry), that is
assumed to be very bad, and pg_stat_statements complains. That gets
dumped to the logs (which ought to be a rare occurrence when the
facility is used correctly). Of course, the (typically particularly
problematic) constant values *do* appear in the logs, and there is a
greppable keyword, potentially for the benefit of a tool like
tail_n_mail. You could think of this as being like a smart
log_min_duration_statement. I think that the DBA needs to tell
pg_stat_statements what to care about, and what bad looks like. If the
DBA doesn't know where to start specifying such things, the 5 queries
with the most calls can usefully have this set to (mean_execution_time
* 1.5) or something. SLA queries can also be "pinned", perhaps  (that
is, given a "stay of execution" when eviction occurs).

-- 
Peter Geoghegan

Re: pg_stat_statements: calls under-estimation propagation

On Fri, Dec 6, 2013 at 12:24 PM, Tom Lane  wrote:
>> There seems to be no problem even if we use bigint as the type of
>> unsigned 32-bit integer like queryid. For example, txid_current()
>> returns the transaction ID, i.e., unsigned 32-bit integer, as bigint.
>> Could you tell me what the problem is when using bigint for queryid?
>
> We're talking about the output of some view, right, not internal storage?
> +1 for using bigint for that.  Using OID is definitely an abuse, because
> the value *isn't* an OID.  And besides, what if we someday decide we need
> 64-bit keys not 32-bit?

Fair enough. I was concerned about the cost of external storage of
64-bit integers (unlike query text, they might have to be stored many
times for many distinct intervals or something like that), but in
hindsight that was fairly miserly of me.

Attached revision displays signed 64-bit integers instead.

-- 
Peter Geoghegan

Re: pg_stat_statements: calls under-estimation propagation

Re: pg_stat_statements: calls under-estimation propagation

On Sat, Dec 7, 2013 at 3:50 PM, Peter Eisentraut  wrote:
> 32-bit buildfarm members are having problems with this patch.

This should fix that problem. Thanks.


-- 
Peter Geoghegan

Re: pg_stat_statements: calls under-estimation propagation

Re: pg_stat_statements: calls under-estimation propagation