Re: [HACKERS] [FEATURE PATCH] pg_stat_statements with plans (v02)

2018-03-20 Thread Julian Markwort
I just realized I made a whitespace error when I modified the comments.
I hope I don't make a habit of sending erroneous mails.

Please accept my apologies, as well as a guaranteed whitespace-error-free patch 
(updated to version 5 for clarity).

Julian

Julian Markwort schrieb am 2018-03-20:
> To anyone who followed along with this for so long, I'd like to present my 
> newest version of this patch.

> As suggested by Arthur Zakirov, there is now only a single GUC ( 
> pg_stat_statements.plan_track ) responsible for the selection of the plans 
> that should be tracked. Possible values are: all, none, good, or bad.
> I've mostly copied functionality from pl_handler.c . This resulted in the 
> need to include varlena.h so I could use the SplitIdentifierString() function 
> to parse the values, of which several (e.g. 
> pg_stat_statements.plan_track='good, bad') could be used.

> I've also added a new GUC:
> pg_stat_statements.plan_fence_factor
> This GUC can be used to scale the fences of the interval, outside of which a 
> plan might be updated.
> Right now, it is set to 1.5 (common factor for the definition of outliers in 
> boxplots) and you can see through additional colums in the pg_stat_statements 
> view, how often these fences are surpassed by execution times and how often 
> the plans are updated. (The colums are: good_plan_outliers, 
> good_plan_updates, bad_plan_outliers, bad_plan_updates and are primarily here 
> for testing and review purposes and are not essential to this patch, they 
> probably don't add any value for the average user)

> Similarly to the first suggestion by Arthur, I've also changed the plan_reset 
> functionality - there is now only one function, 
> pg_stat_statements_plan_reset(queryid bigint), overloaded with (queryid 
> bigint, plantype cstring) args, that can be used to remove both plans (when 
> omitting the cstring) or either of them. The cstring argument accepts 'good' 
> or 'bad'.

> I also added more comments to the estimations of the quartiles and the 
> calculation of the fences.

> The performance impact lies now at 139312 vs 141841 tps, so roughly 1.78% 
> slower than default pg_stat_statements.
> The fact that these results are a little worse than the previous iteration is 
> due to some changes in the definition of the fences which mistakenly 
> calculated by adding the scaled interquartile distance to the mean, instead 
> of adding it to the respective quartiles, which means that plan updates are 
> triggered a little more often.
> For 4259631 transactions however, only 11 updates for the bad plans where 
> triggered.



> I'm looking forward to your opinions!
> Julian
diff --git a/contrib/pg_stat_statements/Makefile b/contrib/pg_stat_statements/Makefile
index 39b368b70e..49bb462d10 100644
--- a/contrib/pg_stat_statements/Makefile
+++ b/contrib/pg_stat_statements/Makefile
@@ -4,7 +4,8 @@ MODULE_big = pg_stat_statements
 OBJS = pg_stat_statements.o $(WIN32RES)
 
 EXTENSION = pg_stat_statements
-DATA = pg_stat_statements--1.4.sql pg_stat_statements--1.4--1.5.sql \
+DATA = pg_stat_statements--1.4.sql \
+	pg_stat_statements--1.5--1.6.sql pg_stat_statements--1.4--1.5.sql \
 	pg_stat_statements--1.3--1.4.sql pg_stat_statements--1.2--1.3.sql \
 	pg_stat_statements--1.1--1.2.sql pg_stat_statements--1.0--1.1.sql \
 	pg_stat_statements--unpackaged--1.0.sql
diff --git a/contrib/pg_stat_statements/expected/pg_stat_statements.out b/contrib/pg_stat_statements/expected/pg_stat_statements.out
index 5318c3550c..2ca549686f 100644
--- a/contrib/pg_stat_statements/expected/pg_stat_statements.out
+++ b/contrib/pg_stat_statements/expected/pg_stat_statements.out
@@ -395,4 +395,40 @@ SELECT query, calls, rows FROM pg_stat_statements ORDER BY query COLLATE "C";
  SELECT pg_stat_statements_reset() | 1 |1
 (8 rows)
 
+-- test to see if any plans have been recorded.
+SELECT
+  CASE WHEN good_plan_time > 0 THEN 1 ELSE 0 END,
+  CASE WHEN bad_plan_time > 0 THEN 1 ELSE 0 END,
+  CASE WHEN good_plan_timestamp >= timestamp '1970-01-01 00:00:00' THEN 1 ELSE 0 END,
+  CASE WHEN bad_plan_timestamp >= timestamp '1970-01-01 00:00:00' THEN 1 ELSE 0 END
+FROM pg_stat_statements ORDER BY query COLLATE "C";
+ case | case | case | case 
+--+--+--+--
+0 |0 |0 |0
+0 |0 |0 |0
+0 |0 |0 |0
+0 |0 |0 |0
+0 |0 |0 |0
+0 |0 |0 |0
+1 |1 |1 |1
+1 |1 |1 |1
+1 |1 |1 |1
+(9 rows)
+
+-- test if there is some text in the recorded plans.
+SELECT substr(good_plan, 0, 11), substr(bad_plan, 0, 11) FROM pg_stat_statements ORDER BY query COLLATE "C";
+   substr   |   substr   
++
+| 
+| 
+| 
+| 
+| 
+| 
+ Query Text | Query Text
+ Query Text | Query Text
+ Query Text | Query Text
+ Query Text | Query Text
+(10 rows)
+
 DROP EXTENSION pg_stat_statements;

Re: [HACKERS] [FEATURE PATCH] pg_stat_statements with plans (v02)

2018-03-20 Thread Julian Markwort
To anyone who followed along with this for so long, I'd like to present my 
newest version of this patch.

As suggested by Arthur Zakirov, there is now only a single GUC ( 
pg_stat_statements.plan_track ) responsible for the selection of the plans that 
should be tracked. Possible values are: all, none, good, or bad.
I've mostly copied functionality from pl_handler.c . This resulted in the need 
to include varlena.h so I could use the SplitIdentifierString() function to 
parse the values, of which several (e.g. pg_stat_statements.plan_track='good, 
bad') could be used.

I've also added a new GUC:
pg_stat_statements.plan_fence_factor
This GUC can be used to scale the fences of the interval, outside of which a 
plan might be updated.
Right now, it is set to 1.5 (common factor for the definition of outliers in 
boxplots) and you can see through additional colums in the pg_stat_statements 
view, how often these fences are surpassed by execution times and how often the 
plans are updated. (The colums are: good_plan_outliers, good_plan_updates, 
bad_plan_outliers, bad_plan_updates and are primarily here for testing and 
review purposes and are not essential to this patch, they probably don't add 
any value for the average user)

Similarly to the first suggestion by Arthur, I've also changed the plan_reset 
functionality - there is now only one function, 
pg_stat_statements_plan_reset(queryid bigint), overloaded with (queryid bigint, 
plantype cstring) args, that can be used to remove both plans (when omitting 
the cstring) or either of them. The cstring argument accepts 'good' or 'bad'.

I also added more comments to the estimations of the quartiles and the 
calculation of the fences.

The performance impact lies now at 139312 vs 141841 tps, so roughly 1.78% 
slower than default pg_stat_statements.
The fact that these results are a little worse than the previous iteration is 
due to some changes in the definition of the fences which mistakenly calculated 
by adding the scaled interquartile distance to the mean, instead of adding it 
to the respective quartiles, which means that plan updates are triggered a 
little more often.
For 4259631 transactions however, only 11 updates for the bad plans where 
triggered.



I'm looking forward to your opinions!
Julian
diff --git a/contrib/pg_stat_statements/Makefile b/contrib/pg_stat_statements/Makefile
index 39b368b70e..49bb462d10 100644
--- a/contrib/pg_stat_statements/Makefile
+++ b/contrib/pg_stat_statements/Makefile
@@ -4,7 +4,8 @@ MODULE_big = pg_stat_statements
 OBJS = pg_stat_statements.o $(WIN32RES)
 
 EXTENSION = pg_stat_statements
-DATA = pg_stat_statements--1.4.sql pg_stat_statements--1.4--1.5.sql \
+DATA = pg_stat_statements--1.4.sql \
+	pg_stat_statements--1.5--1.6.sql pg_stat_statements--1.4--1.5.sql \
 	pg_stat_statements--1.3--1.4.sql pg_stat_statements--1.2--1.3.sql \
 	pg_stat_statements--1.1--1.2.sql pg_stat_statements--1.0--1.1.sql \
 	pg_stat_statements--unpackaged--1.0.sql
diff --git a/contrib/pg_stat_statements/expected/pg_stat_statements.out b/contrib/pg_stat_statements/expected/pg_stat_statements.out
index 5318c3550c..2ca549686f 100644
--- a/contrib/pg_stat_statements/expected/pg_stat_statements.out
+++ b/contrib/pg_stat_statements/expected/pg_stat_statements.out
@@ -395,4 +395,40 @@ SELECT query, calls, rows FROM pg_stat_statements ORDER BY query COLLATE "C";
  SELECT pg_stat_statements_reset() | 1 |1
 (8 rows)
 
+-- test to see if any plans have been recorded.
+SELECT
+  CASE WHEN good_plan_time > 0 THEN 1 ELSE 0 END,
+  CASE WHEN bad_plan_time > 0 THEN 1 ELSE 0 END,
+  CASE WHEN good_plan_timestamp >= timestamp '1970-01-01 00:00:00' THEN 1 ELSE 0 END,
+  CASE WHEN bad_plan_timestamp >= timestamp '1970-01-01 00:00:00' THEN 1 ELSE 0 END
+FROM pg_stat_statements ORDER BY query COLLATE "C";
+ case | case | case | case 
+--+--+--+--
+0 |0 |0 |0
+0 |0 |0 |0
+0 |0 |0 |0
+0 |0 |0 |0
+0 |0 |0 |0
+0 |0 |0 |0
+1 |1 |1 |1
+1 |1 |1 |1
+1 |1 |1 |1
+(9 rows)
+
+-- test if there is some text in the recorded plans.
+SELECT substr(good_plan, 0, 11), substr(bad_plan, 0, 11) FROM pg_stat_statements ORDER BY query COLLATE "C";
+   substr   |   substr   
++
+| 
+| 
+| 
+| 
+| 
+| 
+ Query Text | Query Text
+ Query Text | Query Text
+ Query Text | Query Text
+ Query Text | Query Text
+(10 rows)
+
 DROP EXTENSION pg_stat_statements;
diff --git a/contrib/pg_stat_statements/pg_stat_statements--1.5--1.6.sql b/contrib/pg_stat_statements/pg_stat_statements--1.5--1.6.sql
new file mode 100644
index 00..6c8f743ee5
--- /dev/null
+++ b/contrib/pg_stat_statements/pg_stat_statements--1.5--1.6.sql
@@ -0,0 +1,82 @@
+/* contrib/pg_stat_statements/pg_stat_statements--1.5--1.6.sql */
+
+-- complain if script 

Re: [HACKERS] [FEATURE PATCH] pg_stat_statements with plans (v02)

2018-03-13 Thread Arthur Zakirov
On Mon, Mar 12, 2018 at 02:11:42PM +0100, Julian Markwort wrote:
> > In same manner pg_stat_statements_good_plan_reset() and
> > pg_stat_statements_bad_plan_reset() functions can be combined in
> > function:
> 
> > pg_stat_statements_plan_reset(in queryid bigint, in good boolean, in bad
> > boolean)
> 
> This is also sensible, however if any more kinds of plans would be added in 
> the future, there would be a lot of flags in this function. I think having 
> varying amounts of flags (between extension versions) as arguments to the 
> function also makes any upgrade paths difficult. Thinking more about this, we 
> could also user function overloading to avoid this.
> An alternative would be to have the caller pass the type of plan he wants to 
> reset. Omitting the type would result in the deletion of all plans, maybe?
> pg_stat_statements_plan_reset(in queryid bigint, in plan_type text)

Yes, it is a good idea. But maybe instead of passing an empty string
into plans type we should overload the function with only queryid
argument. I think it is a common practice in PostgreSQL. Otherwise
allowance to pass empty string as plan type may confuse users. So
functions declaration may be the following:

pg_stat_statements_plan_reset(in queryid bigint, in plan_type text)
pg_stat_statements_plan_reset(in queryid bigint)

+   interquartile_dist = 2.0*(0.6745 * 
sqrt(e->counters.sum_var_time / e->counters.calls));

I think it would be better to have defines for 2.0 and 0.6745 values for
the sake of readability.

-- 
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company



Re: [HACKERS] [FEATURE PATCH] pg_stat_statements with plans (v02)

2018-03-12 Thread Julian Markwort
Arthur Zakirov wrote on 2018-03-09:
> I'd like to review the patch and leave a feedback. I tested it with
> different indexes on same table and with same queries and it works fine.

Thanks for taking some time to review my patch, Arthur!

> First of all, GUC variables and functions. How about union
> 'pg_stat_statements.good_plan_enable' and
> 'pg_stat_statements.bad_plan_enable' into
> 'pg_stat_statements.track_plan' with GUC_LIST_INPUT flag? It may accept
> comma separated values 'good' and 'bad'. It lets to add another tracking
> type in future without adding new variable.

This sounds like a good idea to me; Somebody already suggested that tracking an 
"average plan" would be interesting as well, however I don't have any clever 
ideas on how to identify such a plan.

> In same manner pg_stat_statements_good_plan_reset() and
> pg_stat_statements_bad_plan_reset() functions can be combined in
> function:

> pg_stat_statements_plan_reset(in queryid bigint, in good boolean, in bad
> boolean)

This is also sensible, however if any more kinds of plans would be added in the 
future, there would be a lot of flags in this function. I think having varying 
amounts of flags (between extension versions) as arguments to the function also 
makes any upgrade paths difficult. Thinking more about this, we could also user 
function overloading to avoid this.
An alternative would be to have the caller pass the type of plan he wants to 
reset. Omitting the type would result in the deletion of all plans, maybe?
pg_stat_statements_plan_reset(in queryid bigint, in plan_type text)

I'm not sure if there are any better ways to do this?

> Further comments on the code.
 You're right about all of those, thanks!



Re: [HACKERS] [FEATURE PATCH] pg_stat_statements with plans (v02)

2018-03-09 Thread Arthur Zakirov
Greetings,

On Thu, Mar 08, 2018 at 02:58:34PM +0100, Julian Markwort wrote:
> > I'd love to hear more feedback, here are two ideas to polish this
> > patch:
> > a) Right now, good_plan and bad_plan collection can be activated and
> > deactivated with separate GUCs. I think it would be sensible to
> > collect
> > either both or none. (This would result in fewer convoluted
> > conditionals.)
> > b) Would you like to be able to tune the percentiles yourself, to
> > adjust for the point at which a new plan is stored?

