Re: explain plans for foreign servers
| От | dinesh salve |
|---|---|
| Тема | Re: explain plans for foreign servers |
| Дата | |
| Msg-id | CAP+B4TDRPOth7EJcmFMNaA-XchbPpfZ1OPk+__0a1ZMhibJjrw@mail.gmail.com обсуждение исходный текст |
| Ответ на | Re: explain plans for foreign servers (Sami Imseih <samimseih@gmail.com>) |
| Список | pgsql-hackers |
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@gmail.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)
В списке pgsql-hackers по дате отправления: