Re: explain plans for foreign servers

Thanks Sami for feedback. Few points I wanted to call out and need your
inputs on -

Supporting remote_plans options for inserts:
Only "explain insert" are executed with bind variables (verified by logging
all sqls while running make check) and while executing that on remote is
erroring out with error "there is no parameter $1". We can either NOT
support remote plans for insert statements or always use generic_plan
option on remote sql. Using "generic_plan" on remote comes with an
additional check if remote supports this option or not in case remote shard
is older postgres.
I prefer not supporting remote_plans for inserts as there is nothing much
that goes in insert statement plans unless its "insert into..select". User
can always run explain on that select separately. Appreciate your inputs on
this.

About decision which explain options we should forward to remote shard:
This is because local and remote postgres could be different and we still
need to address what all options we send in remote sql as remote shard
might not even support them. We can forward only limited options to remote
which are widely supported (pg >= 9) i.e. verbose, costs, buffers, format
only.
If we need to support all possible options, we need to query the version of
remote postgres and then prepare remote sql. Thoughts?

Regard,
Dinesh (AWS)

On Wed, Dec 10, 2025 at 2:38 AM Sami Imseih <samimseih(at)gmail(dot)com> wrote:

> Hi,
>
> I spent more time reviewing this patch. Here are additional comments.
>
> 1/ Remove unnecessary includes
>
> #include "commands/explain_state.h" from option.c.
> #include "utils/json.h" from postgres_fdw.c.
>
> 2/ Add new EXPLAIN options
>
> MEMORY and SUMMARY flags added to the remote EXPLAIN query formatting.
> These do not require ANALYZE to be used.
>
> A larger question is how would we want to ensure that new core EXPLAIN
> options can be automatically set?
>
> This is not quite common, but perhaps it is a good thing to add a comment
> near ExplainState explaining that if a new option is added, make sure
> that postgre_fdw remote_plans are updated.
>
> ```
> typedef struct ExplainState
> {
> StringInfo str; /* output buffer */
> /* options */
> bool verbose; /* be verbose */
> bool analyze; /* print actual times */
> bool costs; /* print estimated co
>
> ```
>
> 3/ Removed unnecessary pstrdup() when appending remote plan rows to
> explain_plan.
>
> Removed unnecessary pstrdup() when appending remote plan rows to
> explain_plan.
>
> ```
> + appendStringInfo(&explain->explain_plan,
> "%s\n", pstrdup(PQgetvalue(res, i, 0)));
> ```
>
>
> 4/ Simplify foreign table OID handling in postgresExplainForeignScan
>
> I am not sure why we need a list that can only hold a single value.
> Can we just use an Oid variable to store this?
>
>
> 5/ Encapsulates getting the connection, executing the remote EXPLAIN,
> and releasing the connection.
>
> Replaces repeated code in postgresExplainForeignScan,
> postgresExplainForeignModify, and postgresExplainDirectModify.
>
> For #4 and #5, attached is my attempt to simplify these routines. What
> do you think?
>
> 6/ Updated typedefs.list
>
> ... to include PgFdwExplainRemotePlans and PgFdwExplainState.
>
>
> 7/ Tests
>
> I quickly skimmed the tests, but I did not see a join push-down test.
> We should add
> that.
>
>
> --
> Sami Imseih
> Amazon Web Services (AWS)