Re: [PATCH] Include triggers in EXPLAIN

2019-11-04 Thread Andres Freund
Hi,

(minor note - on PG lists the style is to quote in-line and trip)

On 2019-11-04 10:35:25 +0100, Josef Šimánek wrote:
> Thanks for quick response. As I was testing this feature it shows all
> "possible" triggers to be executed running given query. The benefit of
> having this information in EXPLAIN as well is you do not need to execute
> the query (as EXPLAIN ANALYZE does). My usecase is to take a look at query
> before it is executed to get some idea about the plan with EXPLAIN.

I can actually see some value in additional information here, but I'd
probably want to change the format a bit. When explicitly desired (or
perhaps just in verbose mode?), I see value in counting the number of
triggers we know about that need to be checked, how many were excluded
on the basis of the trigger's WHEN clause etc.


> Do you have idea about some case where actual trigger will be missing in
> EXPLAIN with current implementation, but will be present in EXPLAIN
> ANALYZE? I can take a look if there's any way how to handle those cases as
> well.

Any triggers that are fired because of other, listed, triggers causing
other changes. E.g. a logging trigger that inserts into a log table -
EXPLAIN, without ANALYZE, doesn't have a way of knowing about that.

And before you say that sounds like a niche issue - it's not in my
experience. Forgetting the necessary indexes two or three foreign keys
down a CASCADE chain seems to be more common than doing so for tables
directly "linked" with busy ones.

Greetings,

Andres Freund




Re: [PATCH] Include triggers in EXPLAIN

2019-11-04 Thread Josef Šimánek
Hello Tom.

Thanks for quick response. As I was testing this feature it shows all
"possible" triggers to be executed running given query. The benefit of
having this information in EXPLAIN as well is you do not need to execute
the query (as EXPLAIN ANALYZE does). My usecase is to take a look at query
before it is executed to get some idea about the plan with EXPLAIN.

Do you have idea about some case where actual trigger will be missing in
EXPLAIN with current implementation, but will be present in EXPLAIN
ANALYZE? I can take a look if there's any way how to handle those cases as
well.

ne 3. 11. 2019 v 22:49 odesílatel Tom Lane  napsal:

> =?UTF-8?B?Sm9zZWYgxaBpbcOhbmVr?=  writes:
> > Recently I got few times into situation where I was trying to find out
> what
> > is blocking DELETE queries. Running EXPLAIN (even VERBOSE one) wasn't
> > useful, since the reason was slow trigger (missing index on foreign key
> > column). I had to create testing entry and run EXPLAIN ANALYZE DELETE to
> > get this information.
>
> > It will be really valuable for me to show triggers in EXPLAIN query since
> > it will make clear for me there will be any trigger "activated" during
> > execution of DELETE query and that can be the reason for slow DELETE.
>
> I don't really see the point of this patch?  You do get the trigger
> times during EXPLAIN ANALYZE, and I don't believe that a plain EXPLAIN
> is going to have full information about what triggers might fire or
> not fire.
>
> regards, tom lane
>


Re: [PATCH] Include triggers in EXPLAIN

2019-11-03 Thread Tom Lane
=?UTF-8?B?Sm9zZWYgxaBpbcOhbmVr?=  writes:
> Recently I got few times into situation where I was trying to find out what
> is blocking DELETE queries. Running EXPLAIN (even VERBOSE one) wasn't
> useful, since the reason was slow trigger (missing index on foreign key
> column). I had to create testing entry and run EXPLAIN ANALYZE DELETE to
> get this information.

> It will be really valuable for me to show triggers in EXPLAIN query since
> it will make clear for me there will be any trigger "activated" during
> execution of DELETE query and that can be the reason for slow DELETE.

I don't really see the point of this patch?  You do get the trigger
times during EXPLAIN ANALYZE, and I don't believe that a plain EXPLAIN
is going to have full information about what triggers might fire or
not fire.

regards, tom lane




[PATCH] Include triggers in EXPLAIN

2019-11-03 Thread Josef Šimánek
Hello!

Recently I got few times into situation where I was trying to find out what
is blocking DELETE queries. Running EXPLAIN (even VERBOSE one) wasn't
useful, since the reason was slow trigger (missing index on foreign key
column). I had to create testing entry and run EXPLAIN ANALYZE DELETE to
get this information.

It will be really valuable for me to show triggers in EXPLAIN query since
it will make clear for me there will be any trigger "activated" during
execution of DELETE query and that can be the reason for slow DELETE.

