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)