I'd like to review the patch and leave a feedback. I tested it with
different indexes on same table and with same queries and it works fine.

First of all, GUC variables and functions. How about union
'pg_stat_statements.good_plan_enable' and
'pg_stat_statements.bad_plan_enable' into
'pg_stat_statements.track_plan' with GUC_LIST_INPUT flag? It may accept
comma separated values 'good' and 'bad'. It lets to add another tracking
type in future without adding new variable.

In same manner pg_stat_statements_good_plan_reset() and
pg_stat_statements_bad_plan_reset() functions can be combined in
function:

pg_stat_statements_plan_reset(in queryid bigint, in good boolean, in bad
boolean)

Further comments on the code.

+-- test to see if any plans have been recorded.
+SELECT
+  CASE WHEN good_plan_time > 0 THEN 1 ELSE 0 END,
+  CASE WHEN bad_plan_time > 0 THEN 1 ELSE 0 END,
+  CASE WHEN good_plan_timestamp >= timestamp '1970-01-01 00:00:00' THEN 1 ELSE 
0 END,
+  CASE WHEN good_plan_timestamp >= timestamp '1970-01-01 00:00:00' THEN 1 ELSE 
0 END

I think here is a typo. Last case should be bad_plan_timestamp.

+   good_plan_str = palloc(1 * sizeof(char));
+   *good_plan_str = '\0';
...
+   bad_plan_str = palloc(1 * sizeof(char));
+   *bad_plan_str = '\0';