I have seen initial discussion at
https://www.postgresql.org/message-id/flat/20693.732761%40sss.pgh.pa.us
to show time spent in triggers in EXPLAIN ANALYZE including quick
discussion to possibly show triggers during EXPLAIN. Anyway since it
doesn't show any additional cost and just inform about the possibilities, I
still consider this feature useful. This is probably implementation of idea
mentioned at
https://www.postgresql.org/message-id/21221.736869%40sss.pgh.pa.us by
Tom Lane.

After initial discussion with Pavel Stěhule and Tomáš Vondra at czech
postgresql maillist (
https://groups.google.com/forum/#!topic/postgresql-cz/Dq1sT7huVho) I was
able to prepare initial version of this patch. I have added EXPLAIN option
called TRIGGERS enabled by default.There's already autoexplain property for
this. I understand it is not possible to show only triggers which will be
really activated unless query is really executed. EXPLAIN ANALYZE remains
untouched with this patch.

- patch with examples can be found at
https://github.com/simi/postgres/pull/2
- DIFF format https://github.com/simi/postgres/pull/2.diff
- PATCH format (also attached) https://github.com/simi/postgres/pull/2.patch

All regression tests passed with this change locally on latest git master.
I would like to cover this patch with more regression tests, but I wasn't
sure where to place them, since there's no "EXPLAIN" related test "group".
Is "src/test/regress/sql/triggers.sql" the best place to add tests related
to this change?

PS: This is my first try to contribute to postgresql codebase. The quality
of patch is probably not the best, but I will be more than happy to do any
requested change if needed.

Regards,
Josef Šimánek
From 18578e3d07aa159631e0903abae496a6482fa279 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Josef=20=C5=A0im=C3=A1nek?= 
Date: Sat, 27 Jul 2019 09:27:41 +0200
Subject: [PATCH] Show possible triggers in EXPLAIN.

---
 doc/src/sgml/ref/explain.sgml |  9 
 src/backend/commands/explain.c| 43 +++
 src/include/commands/explain.h|  1 +
 src/test/regress/expected/foreign_key.out |  6 ++-
 src/test/regress/expected/insert_conflict.out |  4 +-
 src/test/regress/expected/updatable_views.out | 10 -
 6 files changed, 60 insertions(+), 13 deletions(-)

diff --git a/doc/src/sgml/ref/explain.sgml b/doc/src/sgml/ref/explain.sgml
index 385d10411fa0..ba0d8df88c19 100644
--- a/doc/src/sgml/ref/explain.sgml
+++ b/doc/src/sgml/ref/explain.sgml
@@ -43,6 +43,7 @@ EXPLAIN [ ANALYZE ] [ VERBOSE ] statementboolean ]
 TIMING [ boolean ]
 SUMMARY [ boolean ]
+TRIGGERS [ boolean ]
 FORMAT { TEXT | XML | JSON | YAML }
 
  
@@ -224,6 +225,14 @@ ROLLBACK;
 

 
+   
+TRIGGERS
+
+ 
+   TODO
+ 
+
+   

 FORMAT
 
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index 62fb3434a32f..93845bcaad36 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -149,6 +149,7 @@ ExplainQuery(ParseState *pstate, ExplainStmt *stmt, const char *queryString,
 	ListCell   *lc;
 	bool		timing_set = false;
 	bool		summary_set = false;
+	bool		triggers_set = false;
 
 	/* Parse options list. */
 	foreach(lc, stmt->options)
@@ -175,6 +176,11 @@ ExplainQuery(ParseState *pstate, ExplainStmt *stmt, const char *queryString,
 			summary_set = true;
 			es->summary = defGetBoolean(opt);
 		}
+		else if (strcmp(opt->defname, "triggers") == 0)
+		{
+			triggers_set = true;
+			es->triggers = defGetBoolean(opt);
+		}
 		else if (strcmp(opt->defname, "format") == 0)
 		{
 			char	   *p = defGetString(opt);
@@ -210,6 +216,9 @@ ExplainQuery(ParseState *pstate, ExplainStmt *stmt, const char *queryString,
 	/* if the timing was not set explicitly, set default value */
 	es->timing = (timing_set) ? es->timing : es->analyze;
 
+	/* if the triggers was not set explicitly, set default value */
+	es->triggers = (triggers_set) ? es->triggers : true;
+
 	/* check that timing is used with EXPLAIN ANALYZE */
 	if (es->timing && !es->analyze)
 		ereport(ERROR,
@@ -556,7 +565,7 @@ ExplainOnePlan(PlannedStmt *plannedstmt, IntoClause *into, ExplainState *es,
 	}
 
 	/* Print info about runtime of triggers */
-	if (es->analyze)
+	if (es->triggers)
 		ExplainPrintTriggers(es, queryDesc);
 
 	/*
@@ -911,17 +920,23 @@ report_triggers(ResultRelInfo *rInfo, bool show_relname, ExplainState *es)
 {
 	i