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

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