MIME-Version: 1.0
References: <CAP+B4TD=iy-C2EnsrJgjpwSc7_4pd3Xh-gFzA0bwsw3q8u860g@mail.gmail.com>
 <CAA5RZ0vXiOiodrNQ-Va4FCAkXMpGA=GZDeKjFBRgRvHGuW7s7Q@mail.gmail.com>
 <07147023882ac6c4424df946fd4b82b8e8c211a5.camel@j-davis.com>
 <341357.1741201933@sss.pgh.pa.us> <fc0cd0a77a2a5566f30a630580affb9fc3f43ce7.camel@j-davis.com>
 <CAP+B4TDxE3gbrixL9qR9F+CK_N_N0-HmXSL55QXVZNAorRHSHQ@mail.gmail.com>
 <CAA5RZ0skXF+d7SANSnMi3xMTs9zr6dZadyc5OEtPiMnw-rGizw@mail.gmail.com>
 <CAA5RZ0vFAhGgz-VwE0NwvesHNDN5PWAgWA1B=cECg0=5nt0kjQ@mail.gmail.com> <CAA5RZ0spw7Fmd5pGvFdTaG2GqkSm6HeXJABbBeqzUDkuN4uevQ@mail.gmail.com>
In-Reply-To: <CAA5RZ0spw7Fmd5pGvFdTaG2GqkSm6HeXJABbBeqzUDkuN4uevQ@mail.gmail.com>
From: dinesh salve <cooltodinesh@gmail.com>
Date: Wed, 7 Jan 2026 15:30:25 +0530
Message-ID: <CAP+B4TDRPOth7EJcmFMNaA-XchbPpfZ1OPk+__0a1ZMhibJjrw@mail.gmail.com>
Subject: Re: explain plans for foreign servers
To: Sami Imseih <samimseih@gmail.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, pgsql-hackers@postgresql.org, 
	Jeff Davis <pgsql@j-davis.com>
Content-Type: multipart/alternative; boundary="000000000000c4e05c0647c95e44"
Archived-At: <https://www.postgresql.org/message-id/CAP%2BB4TDRPOth7EJcmFMNaA-XchbPpfZ1OPk%2B__0a1ZMhibJjrw%40mail.gmail.com>
Precedence: bulk

--000000000000c4e05c0647c95e44
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

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 >=3D 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=E2=80=AFAM Sami Imseih <samimseih@gmail.com> w=
rote:

> 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)

--000000000000c4e05c0647c95e44
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

<div dir=3D"ltr"><div dir=3D"ltr">Thanks=C2=A0Sami for feedback. Few points=
 I wanted to call out and need your inputs on -=C2=A0<div><br></div><div>Su=
pporting=C2=A0remote_plans options for inserts:</div><div>Only &quot;explai=
n insert&quot; are executed with bind variables (verified by logging all sq=
ls while running make check) and while executing that on remote is erroring=
 out with error &quot;there is no parameter $1&quot;. We can either NOT sup=
port remote plans for insert statements or always use generic_plan option o=
n remote sql. Using &quot;generic_plan&quot; on remote comes with=C2=A0an a=
dditional check if remote supports this option or not in case remote shard =
is older postgres.</div><div>I prefer not supporting remote_plans for inser=
ts as there is nothing much that goes in insert statement plans unless its =
&quot;insert into..select&quot;. User can always run explain on that select=
 separately. Appreciate your inputs=C2=A0on this.</div><div><br></div><div>=
About decision=C2=A0which=C2=A0explain options we should forward to remote =
shard:</div><div>This is because local and remote postgres could be differe=
nt and we still need to address what all options we send in remote sql as r=
emote shard might not even support them. We can forward only limited option=
s to remote which are widely supported (pg &gt;=3D 9) i.e. verbose, costs, =
buffers, format only.</div>If we need to support all possible options, we n=
eed to query the version of remote postgres and then prepare remote sql. Th=
oughts?<div><br></div><div>Regard,</div><div>Dinesh (AWS)</div></div><br><d=
iv class=3D"gmail_quote gmail_quote_container"><div dir=3D"ltr" class=3D"gm=
ail_attr">On Wed, Dec 10, 2025 at 2:38=E2=80=AFAM Sami Imseih &lt;<a href=
=3D"mailto:samimseih@gmail.com">samimseih@gmail.com</a>&gt; wrote:<br></div=
><blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border=
-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);=
padding-left:1ex">Hi,<br>
<br>
I spent more time reviewing this patch. Here are additional comments.<br>
<br>
1/ Remove unnecessary includes<br>
<br>
#include &quot;commands/explain_state.h&quot; from option.c.<br>
#include &quot;utils/json.h&quot; from postgres_fdw.c.<br>
<br>
2/ Add new EXPLAIN options<br>
<br>
MEMORY and SUMMARY flags added to the remote EXPLAIN query formatting.<br>
These do not require ANALYZE to be used.<br>
<br>
A larger question is how would we want to ensure that new core EXPLAIN<br>
options can be automatically set?<br>
<br>
This is not quite common, but perhaps it is a good thing to add a comment<b=
r>
near ExplainState explaining that if a new option is added, make sure<br>
that postgre_fdw remote_plans are updated.<br>
<br>
```<br>
typedef struct ExplainState<br>
{<br>
=C2=A0 StringInfo=C2=A0 str;=C2=A0 =C2=A0 =C2=A0 /* output buffer */<br>
=C2=A0 /* options */<br>
=C2=A0 bool=C2=A0 =C2=A0 verbose;=C2=A0 =C2=A0 /* be verbose */<br>
=C2=A0 bool=C2=A0 =C2=A0 analyze;=C2=A0 =C2=A0 /* print actual times */<br>
=C2=A0 bool=C2=A0 =C2=A0 costs;=C2=A0 =C2=A0 =C2=A0 /* print estimated co<b=
r>
<br>
```<br>
<br>
3/ Removed unnecessary pstrdup() when appending remote plan rows to<br>
explain_plan.<br>
<br>
Removed unnecessary pstrdup() when appending remote plan rows to explain_pl=
an.<br>
<br>
```<br>
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0appendStringInfo(&amp;explain-&gt;explain_plan,<br>
&quot;%s\n&quot;, pstrdup(PQgetvalue(res, i, 0)));<br>
```<br>
<br>
<br>
4/ Simplify foreign table OID handling in postgresExplainForeignScan<br>
<br>
I am not sure why we need a list that can only hold a single value.<br>
Can we just use an Oid variable to store this?<br>
<br>
<br>
5/ Encapsulates getting the connection, executing the remote EXPLAIN,<br>
and releasing the connection.<br>
<br>
Replaces repeated code in postgresExplainForeignScan,<br>
postgresExplainForeignModify, and postgresExplainDirectModify.<br>
<br>
For #4 and #5, attached is my attempt to simplify these routines. What<br>
do you think?<br>
<br>
6/ Updated typedefs.list<br>
<br>
... to include PgFdwExplainRemotePlans and PgFdwExplainState.<br>
<br>
<br>
7/ Tests<br>
<br>
I quickly skimmed the tests, but I did not see a join push-down test.<br>
We should add<br>
that.<br>
<br>
<br>
--<br>
Sami Imseih<br>
Amazon Web Services (AWS)</blockquote></div><div dir=3D"ltr" class=3D"gmail=
_signature"><div dir=3D"ltr"><div><span style=3D"color:rgb(31,73,125);font-=
family:Calibri,sans-serif;font-size:15px"></span></div></div></div></div>

--000000000000c4e05c0647c95e44--