Subject: Re: explain plans for foreign servers

From: Tom Lane <[email-masked]>
Date: Wed, 05 Mar 2025 14:12:13 -0500

Jeff Davis <[email-masked]> writes:
> Ideally, we'd have EXPLAIN ANALYZE return two result sets, kind of like
> how a query with a semicolon returns two result sets. That changes the
> expected message flow for EXPLAIN ANALYZE, though, so we'd need a new
> option so we are sure the client is expecting it (is this a sane
> idea?).

I'm afraid not.  That pretty fundamentally breaks the wire protocol,
I think.  Also (1) there could be more than two, no, if the query
touches more than one foreign table?  How would the client know
how many to expect?  (2) there would be no particularly compelling
ordering for the multiple resultsets.

> I wonder if Robert's extensible EXPLAIN work[1] could be useful
> here?

I'm wondering the same.  You could certainly imagine cramming
all of the foreign EXPLAIN output into some new field attached
to the ForeignScan node.  Readability and indentation would
require some thought, but the other approaches don't have any
mechanism for addressing that at all.

			regards, tom lane

---

From: Jeff Davis <[email-masked]>
Date: Wed, 05 Mar 2025 15:24:31 -0800

On Wed, 2025-03-05 at 14:12 -0500, Tom Lane wrote:
> I think.

The extended protocol docs say: "The possible responses to Execute are
the same as those described above for queries issued via simple query
protocol, except that Execute doesn't cause ReadyForQuery or
RowDescription to be issued."

Each result set needs a RowDescription, so I think you're right that it
breaks the extended protocol. I missed that the first time.

Regards,
	Jeff Davis

---

From: dinesh salve <[email-masked]>
Date: Tue, 18 Nov 2025 08:55:39 +0530

Hi Sami,
Thanks for the feedback. I have refactored the commit on the latest version
of PG and added a few more tests. To simplify the roll out of this feature,
I decided to work on analyze=false use case first. Please find the attached
patch for the same.

Regards,
Dinesh

-- 

Regards,
Dinesh Salve
Cell:- 91 8125898845

---

From: Sami Imseih <[email-masked]>
Date: Wed, 3 Dec 2025 16:40:26 -0600

> I have refactored the commit on the latest version of PG and added a few more tests.

Thanks for the update!

> To simplify the roll out of this feature, I decided to work on analyze=false use case first.

I did not go through the entire patch yet, but a few things stood out
from my first pass.

1/
RegisterExtensionExplainOption is called during _PG_init, which is fine, but I
also wonder if we can call this during postgresExplainForeignScan as well?

The reason being is for _PG_init to be invoked, the user must load postgres_fdw
(LOAD, session_preload_libraries, shared_preload_libraries), which from my
experience is not very common in postgres_fdw. Users ordinarily just
"CREATE EXTENSION..."

So this needs to be documented [0]

2/

Does this behave sanely with multiple fdw connections? Can we add
tests for this?

+
+                               /*
+                                * add one of the tables to
foreign_scan_table to get the
+                                * serverId for remote plans
+                                */
+                               if (list_length(foreign_scan_table) == 0)
+                                       foreign_scan_table =
lappend_oid(foreign_scan_table, rte->relid);
+

[0] https://www.postgresql.org/docs/current/postgres-fdw.html

--
Sami Imseih
Amazon Web Services (AWS)

---

From: Sami Imseih <[email-masked]>
Date: Wed, 3 Dec 2025 18:34:23 -0600

I forgot to mention this point earlier.

I noticed GENERIC_PLAN is hard-coded to 1 (true).
Is that an oversight, or is it required?

```
+ GENERIC_PLAN 1, \
```

---

From: Sami Imseih <[email-masked]>
Date: Tue, 9 Dec 2025 15:08:08 -0600

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.