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