Here we can use empty string literals instead of pallocs. For example:

const char *good_plan_str;
const char *bad_plan_str;
...
good_plan_str = "";
...
bad_plan_str = "";

+   interquartile_dist = 2.0*(0.6745 * 
sqrt(e->counters.sum_var_time / e->counters.calls));

It is worth to check e->counters.calls for zero here. Because the entry
can be sticky.

-- 
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company



Re: [HACKERS] [FEATURE PATCH] pg_stat_statements with plans (v02)

2018-03-08 Thread Julian Markwort
I've goofed up now, sorry for failing to attach my updated patch.

Am Donnerstag, den 08.03.2018, 14:55 +0100 schrieb Julian Markwort:
> Tom Lane wrote on 2018-03-02:
> > You need to make your changes in a 1.5--1.6
> > upgrade file.  Otherwise there's no clean path for existing
> > installations
> > to upgrade to the new version.
> 
> I've adressed all the issues that were brought up so far:
> 1. there is now only an added 1.5--1.6.sql file.
> 2. the overhead, as previously discussed (as much as a 12% decrease
> in
> TPS during read-only tests), is now gone, the problem was that I was
> collecting the plan String before checking if it needed to be stored
> at
> all.
> The patched version is now 99.95% as fast as unmodified
> pg_stat_statements.
> 3. I've cleaned up my own code and made sure it adheres to GNU C
> coding
> style, I was guilty of some // comments and curly brackets were
> sometimes in the same line as my control structures.
> 
> I'd love to hear more feedback, here are two ideas to polish this
> patch:
> a) Right now, good_plan and bad_plan collection can be activated and
> deactivated with separate GUCs. I think it would be sensible to
> collect
> either both or none. (This would result in fewer convoluted
> conditionals.)
> b) Would you like to be able to tune the percentiles yourself, to
> adjust for the point at which a new plan is stored?
> 
> Greetings
> Juliandiff --git a/contrib/pg_stat_statements/Makefile b/contrib/pg_stat_statements/Makefile
index 39b368b..49bb462 100644
--- a/contrib/pg_stat_statements/Makefile
+++ b/contrib/pg_stat_statements/Makefile
@@ -4,7 +4,8 @@ MODULE_big = pg_stat_statements
 OBJS = pg_stat_statements.o $(WIN32RES)
 
 EXTENSION = pg_stat_statements
