Thread: Make EXPLAIN generate a generic plan for a parameterized query
Today you get
test=> EXPLAIN SELECT * FROM tab WHERE col = $1;
ERROR: there is no parameter $1
which makes sense. Nonetheless, it would be great to get a generic plan
for such a query. Sometimes you don't have the parameters (if you grab
the statement from "pg_stat_statements", or if it is from an error message
in the log, and you didn't enable "log_parameter_max_length_on_error").
Sometimes it is just very painful to substitute the 25 parameters from
the detail message.
With the attached patch you can get the following:
test=> SET plan_cache_mode = force_generic_plan;
SET
test=> EXPLAIN (COSTS OFF) SELECT * FROM pg_proc WHERE oid = $1;
QUERY PLAN
═══════════════════════════════════════════════
Index Scan using pg_proc_oid_index on pg_proc
Index Cond: (oid = $1)
(2 rows)
That's not the same as a full-fledged EXPLAIN (ANALYZE, BUFFERS),
but it can definitely be helpful.
I tied that behavior to the setting of "plan_cache_mode" where you
are guaranteed to get a generic plan; I couldn't think of a better way.
Yours,
Laurenz Albe
Attachment
Laurenz Albe <laurenz.albe@cybertec.at> writes:
> Today you get
> test=> EXPLAIN SELECT * FROM tab WHERE col = $1;
> ERROR: there is no parameter $1
> which makes sense. Nonetheless, it would be great to get a generic plan
> for such a query.
I can see the point, but it also seems like it risks masking stupid
mistakes.
> I tied that behavior to the setting of "plan_cache_mode" where you
> are guaranteed to get a generic plan; I couldn't think of a better way.
I think it might be better to drive it off an explicit EXPLAIN option,
perhaps
EXPLAIN (GENERIC_PLAN) SELECT * FROM tab WHERE col = $1;
This option (bikeshedding on the name welcome) would have the effect
both of allowing unanchored Param symbols and of temporarily forcing
generic-plan mode, so that you don't need additional commands to
set and reset plan_cache_mode. We could also trivially add logic
to disallow the combination of ANALYZE and GENERIC_PLAN, which
would otherwise be a bit messy to prevent.
For context, it does already work to do this when you want to
investigate parameterized plans:
regression=# prepare foo as select * from tenk1 where unique1 = $1;
PREPARE
regression=# explain execute foo(42);
QUERY PLAN
-----------------------------------------------------------------------------
Index Scan using tenk1_unique1 on tenk1 (cost=0.29..8.30 rows=1 width=244)
Index Cond: (unique1 = 42)
(2 rows)
If you're trying to investigate custom-plan behavior, then you
need to supply concrete parameter values somewhere, so I think
this approach is fine for that case. (Shoehorning parameter
values into EXPLAIN options seems like it'd be a bit much.)
However, investigating generic-plan behavior this way is tedious,
since you have to invent irrelevant parameter values, plus mess
with plan_cache_mode or else run the explain half a dozen times.
So I can get behind having a more convenient way for that.
regards, tom lane
On Tue, Oct 11, 2022 at 09:49:14AM -0400, Tom Lane wrote: > > If you're trying to investigate custom-plan behavior, then you > need to supply concrete parameter values somewhere, so I think > this approach is fine for that case. (Shoehorning parameter > values into EXPLAIN options seems like it'd be a bit much.) > However, investigating generic-plan behavior this way is tedious, > since you have to invent irrelevant parameter values, plus mess > with plan_cache_mode or else run the explain half a dozen times. > So I can get behind having a more convenient way for that. One common use case is tools identifying a slow query using pg_stat_statements, identifying some missing indexes and then wanting to check whether the index should be useful using some hypothetical index. FTR I'm working on such a project and for now we have to go to great lengths trying to "unjumble" such queries, so having a way to easily get the answer for a generic plan would be great.
On Wed, 2022-10-12 at 00:03 +0800, Julien Rouhaud wrote: > On Tue, Oct 11, 2022 at 09:49:14AM -0400, Tom Lane wrote: > > I think it might be better to drive it off an explicit EXPLAIN option, > > perhaps > > > > EXPLAIN (GENERIC_PLAN) SELECT * FROM tab WHERE col = $1; > > > > If you're trying to investigate custom-plan behavior, then you > > need to supply concrete parameter values somewhere, so I think > > this approach is fine for that case. (Shoehorning parameter > > values into EXPLAIN options seems like it'd be a bit much.) > > However, investigating generic-plan behavior this way is tedious, > > since you have to invent irrelevant parameter values, plus mess > > with plan_cache_mode or else run the explain half a dozen times. > > So I can get behind having a more convenient way for that. > > One common use case is tools identifying a slow query using pg_stat_statements, > identifying some missing indexes and then wanting to check whether the index > should be useful using some hypothetical index. > > FTR I'm working on such a project and for now we have to go to great lengths > trying to "unjumble" such queries, so having a way to easily get the answer for > a generic plan would be great. Thanks for the suggestions and the encouragement. Here is a patch that implements it with an EXPLAIN option named GENERIC_PLAN. Yours, Laurenz Albe
Attachment
Hi, On Tue, Oct 25, 2022 at 11:08:27AM +0200, Laurenz Albe wrote: > > Here is a patch that > implements it with an EXPLAIN option named GENERIC_PLAN. I only have a quick look at the patch for now. Any reason why you don't rely on the existing explain_filter() function for emitting stable output (without having to remove the costs)? It would also take care of checking that it works in plpgsql.
On Tue, 2022-10-25 at 19:03 +0800, Julien Rouhaud wrote: > On Tue, Oct 25, 2022 at 11:08:27AM +0200, Laurenz Albe wrote: > > Here is a patch that > > implements it with an EXPLAIN option named GENERIC_PLAN. > > I only have a quick look at the patch for now. Any reason why you don't rely > on the existing explain_filter() function for emitting stable output (without > having to remove the costs)? It would also take care of checking that it works > in plpgsql. No, there is no principled reason I did it like that. Version 2 does it like you suggest. Yours, Laurenz Albe
Attachment
Hi, On 2022-10-29 10:35:26 +0200, Laurenz Albe wrote: > On Tue, 2022-10-25 at 19:03 +0800, Julien Rouhaud wrote: > > On Tue, Oct 25, 2022 at 11:08:27AM +0200, Laurenz Albe wrote: > > > Here is a patch that > > > implements it with an EXPLAIN option named GENERIC_PLAN. > > > > I only have a quick look at the patch for now. Any reason why you don't rely > > on the existing explain_filter() function for emitting stable output (without > > having to remove the costs)? It would also take care of checking that it works > > in plpgsql. > > No, there is no principled reason I did it like that. Version 2 does it like > you suggest. This fails to build the docs: https://cirrus-ci.com/task/5609301511766016 [17:47:01.064] ref/explain.sgml:179: parser error : Opening and ending tag mismatch: likeral line 179 and literal [17:47:01.064] <likeral>ANALYZE</literal>, since a statement with unknown parameters [17:47:01.064] ^ Greetings, Andres Freund
On Tue, 2022-12-06 at 10:17 -0800, Andres Freund wrote: > On 2022-10-29 10:35:26 +0200, Laurenz Albe wrote: > > > > Here is a patch that > > > > implements it with an EXPLAIN option named GENERIC_PLAN. > > This fails to build the docs: > > https://cirrus-ci.com/task/5609301511766016 > > [17:47:01.064] ref/explain.sgml:179: parser error : Opening and ending tag mismatch: likeral line 179 and literal > [17:47:01.064] <likeral>ANALYZE</literal>, since a statement with unknown parameters > [17:47:01.064] ^ *blush* Here is a fixed version. Yours, Laurenz Albe
Attachment
On Wed, Dec 7, 2022 at 3:23 AM Laurenz Albe <laurenz.albe@cybertec.at> wrote:
On Tue, 2022-12-06 at 10:17 -0800, Andres Freund wrote:
> On 2022-10-29 10:35:26 +0200, Laurenz Albe wrote:
> > > > Here is a patch that
> > > > implements it with an EXPLAIN option named GENERIC_PLAN.
>
> This fails to build the docs:
>
> https://cirrus-ci.com/task/5609301511766016
>
> [17:47:01.064] ref/explain.sgml:179: parser error : Opening and ending tag mismatch: likeral line 179 and literal
> [17:47:01.064] <likeral>ANALYZE</literal>, since a statement with unknown parameters
> [17:47:01.064] ^
*blush* Here is a fixed version.
analyze.c: In function 'transformStmt':
analyze.c:2919:35: warning: 'generic_plan' may be used uninitialized in this function [-Wmaybe-uninitialized]
2919 | pstate->p_generic_explain = generic_plan;
| ~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~
analyze.c:2909:25: note: 'generic_plan' was declared here
2909 | bool generic_plan;
| ^~~~~~~~~~~~
analyze.c:2919:35: warning: 'generic_plan' may be used uninitialized in this function [-Wmaybe-uninitialized]
2919 | pstate->p_generic_explain = generic_plan;
| ~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~
analyze.c:2909:25: note: 'generic_plan' was declared here
2909 | bool generic_plan;
| ^~~~~~~~~~~~
On Tue, 2022-12-27 at 14:37 -0800, Michel Pelletier wrote: > I built and tested this patch for review and it works well, although I got the following warning when building: > > analyze.c: In function 'transformStmt': > analyze.c:2919:35: warning: 'generic_plan' may be used uninitialized in this function [-Wmaybe-uninitialized] > 2919 | pstate->p_generic_explain = generic_plan; > | ~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~ > analyze.c:2909:25: note: 'generic_plan' was declared here > 2909 | bool generic_plan; > | ^~~~~~~~~~~~ Thanks for checking. The variable should indeed be initialized, although my compiler didn't complain. Attached is a fixed version. Yours, Laurenz Albe
Attachment
Hi Laurenz, I'm testing your patch and the GENERIC_PLAN parameter seems to work just OK .. db=# CREATE TABLE t (col numeric); CREATE TABLE db=# CREATE INDEX t_col_idx ON t (col); CREATE INDEX db=# INSERT INTO t SELECT random() FROM generate_series(1,100000) ; INSERT 0 100000 db=# EXPLAIN (GENERIC_PLAN) SELECT * FROM t WHERE col = $1; QUERY PLAN --------------------------------------------------------------------------- Bitmap Heap Scan on t (cost=15.27..531.67 rows=368 width=32) Recheck Cond: (col = $1) -> Bitmap Index Scan on t_col_idx (cost=0.00..15.18 rows=368 width=0) Index Cond: (col = $1) (4 rows) .. the error message when combining GENERIC_PLAN with ANALYSE also works as expected db=# EXPLAIN (ANALYSE, GENERIC_PLAN) SELECT * FROM t WHERE col = $1; ERROR: EXPLAIN ANALYZE cannot be used with GENERIC_PLAN .. and the system also does not throw an error when it's used along other parameters, e.g. VERBOSE, WAL, SUMMARY, etc. However, when GENERIC_PLAN is used combined with BUFFERS, the 'Buffers' node is shown the first time the query executed in a session: psql (16devel) Type "help" for help. postgres=# \c db You are now connected to database "db" as user "postgres". db=# EXPLAIN (BUFFERS, GENERIC_PLAN) SELECT * FROM t WHERE col = $1; QUERY PLAN ------------------------------------------------------------------------- Index Only Scan using t_col_idx on t (cost=0.42..4.44 rows=1 width=11) Index Cond: (col = $1) Planning: Buffers: shared hit=62 (4 rows) db=# EXPLAIN (BUFFERS, GENERIC_PLAN) SELECT * FROM t WHERE col = $1; QUERY PLAN ------------------------------------------------------------------------- Index Only Scan using t_col_idx on t (cost=0.42..4.44 rows=1 width=11) Index Cond: (col = $1) (2 rows) db=# EXPLAIN (BUFFERS, GENERIC_PLAN) SELECT * FROM t WHERE col = $1; QUERY PLAN ------------------------------------------------------------------------- Index Only Scan using t_col_idx on t (cost=0.42..4.44 rows=1 width=11) Index Cond: (col = $1) (2 rows) Is it the expected behaviour? Also, this new parameter seems only to work between parenthesis `(GENERIC_PLAN)`: db=# EXPLAIN GENERIC_PLAN SELECT * FROM t WHERE col = $1; ERROR: syntax error at or near "GENERIC_PLAN" LINE 1: EXPLAIN GENERIC_PLAN SELECT * FROM t WHERE col = $1; If it's intended to be consistent with the other "single parameters", perhaps it should work also without parenthesis? e.g. db=# EXPLAIN ANALYSE SELECT * FROM t WHERE col < 0.42; QUERY PLAN ------------------------------------------------------------------------------------------------------------------------------ Index Only Scan using t_col_idx on t (cost=0.42..1637.25 rows=41876 width=11) (actual time=0.103..6.293 rows=41932 loops=1) Index Cond: (col < 0.42) Heap Fetches: 0 Planning Time: 0.071 ms Execution Time: 7.316 ms (5 rows) db=# EXPLAIN VERBOSE SELECT * FROM t WHERE col < 0.42; QUERY PLAN --------------------------------------------------------------------------------------- Index Only Scan using t_col_idx on public.t (cost=0.42..1637.25 rows=41876 width=11) Output: col Index Cond: (t.col < 0.42) (3 rows) On a very personal note: wouldn't just GENERIC (without _PLAN) suffice? Don't bother with it if you disagree :-) Cheers Jim On 09.01.23 17:40, Laurenz Albe wrote: > On Tue, 2022-12-27 at 14:37 -0800, Michel Pelletier wrote: >> I built and tested this patch for review and it works well, although I got the following warning when building: >> >> analyze.c: In function 'transformStmt': >> analyze.c:2919:35: warning: 'generic_plan' may be used uninitialized in this function [-Wmaybe-uninitialized] >> 2919 | pstate->p_generic_explain = generic_plan; >> | ~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~ >> analyze.c:2909:25: note: 'generic_plan' was declared here >> 2909 | bool generic_plan; >> | ^~~~~~~~~~~~ > Thanks for checking. The variable should indeed be initialized, although > my compiler didn't complain. > > Attached is a fixed version. > > Yours, > Laurenz Albe
Jim Jones <jim.jones@uni-muenster.de> writes:
> However, when GENERIC_PLAN is used combined with BUFFERS, the 'Buffers'
> node is shown the first time the query executed in a session:
> psql (16devel)
> Type "help" for help.
> postgres=# \c db
> You are now connected to database "db" as user "postgres".
> db=# EXPLAIN (BUFFERS, GENERIC_PLAN) SELECT * FROM t WHERE col = $1;
> QUERY PLAN
> -------------------------------------------------------------------------
> Index Only Scan using t_col_idx on t (cost=0.42..4.44 rows=1 width=11)
> Index Cond: (col = $1)
> Planning:
> Buffers: shared hit=62
> (4 rows)
> db=# EXPLAIN (BUFFERS, GENERIC_PLAN) SELECT * FROM t WHERE col = $1;
> QUERY PLAN
> -------------------------------------------------------------------------
> Index Only Scan using t_col_idx on t (cost=0.42..4.44 rows=1 width=11)
> Index Cond: (col = $1)
> (2 rows)
That's not new to this patch, the same thing happens without it.
It's reflecting catalog accesses involved in loading per-session
caches (which, therefore, needn't be repeated the second time).
> Also, this new parameter seems only to work between parenthesis
> `(GENERIC_PLAN)`:
> db=# EXPLAIN GENERIC_PLAN SELECT * FROM t WHERE col = $1;
> ERROR: syntax error at or near "GENERIC_PLAN"
> LINE 1: EXPLAIN GENERIC_PLAN SELECT * FROM t WHERE col = $1;
That's true of all but the oldest EXPLAIN options. We don't do that
anymore because new options would have to become grammar keywords
to support that.
> On a very personal note: wouldn't just GENERIC (without _PLAN) suffice?
> Don't bother with it if you disagree :-)
FWIW, "GENERIC" would be too generic for my taste ;-). But I agree
it's a judgement call.
regards, tom lane
Laurenz Albe <laurenz.albe@cybertec.at> writes:
> [ 0001-Add-EXPLAIN-option-GENERIC_PLAN.v4.patch ]
I took a closer look at this patch, and didn't like the implementation
much. You're not matching the behavior of PREPARE at all: for example,
this patch is content to let $1 be resolved with different types in
different places. We should be using the existing infrastructure that
parse_analyze_varparams uses.
Also, I believe that in contexts such as plpgsql, it is possible that
there's an external source of $N definitions, which we should probably
continue to honor even with GENERIC_PLAN.
So that leads me to think the code should be more like this. I'm not
sure if it's worth spending documentation and testing effort on the
case where we don't override an existing p_paramref_hook.
regards, tom lane
diff --git a/doc/src/sgml/ref/explain.sgml b/doc/src/sgml/ref/explain.sgml
index d4895b9d7d..58350624e7 100644
--- a/doc/src/sgml/ref/explain.sgml
+++ b/doc/src/sgml/ref/explain.sgml
@@ -40,6 +40,7 @@ EXPLAIN [ ANALYZE ] [ VERBOSE ] <replaceable class="parameter">statement</replac
VERBOSE [ <replaceable class="parameter">boolean</replaceable> ]
COSTS [ <replaceable class="parameter">boolean</replaceable> ]
SETTINGS [ <replaceable class="parameter">boolean</replaceable> ]
+ GENERIC_PLAN [ <replaceable class="parameter">boolean</replaceable> ]
BUFFERS [ <replaceable class="parameter">boolean</replaceable> ]
WAL [ <replaceable class="parameter">boolean</replaceable> ]
TIMING [ <replaceable class="parameter">boolean</replaceable> ]
@@ -167,6 +168,20 @@ ROLLBACK;
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><literal>GENERIC_PLAN</literal></term>
+ <listitem>
+ <para>
+ Generate a generic plan for the statement (see <xref linkend="sql-prepare"/>
+ for details about generic plans). The statement can contain parameter
+ placeholders like <literal>$1</literal>, if it is a statement that can
+ use parameters. This option cannot be used together with
+ <literal>ANALYZE</literal>, since a statement with unknown parameters
+ cannot be executed.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry>
<term><literal>BUFFERS</literal></term>
<listitem>
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index 35c23bd27d..ab21a67862 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -190,6 +190,8 @@ ExplainQuery(ParseState *pstate, ExplainStmt *stmt,
es->wal = defGetBoolean(opt);
else if (strcmp(opt->defname, "settings") == 0)
es->settings = defGetBoolean(opt);
+ else if (strcmp(opt->defname, "generic_plan") == 0)
+ es->generic = defGetBoolean(opt);
else if (strcmp(opt->defname, "timing") == 0)
{
timing_set = true;
@@ -227,6 +229,13 @@ ExplainQuery(ParseState *pstate, ExplainStmt *stmt,
parser_errposition(pstate, opt->location)));
}
+ /* check that GENERIC_PLAN is not used with EXPLAIN ANALYZE */
+ if (es->generic && es->analyze)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("EXPLAIN ANALYZE cannot be used with GENERIC_PLAN")));
+
+ /* check that WAL is used with EXPLAIN ANALYZE */
if (es->wal && !es->analyze)
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c
index e892df9819..9143964e78 100644
--- a/src/backend/parser/analyze.c
+++ b/src/backend/parser/analyze.c
@@ -27,6 +27,7 @@
#include "access/sysattr.h"
#include "catalog/pg_proc.h"
#include "catalog/pg_type.h"
+#include "commands/defrem.h"
#include "miscadmin.h"
#include "nodes/makefuncs.h"
#include "nodes/nodeFuncs.h"
@@ -2906,10 +2907,38 @@ static Query *
transformExplainStmt(ParseState *pstate, ExplainStmt *stmt)
{
Query *result;
+ bool generic_plan = false;
+ Oid *paramTypes = NULL;
+ int numParams = 0;
+
+ /*
+ * If we have no external source of parameter definitions, and the
+ * GENERIC_PLAN option is specified, then accept variable parameter
+ * definitions (as occurs in PREPARE, for example).
+ */
+ if (pstate->p_paramref_hook == NULL)
+ {
+ ListCell *lc;
+
+ foreach(lc, stmt->options)
+ {
+ DefElem *opt = (DefElem *) lfirst(lc);
+
+ if (strcmp(opt->defname, "generic_plan") == 0)
+ generic_plan = defGetBoolean(opt);
+ /* don't "break", as we want the last value */
+ }
+ if (generic_plan)
+ setup_parse_variable_parameters(pstate, ¶mTypes, &numParams);
+ }
/* transform contained query, allowing SELECT INTO */
stmt->query = (Node *) transformOptionalSelectInto(pstate, stmt->query);
+ /* make sure all is well with parameter types */
+ if (generic_plan)
+ check_variable_parameters(pstate, (Query *) stmt->query);
+
/* represent the command as a utility Query */
result = makeNode(Query);
result->commandType = CMD_UTILITY;
diff --git a/src/include/commands/explain.h b/src/include/commands/explain.h
index 7c1071ddd1..3d3e632a0c 100644
--- a/src/include/commands/explain.h
+++ b/src/include/commands/explain.h
@@ -46,6 +46,7 @@ typedef struct ExplainState
bool timing; /* print detailed node timing */
bool summary; /* print total planning and execution timing */
bool settings; /* print modified settings */
+ bool generic; /* generate a generic plan */
ExplainFormat format; /* output format */
/* state for output formatting --- not reset for each new plan tree */
int indent; /* current indentation level */
diff --git a/src/test/regress/expected/explain.out b/src/test/regress/expected/explain.out
index 48620edbc2..18f7ac93c4 100644
--- a/src/test/regress/expected/explain.out
+++ b/src/test/regress/expected/explain.out
@@ -517,3 +517,17 @@ select explain_filter('explain (verbose) select * from int8_tbl i8');
Query Identifier: N
(3 rows)
+-- Test EXPLAIN (GENERIC_PLAN)
+select explain_filter('explain (generic_plan) select unique1 from tenk1 where thousand = $1');
+ explain_filter
+---------------------------------------------------------------------------------
+ Bitmap Heap Scan on tenk1 (cost=N.N..N.N rows=N width=N)
+ Recheck Cond: (thousand = $N)
+ -> Bitmap Index Scan on tenk1_thous_tenthous (cost=N.N..N.N rows=N width=N)
+ Index Cond: (thousand = $N)
+(4 rows)
+
+-- should fail
+select explain_filter('explain (analyze, generic_plan) select unique1 from tenk1 where thousand = $1');
+ERROR: EXPLAIN ANALYZE cannot be used with GENERIC_PLAN
+CONTEXT: PL/pgSQL function explain_filter(text) line 5 at FOR over EXECUTE statement
diff --git a/src/test/regress/sql/explain.sql b/src/test/regress/sql/explain.sql
index ae3f7a308d..fce031775a 100644
--- a/src/test/regress/sql/explain.sql
+++ b/src/test/regress/sql/explain.sql
@@ -128,3 +128,8 @@ select explain_filter('explain (verbose) select * from t1 where pg_temp.mysin(f1
-- Test compute_query_id
set compute_query_id = on;
select explain_filter('explain (verbose) select * from int8_tbl i8');
+
+-- Test EXPLAIN (GENERIC_PLAN)
+select explain_filter('explain (generic_plan) select unique1 from tenk1 where thousand = $1');
+-- should fail
+select explain_filter('explain (analyze, generic_plan) select unique1 from tenk1 where thousand = $1');
On Tue, 2023-01-31 at 13:49 -0500, Tom Lane wrote: > Laurenz Albe <laurenz.albe@cybertec.at> writes: > > [ 0001-Add-EXPLAIN-option-GENERIC_PLAN.v4.patch ] > > I took a closer look at this patch, and didn't like the implementation > much. You're not matching the behavior of PREPARE at all: for example, > this patch is content to let $1 be resolved with different types in > different places. We should be using the existing infrastructure that > parse_analyze_varparams uses. > > Also, I believe that in contexts such as plpgsql, it is possible that > there's an external source of $N definitions, which we should probably > continue to honor even with GENERIC_PLAN. > > So that leads me to think the code should be more like this. I'm not > sure if it's worth spending documentation and testing effort on the > case where we don't override an existing p_paramref_hook. Thanks, that looks way cleaner. I played around with it, and I ran into a problem with partitions that are foreign tables: CREATE TABLE loc1 (id integer NOT NULL, key integer NOT NULL CHECK (key = 1), value text); CREATE TABLE loc2 (id integer NOT NULL, key integer NOT NULL CHECK (key = 2), value text); CREATE TABLE looppart (id integer GENERATED ALWAYS AS IDENTITY, key integer NOT NULL, value text) PARTITION BY LIST (key); CREATE FOREIGN TABLE looppart1 PARTITION OF looppart FOR VALUES IN (1) SERVER loopback OPTIONS (table_name 'loc1'); CREATE FOREIGN TABLE looppart2 PARTITION OF looppart FOR VALUES IN (2) SERVER loopback OPTIONS (table_name 'loc2'); EXPLAIN (GENERIC_PLAN) SELECT * FROM looppart WHERE key = $1; ERROR: no value found for parameter 1 The solution could be to set up a dynamic parameter hook in the ExprContext in ecxt_param_list_info->paramFetch so that ExecEvalParamExtern doesn't complain about the missing parameter. Does that make sense? How do I best hook into the executor to set that up? Yours, Laurenz Albe
Laurenz Albe <laurenz.albe@cybertec.at> writes:
> I played around with it, and I ran into a problem with partitions that
> are foreign tables:
> ...
> EXPLAIN (GENERIC_PLAN) SELECT * FROM looppart WHERE key = $1;
> ERROR: no value found for parameter 1
Hmm, offhand I'd say that something is doing something it has no
business doing when EXEC_FLAG_EXPLAIN_ONLY is set (that is, premature
evaluation of an expression). I wonder whether this failure is
reachable without this patch.
regards, tom lane
On Fri, 2023-02-03 at 09:44 -0500, Tom Lane wrote: > Laurenz Albe <laurenz.albe@cybertec.at> writes: > > I played around with it, and I ran into a problem with partitions that > > are foreign tables: > > ... > > EXPLAIN (GENERIC_PLAN) SELECT * FROM looppart WHERE key = $1; > > ERROR: no value found for parameter 1 > > Hmm, offhand I'd say that something is doing something it has no > business doing when EXEC_FLAG_EXPLAIN_ONLY is set (that is, premature > evaluation of an expression). I wonder whether this failure is > reachable without this patch. Thanks for the pointer. Perhaps something like the attached? The changes in "CreatePartitionPruneState" make my test case work, but they cause a difference in the regression tests, which would be intended if we have no partition pruning with plain EXPLAIN. Yours, Laurenz Albe
Attachment
Laurenz Albe <laurenz.albe@cybertec.at> writes:
> Thanks for the pointer. Perhaps something like the attached?
> The changes in "CreatePartitionPruneState" make my test case work,
> but they cause a difference in the regression tests, which would be
> intended if we have no partition pruning with plain EXPLAIN.
Hmm, so I see (and the cfbot entry for this patch is now all red,
because you didn't include that diff in the patch).
I'm not sure if we can get away with that behavioral change.
People are probably expecting the current behavior for existing
cases.
I can think of a couple of possible ways forward:
* Fix things so that the generic parameters appear to have NULL
values when inspected during executor startup. I'm not sure
how useful that'd be though. In partition-pruning cases that'd
lead to EXPLAIN (GENERIC_PLAN) showing the plan with all
partitions pruned, other than the one for NULL values if there
is one. Doesn't sound too helpful.
* Invent another executor flag that's a "stronger" version of
EXEC_FLAG_EXPLAIN_ONLY, and pass that when any generic parameters
exist, and check it in CreatePartitionPruneState to decide whether
to do startup pruning. This avoids changing behavior for existing
cases, but I'm not sure how much it has to recommend it otherwise.
Skipping the startup prune step seems like it'd produce misleading
results in another way, ie showing too many partitions not too few.
This whole business of partition pruning dependent on the
generic parameters is making me uncomfortable. It seems like
we *can't* show a plan that is either representative of real
execution or comparable to what you get from more-traditional
EXPLAIN usage. Maybe we need to step back and think more.
regards, tom lane
On Fri, 2023-02-03 at 15:11 -0500, Tom Lane wrote: > I can think of a couple of possible ways forward: > > * Fix things so that the generic parameters appear to have NULL > values when inspected during executor startup. I'm not sure > how useful that'd be though. In partition-pruning cases that'd > lead to EXPLAIN (GENERIC_PLAN) showing the plan with all > partitions pruned, other than the one for NULL values if there > is one. Doesn't sound too helpful. > > * Invent another executor flag that's a "stronger" version of > EXEC_FLAG_EXPLAIN_ONLY, and pass that when any generic parameters > exist, and check it in CreatePartitionPruneState to decide whether > to do startup pruning. This avoids changing behavior for existing > cases, but I'm not sure how much it has to recommend it otherwise. > Skipping the startup prune step seems like it'd produce misleading > results in another way, ie showing too many partitions not too few. > > This whole business of partition pruning dependent on the > generic parameters is making me uncomfortable. It seems like > we *can't* show a plan that is either representative of real > execution or comparable to what you get from more-traditional > EXPLAIN usage. Maybe we need to step back and think more. I slept over it, and the second idea now looks like the the right approach to me. My idea of seeing a generic plan is that plan-time partition pruning happens, but not execution-time pruning, so that I get no "subplans removed". Are there any weird side effects of skipping the startup prune step? Anyway, attached is v7 that tries to do it that way. It feels fairly good to me. I invented a new executor flag EXEC_FLAG_EXPLAIN_GENERIC. To avoid having to change all the places that check EXEC_FLAG_EXPLAIN_ONLY to also check for the new flag, I decided that the new flag can only be used as "add-on" to EXEC_FLAG_EXPLAIN_ONLY. Yours, Laurenz Albe
Attachment
Hi, On 2023-02-05 18:24:03 +0100, Laurenz Albe wrote: > Anyway, attached is v7 that tries to do it that way. This consistently fails on CI: https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest%2F42%2F3962 https://api.cirrus-ci.com/v1/artifact/task/5156723929907200/testrun/build/testrun/regress/regress/regression.diffs Greetings, Andres Freund
On Mon, 2023-02-13 at 16:33 -0800, Andres Freund wrote: > On 2023-02-05 18:24:03 +0100, Laurenz Albe wrote: > > Anyway, attached is v7 that tries to do it that way. > > This consistently fails on CI: > https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest%2F42%2F3962 > > https://api.cirrus-ci.com/v1/artifact/task/5156723929907200/testrun/build/testrun/regress/regress/regression.diffs Thanks for pointing out. Here is an improved version. Yours, Laurenz Albe
Attachment
Hi,
I reviewed the patch and find the idea of allowing $placeholders with
EXPLAIN very useful, it will solve relevant real-world use-cases.
(Queries from pg_stat_statements, found in the log, or in application
code.)
I have some comments:
> This allows EXPLAIN to generate generic plans for parameterized statements
> (that have parameter placeholders like $1 in the statement text).
> + <varlistentry>
> + <term><literal>GENERIC_PLAN</literal></term>
> + <listitem>
> + <para>
> + Generate a generic plan for the statement (see <xref linkend="sql-prepare"/>
> + for details about generic plans). The statement can contain parameter
> + placeholders like <literal>$1</literal> (but then it has to be a statement
> + that supports parameters). This option cannot be used together with
> + <literal>ANALYZE</literal>, since a statement with unknown parameters
> + cannot be executed.
Like in the commit message quoted above, I would put more emphasis on
"parameterized query" here:
Allow the statement to contain parameter placeholders like
<literal>$1</literal> and generate a generic plan for it.
This option cannot be used together with <literal>ANALYZE</literal>.
> + /* check that GENERIC_PLAN is not used with EXPLAIN ANALYZE */
> + if (es->generic && es->analyze)
> + ereport(ERROR,
> + (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> + errmsg("EXPLAIN ANALYZE cannot be used with GENERIC_PLAN")));
To put that in line with the other error messages in that context, I'd
inject an extra "option":
errmsg("EXPLAIN option ANALYZE cannot be used with GENERIC_PLAN")));
> --- a/src/test/regress/sql/explain.sql
> +++ b/src/test/regress/sql/explain.sql
> @@ -128,3 +128,33 @@ select explain_filter('explain (verbose) select * from t1 where pg_temp.mysin(f1
> -- Test compute_query_id
> set compute_query_id = on;
> select explain_filter('explain (verbose) select * from int8_tbl i8');
> +
> +-- Test EXPLAIN (GENERIC_PLAN)
> +select explain_filter('explain (generic_plan) select unique1 from tenk1 where thousand = $1');
> +-- should fail
> +select explain_filter('explain (analyze, generic_plan) select unique1 from tenk1 where thousand = $1');
> +-- Test EXPLAIN (GENERIC_PLAN) with partition pruning
> +-- should prune at plan time, but not at execution time
> +create extension if not exists postgres_fdw;
"create extension postgres_fdw" cannot be used from src/test/regress/
since contrib/ might not have been built.
> +create server loop42 foreign data wrapper postgres_fdw;
> +create user mapping for current_role server loop42 options (password_required 'false');
> +create table gen_part (
> + key1 integer not null,
> + key2 integer not null
> +) partition by list (key1);
> +create table gen_part_1
> + partition of gen_part for values in (1)
> + partition by range (key2);
> +create foreign table gen_part_1_1
> + partition of gen_part_1 for values from (1) to (2)
> + server loop42 options (table_name 'whatever_1_1');
> +create foreign table gen_part_1_2
> + partition of gen_part_1 for values from (2) to (3)
> + server loop42 options (table_name 'whatever_1_2');
> +create foreign table gen_part_2
> + partition of gen_part for values in (2)
> + server loop42 options (table_name 'whatever_2');
> +select explain_filter('explain (generic_plan) select key1, key2 from gen_part where key1 = 1 and key2 = $1');
I suggest leaving this test in place here, but with local tables (to
show that plan time pruning using the one provided parameter works),
and add a comment here explaining that is being tested:
-- create a partition hierarchy to show that plan time pruning removes
-- the key1=2 table but generates a generic plan for key2=$1
The test involving postgres_fdw is still necessary to exercise the new
EXEC_FLAG_EXPLAIN_GENERIC code path, but needs to be moved elsewhere,
probably src/test/modules/.
In the new location, likewise add a comment why this test needs to
look this way.
Christoph
Thanks for the review!
On Tue, 2023-03-21 at 16:32 +0100, Christoph Berg wrote:
> I have some comments:
>
> > This allows EXPLAIN to generate generic plans for parameterized statements
> > (that have parameter placeholders like $1 in the statement text).
>
> > + <varlistentry>
> > + <term><literal>GENERIC_PLAN</literal></term>
> > + <listitem>
> > + <para>
> > + Generate a generic plan for the statement (see <xref linkend="sql-prepare"/>
> > + for details about generic plans). The statement can contain parameter
> > + placeholders like <literal>$1</literal> (but then it has to be a statement
> > + that supports parameters). This option cannot be used together with
> > + <literal>ANALYZE</literal>, since a statement with unknown parameters
> > + cannot be executed.
>
> Like in the commit message quoted above, I would put more emphasis on
> "parameterized query" here:
>
> Allow the statement to contain parameter placeholders like
> <literal>$1</literal> and generate a generic plan for it.
> This option cannot be used together with <literal>ANALYZE</literal>.
I went with
Allow the statement to contain parameter placeholders like
<literal>$1</literal> and generate a generic plan for it.
See <xref linkend="sql-prepare"/> for details about generic plans
and the statements that support parameters.
This option cannot be used together with <literal>ANALYZE</literal>.
> > + /* check that GENERIC_PLAN is not used with EXPLAIN ANALYZE */
> > + if (es->generic && es->analyze)
> > + ereport(ERROR,
> > + (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> > + errmsg("EXPLAIN ANALYZE cannot be used with GENERIC_PLAN")));
>
> To put that in line with the other error messages in that context, I'd
> inject an extra "option":
>
> errmsg("EXPLAIN option ANALYZE cannot be used with GENERIC_PLAN")));
Done.
> > --- a/src/test/regress/sql/explain.sql
> > +++ b/src/test/regress/sql/explain.sql
> > [...]
> > +create extension if not exists postgres_fdw;
>
> "create extension postgres_fdw" cannot be used from src/test/regress/
> since contrib/ might not have been built.
Ouch. Good catch.
> I suggest leaving this test in place here, but with local tables (to
> show that plan time pruning using the one provided parameter works),
> and add a comment here explaining that is being tested:
>
> -- create a partition hierarchy to show that plan time pruning removes
> -- the key1=2 table but generates a generic plan for key2=$1
I did that, with a different comment.
> The test involving postgres_fdw is still necessary to exercise the new
> EXEC_FLAG_EXPLAIN_GENERIC code path, but needs to be moved elsewhere,
> probably src/test/modules/.
Tests for postgres_fdw are in contrib/postgres_fdw/sql/postgres_fdw.sql,
so I added the test there.
Version 9 of the patch is attached.
Yours,
Laurenz Albe
Attachment
And here is v10, which includes tab completion for the new option. Yours, Laurenz Albe
Attachment
Re: Laurenz Albe > And here is v10, which includes tab completion for the new option. IMHO everything looks good now. Marking as ready for committer. Thanks! Christoph
Laurenz Albe <laurenz.albe@cybertec.at> writes:
> On Tue, 2023-03-21 at 16:32 +0100, Christoph Berg wrote:
>> The test involving postgres_fdw is still necessary to exercise the new
>> EXEC_FLAG_EXPLAIN_GENERIC code path, but needs to be moved elsewhere,
>> probably src/test/modules/.
> Tests for postgres_fdw are in contrib/postgres_fdw/sql/postgres_fdw.sql,
> so I added the test there.
I don't actually see why a postgres_fdw test case is needed at all?
The tests in explain.sql seem sufficient.
regards, tom lane
Re: Tom Lane > I don't actually see why a postgres_fdw test case is needed at all? > The tests in explain.sql seem sufficient. When I asked Laurenz about that he told me that local tables wouldn't exercise the code specific for EXEC_FLAG_EXPLAIN_GENERIC. (Admittedly my knowledge of the planner wasn't deep enough to verify that. Laurenz is currently traveling, so I don't know if he could answer this himself now.) Christoph
Christoph Berg <myon@debian.org> writes:
> Re: Tom Lane
>> I don't actually see why a postgres_fdw test case is needed at all?
>> The tests in explain.sql seem sufficient.
> When I asked Laurenz about that he told me that local tables wouldn't
> exercise the code specific for EXEC_FLAG_EXPLAIN_GENERIC.
But there isn't any ... or at least, I fail to see what isn't sufficiently
exercised by that new explain.sql test case that's identical to this one
except for being a non-foreign table. Perhaps at some point this patch
modified postgres_fdw code? But it doesn't now.
I don't mind having a postgres_fdw test if there's something for it
to test, but it just looks duplicative to me. Other things being
equal, I'd prefer to test this feature in explain.sql, since (a) it's
a core feature and (b) the core tests are better parallelized than the
contrib tests, so the same test should be cheaper to run.
> (Admittedly my knowledge of the planner wasn't deep enough to verify
> that. Laurenz is currently traveling, so I don't know if he could
> answer this himself now.)
OK, thanks for the status update. What I'll do to get this off my
plate is to push the patch without the postgres_fdw test -- if
Laurenz wants to advocate for that when he returns, we can discuss it
more.
regards, tom lane
On Fri, 2023-03-24 at 16:41 -0400, Tom Lane wrote: > Christoph Berg <myon@debian.org> writes: > > Re: Tom Lane > > > I don't actually see why a postgres_fdw test case is needed at all? > > > The tests in explain.sql seem sufficient. > > > When I asked Laurenz about that he told me that local tables wouldn't > > exercise the code specific for EXEC_FLAG_EXPLAIN_GENERIC. > > But there isn't any ... or at least, I fail to see what isn't sufficiently > exercised by that new explain.sql test case that's identical to this one > except for being a non-foreign table. Perhaps at some point this patch > modified postgres_fdw code? But it doesn't now. > > I don't mind having a postgres_fdw test if there's something for it > to test, but it just looks duplicative to me. Other things being > equal, I'd prefer to test this feature in explain.sql, since (a) it's > a core feature and (b) the core tests are better parallelized than the > contrib tests, so the same test should be cheaper to run. > > > (Admittedly my knowledge of the planner wasn't deep enough to verify > > that. Laurenz is currently traveling, so I don't know if he could > > answer this himself now.) > > OK, thanks for the status update. What I'll do to get this off my > plate is to push the patch without the postgres_fdw test -- if > Laurenz wants to advocate for that when he returns, we can discuss it > more. Thanks for committing this. As Chrisoph mentioned, I found that I could not reproduce the problem that was addressed by the EXEC_FLAG_EXPLAIN_GENERIC hack using local partitioned tables. My optimizer knowledge is not deep enough to tell why, and it might well be a bug lurking in the FDW part of the optimizer code. It is not FDW specific, since I discovered it with oracle_fdw and could reproduce it with postgres_fdw. I was aware that it is awkward to add a test to a contrib module, but I thought that I should add a test that exercises the new code path. But I am fine without the postgres_fdw test. Yours, Laurenz Albe