Horton

Thread

Commits

  1. Invent GENERIC_PLAN option for EXPLAIN.

  1. Make EXPLAIN generate a generic plan for a parameterized query

    Laurenz Albe <laurenz.albe@cybertec.at> — 2022-10-11T12:37:25Z

    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

  2. Re: Make EXPLAIN generate a generic plan for a parameterized query

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-10-11T13:49:14Z

    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

  3. Re: Make EXPLAIN generate a generic plan for a parameterized query

    Julien Rouhaud <rjuju123@gmail.com> — 2022-10-11T16:03:48Z

    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.

  4. Re: Make EXPLAIN generate a generic plan for a parameterized query

    Laurenz Albe <laurenz.albe@cybertec.at> — 2022-10-25T09:08:27Z

    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

  5. Re: Make EXPLAIN generate a generic plan for a parameterized query

    Julien Rouhaud <rjuju123@gmail.com> — 2022-10-25T11:03:41Z

    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.

  6. Re: Make EXPLAIN generate a generic plan for a parameterized query

    Laurenz Albe <laurenz.albe@cybertec.at> — 2022-10-29T08:35:26Z

    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

  7. Re: Make EXPLAIN generate a generic plan for a parameterized query

    Andres Freund <andres@anarazel.de> — 2022-12-06T18:17:08Z

    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

  8. Re: Make EXPLAIN generate a generic plan for a parameterized query

    Laurenz Albe <laurenz.albe@cybertec.at> — 2022-12-07T11:23:02Z

    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

  9. Re: Make EXPLAIN generate a generic plan for a parameterized query

    Michel Pelletier <pelletier.michel@gmail.com> — 2022-12-27T22:37:11Z

    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.
>

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;
      |                         ^~~~~~~~~~~~

  10. Re: Make EXPLAIN generate a generic plan for a parameterized query

    Laurenz Albe <laurenz.albe@cybertec.at> — 2023-01-09T16:40:01Z

    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

  11. Re: Make EXPLAIN generate a generic plan for a parameterized query

    Jim Jones <jim.jones@uni-muenster.de> — 2023-01-16T13:39:08Z

    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

  12. Re: Make EXPLAIN generate a generic plan for a parameterized query

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-01-16T17:02:12Z

    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

  13. Re: Make EXPLAIN generate a generic plan for a parameterized query

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-01-31T18:49:38Z

    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

  14. Re: Make EXPLAIN generate a generic plan for a parameterized query

    Laurenz Albe <laurenz.albe@cybertec.at> — 2023-02-03T13:30:59Z

    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

  15. Re: Make EXPLAIN generate a generic plan for a parameterized query

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-02-03T14:44:43Z

    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

  16. Re: Make EXPLAIN generate a generic plan for a parameterized query

    Laurenz Albe <laurenz.albe@cybertec.at> — 2023-02-03T16:14:29Z

    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

  17. Re: Make EXPLAIN generate a generic plan for a parameterized query

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-02-03T20:11:22Z

    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

  18. Re: Make EXPLAIN generate a generic plan for a parameterized query

    Laurenz Albe <laurenz.albe@cybertec.at> — 2023-02-05T17:24:03Z

    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

  19. Re: Make EXPLAIN generate a generic plan for a parameterized query

    Andres Freund <andres@anarazel.de> — 2023-02-14T00:33:06Z

    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

  20. Re: Make EXPLAIN generate a generic plan for a parameterized query

    Laurenz Albe <laurenz.albe@cybertec.at> — 2023-02-14T12:44:44Z

    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

  21. Re: Make EXPLAIN generate a generic plan for a parameterized query

    Christoph Berg <myon@debian.org> — 2023-03-21T15:32:48Z

    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

  22. Re: Make EXPLAIN generate a generic plan for a parameterized query

    Laurenz Albe <laurenz.albe@cybertec.at> — 2023-03-22T13:15:23Z

    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

  23. Re: Make EXPLAIN generate a generic plan for a parameterized query

    Laurenz Albe <laurenz.albe@cybertec.at> — 2023-03-23T18:31:26Z

    And here is v10, which includes tab completion for the new option.

Yours,
Laurenz Albe

  24. Re: Make EXPLAIN generate a generic plan for a parameterized query

    Christoph Berg <myon@debian.org> — 2023-03-24T10:58:09Z

    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

  25. Re: Make EXPLAIN generate a generic plan for a parameterized query

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-03-24T17:20:06Z

    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

  26. Re: Make EXPLAIN generate a generic plan for a parameterized query

    Christoph Berg <myon@debian.org> — 2023-03-24T20:26:32Z

    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

  27. Re: Make EXPLAIN generate a generic plan for a parameterized query

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-03-24T20:41:13Z

    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

  28. Re: Make EXPLAIN generate a generic plan for a parameterized query

    Laurenz Albe <laurenz.albe@cybertec.at> — 2023-03-28T06:35:57Z

    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