-DATA = pg_stat_statements--1.4.sql pg_stat_statements--1.4--1.5.sql \
+DATA = pg_stat_statements--1.4.sql \
+	pg_stat_statements--1.5--1.6.sql pg_stat_statements--1.4--1.5.sql \
 	pg_stat_statements--1.3--1.4.sql pg_stat_statements--1.2--1.3.sql \
 	pg_stat_statements--1.1--1.2.sql pg_stat_statements--1.0--1.1.sql \
 	pg_stat_statements--unpackaged--1.0.sql
diff --git a/contrib/pg_stat_statements/expected/pg_stat_statements.out b/contrib/pg_stat_statements/expected/pg_stat_statements.out
index 5318c35..3e79890 100644
--- a/contrib/pg_stat_statements/expected/pg_stat_statements.out
+++ b/contrib/pg_stat_statements/expected/pg_stat_statements.out
@@ -395,4 +395,40 @@ SELECT query, calls, rows FROM pg_stat_statements ORDER BY query COLLATE "C";
  SELECT pg_stat_statements_reset() | 1 |1
 (8 rows)
 
+-- test to see if any plans have been recorded.
+SELECT
+  CASE WHEN good_plan_time > 0 THEN 1 ELSE 0 END,
+  CASE WHEN bad_plan_time > 0 THEN 1 ELSE 0 END,
+  CASE WHEN good_plan_timestamp >= timestamp '1970-01-01 00:00:00' THEN 1 ELSE 0 END,
+  CASE WHEN good_plan_timestamp >= timestamp '1970-01-01 00:00:00' THEN 1 ELSE 0 END
+FROM pg_stat_statements ORDER BY query COLLATE "C";
+ case | case | case | case 
+--+--+--+--
+0 |0 |0 |0
+0 |0 |0 |0
+0 |0 |0 |0
+0 |0 |0 |0
+0 |0 |0 |0
+0 |0 |0 |0
+1 |1 |1 |1
+1 |1 |1 |1
+1 |1 |1 |1
+(9 rows)
+
+-- test if there is some text in the recorded plans.
+select substr(good_plan, 0, 11), substr(bad_plan, 0, 11) from pg_stat_statements ORDER BY query COLLATE "C";
+   substr   |   substr   
++
+| 
+| 
+| 
+| 
+| 
+| 
+ Query Text | Query Text
+ Query Text | Query Text
+ Query Text | Query Text
+ Query Text | Query Text
+(10 rows)
+
 DROP EXTENSION pg_stat_statements;
