Re: explain plans for foreign servers

Hi,

Thanks for working on this patch!

I looked at the patch from and I have several comments. There are
some others, but wanted to start with the most important I found, in order
of importance.

1/ The use of NOTICE to propagate the explain plan.
I see the message content is checked, but this does not look robust
and could lead to
some strange results if another ExecutorRun hook emits a similar notice message.

+ // We might receive plans per batch of cursor, but we only need
to store one.
+ // do we really need to handle len==0. report warn if we still
recived. have test around this warn.
+ if (strstr(notice, "postgres_fdw_explain_plan") &&
explain_plans->len == 0) {

2/ The current patch requires that the remote side has postgres_fdw
enabled. This seems very much
against the philosophy of FDWs. Only the side which creates the
foreign tables should require
the extension to be installed.

Also, if auto_explain_plan is enabled on the foreign table side, it
seems the explain_ExecutorRun
is exercised. This code path should only be taken on the remote side. right?

3/ ExecutorRun is the wrong place to send the plans from, because
postgres_fdw performs
FETCHES from a SQL declared cursor, and each fetch will hit
ExecutorRun. If you return the
first plan from ExecutorRun and stop consuming the rest of the plans,
you will only get the
results from a single fetch only. The plan should be generated and
sent back at CLOSE cursor time.

4/ As far as presenting the remote plans, I think adding them inline
in the EXPLAIN output
will make the plans hard to read, especially fas the plans become more complex.
What about they get added to a new section called "remote plans" and
the remote plans will
be identified by the plan_node_id, which we can add.

Below is a sketch-up to make it clear what I am thinking.

"""
Sort (cost=1..10 rows=10 width=120) (actual time=1..10 rows=10 loops=1)
-> Foreign Scan on prices (cost=100.00..200.00 rows=10width=59)
(actual time=1..100 rows=10 loops=1) (node=1)
Planning Time: 10 ms
Execution Time: 100 ms

Remote Plans
---------------
node 1:

Seq Scan on prices (cost=100.00..4576146.22 rows=467980 width=59)
(actual time=8737.505..2258486.086 rows=31752 loops=1)
"""

Here is a thought about how to generate and consume the plans.

What if we do something like a new EXPLAIN option which returns all the rows
back to the client, and then writes out the plan to some local memory. We would
then be able to fetch the last plan through a sql function, i.e.
SELECT pg_last_explain().

This may have applications beyond postgre_fdw; but in the case of
postgres_fdw, it will
call the remote sql using this EXPLAIN option and at the end of
execution, it will be
responsible to fetch the plans from pg_last_explain. I Have not fully
formulated this idea,
but wanted to share it.

Regards,

Sami Imseih
Amazon Web Services (AWS)