diff --git a/contrib/pg_stat_statements/pg_stat_statements--1.5--1.6.sql b/contrib/pg_stat_statements/pg_stat_statements--1.5--1.6.sql
new file mode 100644
index 000..5302d01
--- /dev/null
+++ b/contrib/pg_stat_statements/pg_stat_statements--1.5--1.6.sql
@@ -0,0 +1,78 @@
+/* contrib/pg_stat_statements/pg_stat_statements--1.5--1.6.sql */
+
+-- complain if script is sourced in psql, rather than via CREATE EXTENSION
+\echo Use "ALTER EXTENSION pg_stat_statements UPDATE TO '1.6'" to load this file. \quit
+
+/* First we have to remove them from the extension */
+ALTER EXTENSION pg_stat_statements DROP VIEW pg_stat_statements;
+ALTER EXTENSION pg_stat_statements DROP FUNCTION pg_stat_statements(boolean);
+ALTER EXTENSION pg_stat_statements DROP FUNCTION pg_stat_statements_reset();
+
+/* Then we can drop them */
+DROP VIEW pg_stat_statements;
+DROP FUNCTION pg_stat_statements(boolean);
+DROP FUNCTION pg_stat_statements_reset();
+
+-- Register functions.
+CREATE FUNCTION pg_stat_statements_reset()
+RETURNS void
+AS 'MODULE_PATHNAME'
+LANGUAGE C PARALLEL SAFE;
+
+CREATE FUNCTION pg_stat_statements_good_plan_reset(IN queryid bigint)
+RETURNS void
+AS 'MODULE_PATHNAME'
+LANGUAGE C PARALLEL SAFE;
+
+CREATE FUNCTION pg_stat_statements_bad_plan_reset(IN queryid 

Re: [HACKERS] [FEATURE PATCH] pg_stat_statements with plans (v02)

2018-03-08 Thread Julian Markwort
Tom Lane wrote on 2018-03-02:
> You need to make your changes in a 1.5--1.6
> upgrade file.  Otherwise there's no clean path for existing
> installations
> to upgrade to the new version.

I've adressed all the issues that were brought up so far:
1. there is now only an added 1.5--1.6.sql file.
2. the overhead, as previously discussed (as much as a 12% decrease in
TPS during read-only tests), is now gone, the problem was that I was
collecting the plan String before checking if it needed to be stored at
all.
The patched version is now 99.95% as fast as unmodified
pg_stat_statements.
3. I've cleaned up my own code and made sure it adheres to GNU C coding
style, I was guilty of some // comments and curly brackets were
sometimes in the same line as my control structures.

I'd love to hear more feedback, here are two ideas to polish this
patch:
a) Right now, good_plan and bad_plan collection can be activated and
deactivated with separate GUCs. I think it would be sensible to collect
either both or none. (This would result in fewer convoluted
conditionals.)
b) Would you like to be able to tune the percentiles yourself, to
adjust for the point at which a new plan is stored?

Greetings
Julian

smime.p7s
Description: S/MIME cryptographic signature


Re: [HACKERS] [FEATURE PATCH] pg_stat_statements with plans (v02)

2018-03-02 Thread Tom Lane
Julian Markwort  writes:
> Andres Freund wrote on 2018-03-02:
>> and I'd checked that 1.5 already exists. But you just renamed the file,
>> presumably because it's essentially rewriting the whole file?  I'm not
>> sure I'm a big fan of doing so, because that makes testing the upgrade
>> path more work.

> You're right about 1.5 already existing. I wasn't sure about the versioning 
> policy for extensions and seeing as version 1.5 only changed a few grants I 
> quasi reused 1.5 for my intentions.

Nope, that's totally wrong.  You can get away with that if we've not
already shipped a 1.5 release --- but we did ship it in v10, so that
version is frozen now.  You need to make your changes in a 1.5--1.6
upgrade file.  Otherwise there's no clean path for existing installations
to upgrade to the new version.

regards, tom lane



Re: [HACKERS] [FEATURE PATCH] pg_stat_statements with plans (v02)

2018-03-02 Thread Julian Markwort
Andres Freund wrote on 2018-03-02:
> Yea, I misread the diff to think you added a conflicting version. Due
> to:
> -DATA =3D pg_stat_statements--1.4.sql pg_stat_statements--1.4--1.5.sql \
> +DATA =3D pg_stat_statements--1.5.sql pg_stat_statements--1.4--1.5.sql \

> and I'd checked that 1.5 already exists. But you just renamed the file,
> presumably because it's essentially rewriting the whole file?  I'm not
> sure I'm a big fan of doing so, because that makes testing the upgrade
> path more work.

You're right about 1.5 already existing. I wasn't sure about the versioning 
policy for extensions and seeing as version 1.5 only changed a few grants I 
quasi reused 1.5 for my intentions.

> What workload did you run? read/write or readonly? This seems like a
> feature were readonly makes a lot more sense. But ~1800 tps strongly
> suggests that's not what you did?

I'm sorry I forgot to mention this; I ran all tests as read-write.

> > With pg_stat_statements on, the latter test (10 minutes) resulted in 1833 
> > tps, while the patched version resulted in 1700 tps, so a little over 7% 
> > overhead? Well, the "control run", without pg_stat_statements delivered 
> > only 1806 tps, so variance seems to be quite high.

> That's quite some overhead, I'd say.

Yes, but I wouldn't give a warranty that it is neither more nor less overhead 
than 7%, seeing as for my testing, the tps were higher for (unmodified) pgss 
enabled vs no pgss at all.

> > If anybody has any recommendations for a setup that generates less 
> > variance, I'll try this again.

> I'd suggest disabling turboost, in my experience that makes tests
> painful to repeat, because it'll strongly depend on the current HW
> temperature.
This might be a problem for average systems but I'm fairly certain this isn't 
the issue here.

I might try some more benchmarks and will in particular look into running 
read-only tests, as the aforementioned 840 EVO SSD ist -comparatively speaking- 
pretty slow.
Do you have any recommendations as to what constitutes adequate testing times 
regarding pgbench?

Kind regards
Julian



Re: [HACKERS] [FEATURE PATCH] pg_stat_statements with plans (v02)

2018-03-02 Thread Tom Lane
Andres Freund  writes:
> Yea, I misread the diff to think you added a conflicting version. Due
> to:
> -DATA =3D pg_stat_statements--1.4.sql pg_stat_statements--1.4--1.5.sql \
> +DATA =3D pg_stat_statements--1.5.sql pg_stat_statements--1.4--1.5.sql \

> and I'd checked that 1.5 already exists. But you just renamed the file,
> presumably because it's essentially rewriting the whole file?  I'm not
> sure I'm a big fan of doing so, because that makes testing the upgrade
> path more work.

Yeah, that is not project style anymore.  Just add a delta file and leave
it at that.  See e.g. de1d042f5979bc1388e9a6d52a4d445342b04932 for an
example.

(We might from time to time roll up the deltas and replace the base
file with a newer version, but I think that should happen in separate
housekeeping commits.)

regards, tom lane



Re: [HACKERS] [FEATURE PATCH] pg_stat_statements with plans (v02)

2018-03-02 Thread Andres Freund
On 2018-03-02 18:07:32 +0100, Julian Markwort wrote:
> Andres Freund wrote on 2018-03-01:
> > I think the patch probably doesn't apply anymore, due to other changes
> > to pg_stat_statements since its posting. Could you refresh?
> 
> pgss_plans_v02.patch applies cleanly to master, there were no changes to 
> pg_stat_statements since the copyright updates at the beginning of January.
> (pgss_plans_v02.patch is attached to message 
> 1bd396a9-4573-55ad-7ce8-fe7adffa1...@uni-muenster.de and can be found in the 
> current commitfest as well.)

Yea, I misread the diff to think you added a conflicting version. Due
to:
-DATA =3D pg_stat_statements--1.4.sql pg_stat_statements--1.4--1.5.sql \
+DATA =3D pg_stat_statements--1.5.sql pg_stat_statements--1.4--1.5.sql \

and I'd checked that 1.5 already exists. But you just renamed the file,
presumably because it's essentially rewriting the whole file?  I'm not
sure I'm a big fan of doing so, because that makes testing the upgrade
path more work.

> I've tried to gather some meaningful results, however either my testing 
> methodology was flawed (as variance between all my passes of pgbench was 
> rather high) or the takeaway is that the feature only generates little 
> overhead.
> This is what I've run on my workstation using a Ryzen 1700 and 16GB of RAM 
> and an old Samsung 840 Evo as boot drive, which also held the database:
> The database used for the tests was dropped and pgbench initialized anew for 
> each test (pgss off, pgss on, pgss on with plan collection) using a scaling 
> of 16437704*0.003~=50 (roughly what the phoronix test suite uses for a buffer 
> test).
> Also similar to the phoronix test suite, I used 8 jobs and 32 connections for 
> a normal multithreaded load.
> 
> I then ran 10 passes, each for 60 seconds, with a 30 second pause between 
> them, as well as another test which ran for 10 minutes.

What workload did you run? read/write or readonly? This seems like a
feature were readonly makes a lot more sense. But ~1800 tps strongly
suggests that's not what you did?


> With pg_stat_statements on, the latter test (10 minutes) resulted in 1833 
> tps, while the patched version resulted in 1700 tps, so a little over 7% 
> overhead? Well, the "control run", without pg_stat_statements delivered only 
> 1806 tps, so variance seems to be quite high.

That's quite some overhead, I'd say.


> If anybody has any recommendations for a setup that generates less variance, 
> I'll try this again.

I'd suggest disabling turboost, in my experience that makes tests
painful to repeat, because it'll strongly depend on the current HW
temperature.

Greetings,

Andres Freund



Re: [HACKERS] [FEATURE PATCH] pg_stat_statements with plans (v02)

2018-03-02 Thread Julian Markwort
Andres Freund wrote on 2018-03-01:
> I think the patch probably doesn't apply anymore, due to other changes
> to pg_stat_statements since its posting. Could you refresh?

pgss_plans_v02.patch applies cleanly to master, there were no changes to 
pg_stat_statements since the copyright updates at the beginning of January.
(pgss_plans_v02.patch is attached to message 
1bd396a9-4573-55ad-7ce8-fe7adffa1...@uni-muenster.de and can be found in the 
current commitfest as well.)

> I've not done any sort of review. Scrolling through I noticed //
> comments which aren't pg coding style.

I'll fix that along with any other problems that might be found in a review.


> I'd like to see a small benchmark showing the overhead of the feature.
> Both in runtime and storage size.

I've tried to gather some meaningful results, however either my testing 
methodology was flawed (as variance between all my passes of pgbench was rather 
high) or the takeaway is that the feature only generates little overhead.
This is what I've run on my workstation using a Ryzen 1700 and 16GB of RAM and 
an old Samsung 840 Evo as boot drive, which also held the database:
The database used for the tests was dropped and pgbench initialized anew for 
each test (pgss off, pgss on, pgss on with plan collection) using a scaling of 
16437704*0.003~=50 (roughly what the phoronix test suite uses for a buffer 
test).
Also similar to the phoronix test suite, I used 8 jobs and 32 connections for a 
normal multithreaded load.

I then ran 10 passes, each for 60 seconds, with a 30 second pause between them, 
as well as another test which ran for 10 minutes.

With pg_stat_statements on, the latter test (10 minutes) resulted in 1833 tps, 
while the patched version resulted in 1700 tps, so a little over 7% overhead? 
Well, the "control run", without pg_stat_statements delivered only 1806 tps, so 
variance seems to be quite high.

The results of the ten successive tests, each running 60 seconds and then 
waiting for 30 seconds, are displayed in the attached plot.
I've tinkered with different settings with pgbench for quite some time now and 
all I can come up with are runs with high variance between them.

If anybody has any recommendations for a setup that generates less variance, 
I'll try this again.

Finally, the more interesting metric regarding this patch is the size of the 
pg_stat_statements.stat file, which stores all the metrics while the database 
is shut down. I reckon that the size of pgss_query_texts.stat (which holds only 
the query strings and plan strings while the database is running) will be 
similar, however it might fluctuate more as new strings are simply appended to 
the file until the garbagecollector decides that it has to be cleaned up.
After running the aforementioned tests, the file was 8566 bytes in size for 
pgss in it's unmodified form, while the tests resulted in 32607 bytes for the 
pgss that collects plans as well. This seems reasonable as plans strings are 
usually longer than the statements from which they result. Worst case, the 
pg_stat_statements.stat holds two plans for each type of statement.
I've not tested the length of the file with different encodings, such as JSON, 
YAML, or XML, however I do not expect any hugely different results.

Greetings
Julian


pgss_plans_pgbench.pdf
Description: Adobe PDF document


Re: [HACKERS] [FEATURE PATCH] pg_stat_statements with plans (v02)

2018-03-01 Thread Andres Freund
Hi,

On 2018-01-10 15:05:39 +0100, Julian Markwort wrote:
> I'd like to follow up to my previous proposition of tracking (some) best and
> worst plans for different queries in the pg_stat_statements extension.

Cool feature.

I think the patch probably doesn't apply anymore, due to other changes
to pg_stat_statements since its posting. Could you refresh?

I've not done any sort of review. Scrolling through I noticed //
comments which aren't pg coding style.

I'd like to see a small benchmark showing the overhead of the feature.
Both in runtime and storage size.

Greetings,

Andres Freund



Re: [HACKERS] [FEATURE PATCH] pg_stat_statements with plans (v02)

2018-01-15 Thread Julian Markwort

On 01/11/2018 03:43 PM, David Fetter wrote:

Is the assumption of a normal distribution reasonable for outlier
plans as you've seen them?

This is a difficult but fair question.
First of all, I'd like to clarify that the normal distribution is 
assumed for the set of all execution times matching a queryid; No 
assumptions are made about the distribution of the outliers themselves.
The primary goal of this approach was the limitation of plan updates, to 
avoid unnecessary IO operations.
When query performance does not vary much, no updates of the plans 
should be necessary, but as soon as query performance varies too much, 
the new plan should be stored.
For the purpose of distinguishing reasonable variance between execution 
times and great variance due to changing conditions which ultimately 
might result in a different plan, the assumption of a normal 
distribution for all execution times suits well.


Based on some early testing, this results in only a few percent of 
updates (1-3%) in relation to the total number of calls, when running 
some short pgbench tests.
As the sample size grows, the assumption of a normal distribution 
becomes increasingly accurate and the (unnecessary) sampling of plans 
decreases.
In a different test, I ran several queries with identical table sizes, 
the queries were fairly simple,  and the statistical evaluation led to 
few updates during these tests. When I increased the table size 
significantly, the database switched to a different plan. Because the 
execution time differed significantly, this new bad plan was stored. 
Similarly, after running a certain query a couple of times, I created an 
index on my test data, which resulted in a speedup which was significant 
enough to result in an update of the good plan.


Now, if a change to the data or the index situation only resulted in an 
insignificant performance increase or decrease (one that falls into the 
interval defined as [mean - 1.5*IQD, mean + 1-5*IQD] ), I think it might 
be possible to assume that we are not interested in an updated plan for 
this scenario.




smime.p7s
Description: S/MIME Cryptographic Signature


Re: [HACKERS] [FEATURE PATCH] pg_stat_statements with plans (v02)

2018-01-11 Thread David Fetter
On Wed, Jan 10, 2018 at 03:05:39PM +0100, Julian Markwort wrote:
> Hello hackers,
> 
> I'd like to follow up to my previous proposition of tracking (some) best and
> worst plans for different queries in the pg_stat_statements extension.
> 
> Based on the comments and suggestions made towards my last endeavour, I've
> taken the path of computing the interquartile distance (by means of an
> adapted z-test, under the assumption of normal distribution, based on the
> mean_time and stddev_time already used by the extension).

Is the assumption of a normal distribution reasonable for outlier
plans as you've seen them?

Best,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate



Re: [HACKERS] [FEATURE PATCH] pg_stat_statements with plans (v02)

2018-01-10 Thread Julian Markwort

Hello hackers,

I'd like to follow up to my previous proposition of tracking (some) best 
and worst plans for different queries in the pg_stat_statements extension.


Based on the comments and suggestions made towards my last endeavour, 
I've taken the path of computing the interquartile distance (by means of 
an adapted z-test, under the assumption of normal distribution, based on 
the mean_time and stddev_time already used by the extension).


A bad plan is recorded, if there is no previously recorded plan, or if 
the current execution time is greater than the maximum of the previously 
recorded plan's time and the query's mean+1.5*interquartile_distance.
A good plan is recorded on a similar condition; The execution time needs 
to be shorter than the minimum of the previously recorded good plan's 
time and the query's mean-1.5*interquartile_distance.


The boundaries are chosen to resemble the boundaries for whiskers in 
boxplots.
Using these boundaries, plans will be updated very seldomly, as long as 
they are more or less normally distributed.
Changes in the plans (for example the use of indices) used for each kind 
of query will most likely result in execution times exceeding these 
boundaries, so such changes are (very probably) recorded.


The ideal solution would be to compare the current plan with the last 
plan and only update when there is a difference between them, however I 
think this is unreasonably complex and a rather expensive task to 
compute on the completion of every query.


The intent of this patch is to provide a quick insight into the plans 
currently used by the database for the execution of certain queries. The 
tracked plans only represent instances of queries with very good or very 
poor performance.


I've (re)submitted this patch for the next commitfest as well.

Kind regards
Julian


On 03/04/2017 02:56 PM, Julian Markwort wrote:
Alright, for the next version of this patch I'll look into standard 
deviation (an implementation of Welfords' algorithm already exists in 
pg_stat_statements).


On 3/4/17 14:18, Peter Eisentraut wrote:


The other problem is that this measures execution time, which can vary
for reasons other than plan.  I would have expected that the cost
numbers are tracked somehow.
I've already thought of tracking specific parts of the explanation, 
like the cost numbers, instead of the whole string, I'll think of 
something, but if anybody has any bright ideas in the meantime, I'd 
gladly listen to them.



There is also the issue of generic vs specific plans, which this
approach might be papering over.
Would you be so kind and elaborate a little bit on this? I'm not sure 
if I understand this correctly. This patch only tracks specific plans, 
yes. The inital idea was that there might be some edge-cases that are 
not apparent when looking at generalized plans or queries.


kind regards
Julian


diff --git a/contrib/pg_stat_statements/Makefile b/contrib/pg_stat_statements/Makefile
index 39b368b70e..4d658d0ec7 100644
--- a/contrib/pg_stat_statements/Makefile
+++ b/contrib/pg_stat_statements/Makefile
@@ -4,7 +4,7 @@ MODULE_big = pg_stat_statements
 OBJS = pg_stat_statements.o $(WIN32RES)
 
 EXTENSION = pg_stat_statements
-DATA = pg_stat_statements--1.4.sql pg_stat_statements--1.4--1.5.sql \
+DATA = pg_stat_statements--1.5.sql pg_stat_statements--1.4--1.5.sql \
 	pg_stat_statements--1.3--1.4.sql pg_stat_statements--1.2--1.3.sql \
 	pg_stat_statements--1.1--1.2.sql pg_stat_statements--1.0--1.1.sql \
 	pg_stat_statements--unpackaged--1.0.sql
diff --git a/contrib/pg_stat_statements/expected/pg_stat_statements.out b/contrib/pg_stat_statements/expected/pg_stat_statements.out
index 5318c3550c..3e79890d50 100644
--- a/contrib/pg_stat_statements/expected/pg_stat_statements.out
+++ b/contrib/pg_stat_statements/expected/pg_stat_statements.out
@@ -395,4 +395,40 @@ SELECT query, calls, rows FROM pg_stat_statements ORDER BY query COLLATE "C";
  SELECT pg_stat_statements_reset() | 1 |1
 (8 rows)
 
+-- test to see if any plans have been recorded.
+SELECT
+  CASE WHEN good_plan_time > 0 THEN 1 ELSE 0 END,
+  CASE WHEN bad_plan_time > 0 THEN 1 ELSE 0 END,
+  CASE WHEN good_plan_timestamp >= timestamp '1970-01-01 00:00:00' THEN 1 ELSE 0 END,
+  CASE WHEN good_plan_timestamp >= timestamp '1970-01-01 00:00:00' THEN 1 ELSE 0 END
+FROM pg_stat_statements ORDER BY query COLLATE "C";
+ case | case | case | case 
+--+--+--+--
+0 |0 |0 |0
+0 |0 |0 |0
+0 |0 |0 |0
+0 |0 |0 |0
+0 |0 |0 |0
+0 |0 |0 |0
+1 |1 |1 |1
+1 |1 |1 |1
+1 |1 |1 |1
+(9 rows)
+
+-- test if there is some text in the recorded plans.
+select substr(good_plan, 0, 11), substr(bad_plan, 0, 11) from pg_stat_statements ORDER BY query COLLATE "C";
+   substr   |   substr   
++
+| 
+| 
+| 
+