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)
/*
* explain_remote_query
* Helper function to get connection and execute remote EXPLAIN
*/
static void
explain_remote_query(int plan_node_id, ExplainState *es,
PgFdwExplainState *pgfdw_explain_state,
Oid foreign_table_oid, char *sql)
{
UserMapping *user;
PGconn *conn;
ForeignTable *table;
table = GetForeignTable(foreign_table_oid);
user = GetUserMapping(GetUserId(), table->serverid);
conn = GetConnection(user, false, NULL);
postgresExplainStatement(plan_node_id, es, pgfdw_explain_state, conn,
sql);
ReleaseConnection(conn);
}
/*
* postgresExplainForeignScan
* Produce extra output for EXPLAIN of a ForeignScan on a foreign
table
*/
static void
postgresExplainForeignScan(ForeignScanState *node, ExplainState *es)
{
ForeignScan *plan = castNode(ForeignScan, node->ss.ps.plan);
List *fdw_private = plan->fdw_private;
PgFdwExplainState *pgfdw_explain_state;
char *sql;
Oid foreign_table_oid = InvalidOid;
if (node->ss.ss_currentRelation)
foreign_table_oid =
RelationGetRelid(node->ss.ss_currentRelation);
/*
* Identify foreign scans that are really joins or upper relations. The
* input looks something like "(1) LEFT JOIN (2)", and we must replace
the
* digit string(s), which are RT indexes, with the correct relation
names.
* We do that here, not when the plan is created, because we can't know
* what aliases ruleutils.c will assign at plan creation time.
*/
if (list_length(fdw_private) > FdwScanPrivateRelations)
{
StringInfoData relations;
char *rawrelations;
char *ptr;
int minrti,
rtoffset;
rawrelations = strVal(list_nth(fdw_private,
FdwScanPrivateRelations));
/*
* A difficulty with using a string representation of RT
indexes is
* that setrefs.c won't update the string when flattening the
* rangetable. To find out what rtoffset was applied, identify
the
* minimum RT index appearing in the string and compare it to
the
* minimum member of plan->fs_base_relids. (We expect all the
relids
* in the join will have been offset by the same amount; the
Asserts
* below should catch it if that ever changes.)
*/
minrti = INT_MAX;
ptr = rawrelations;
while (*ptr)
{
if (isdigit((unsigned char) *ptr))
{
int rti = strtol(ptr, &ptr,
10);
if (rti < minrti)
minrti = rti;
}
else
ptr++;
}
rtoffset = bms_next_member(plan->fs_base_relids, -1) - minrti;
/* Now we can translate the string */
initStringInfo(&relations);
ptr = rawrelations;
while (*ptr)
{
if (isdigit((unsigned char) *ptr))
{
int rti = strtol(ptr, &ptr,
10);
RangeTblEntry *rte;
char *relname;
char *refname;
rti += rtoffset;
Assert(bms_is_member(rti,
plan->fs_base_relids));
rte = rt_fetch(rti, es->rtable);
Assert(rte->rtekind == RTE_RELATION);
/* This logic should agree with explain.c's
ExplainTargetRel */
relname = get_rel_name(rte->relid);
/*
* Save first table OID for getting server
connection
*/
if (!OidIsValid(foreign_table_oid))
foreign_table_oid = rte->relid;
if (es->verbose)
{
char *namespace;
namespace =
get_namespace_name_or_temp(get_rel_namespace(rte->relid));
appendStringInfo(&relations, "%s.%s",
quote_identifier(namespace),
quote_identifier(relname));
}
else
appendStringInfoString(&relations,
quote_identifier(relname));
refname = (char *) list_nth(es->rtable_names,
rti - 1);
if (refname == NULL)
refname = rte->eref->aliasname;
if (strcmp(refname, relname) != 0)
appendStringInfo(&relations, " %s",
quote_identifier(refname));
}
else
appendStringInfoChar(&relations, *ptr++);
}
ExplainPropertyText("Relations", relations.data, es);
}
sql = strVal(list_nth(fdw_private, FdwScanPrivateSelectSql));
/*
* Add remote query, when VERBOSE option is specified.
*/
if (es->verbose)
ExplainPropertyText("Remote SQL", sql, es);
pgfdw_explain_state = GetExplainExtensionState(es, GET_EXTENSION_ID());
/* If we don't have a foreign table oid by now, something went wrong */
Assert(foreign_table_oid);
if (pgfdw_explain_state && pgfdw_explain_state->remote_plans)
explain_remote_query(node->ss.ps.plan->plan_node_id,
es,
pgfdw_explain_state,
foreign_table_oid,
sql);
}
/*
* postgresExplainForeignModify
* Produce extra output for EXPLAIN of a ModifyTable on a foreign
table
*/
static void
postgresExplainForeignModify(ModifyTableState *mtstate,
ResultRelInfo *rinfo,
List *fdw_private,
int subplan_index,
ExplainState *es)
{
char *sql = strVal(list_nth(fdw_private,
FdwModifyPrivateUpdateSql));
PgFdwExplainState *pgfdw_explain_state;
if (es->verbose)
{
ExplainPropertyText("Remote SQL", sql, es);
/*
* For INSERT we should always have batch size >= 1, but UPDATE
and
* DELETE don't support batching so don't show the property.
*/
if (rinfo->ri_BatchSize > 0)
ExplainPropertyInteger("Batch Size", NULL,
rinfo->ri_BatchSize, es);
}
pgfdw_explain_state = GetExplainExtensionState(es, GET_EXTENSION_ID());
if (pgfdw_explain_state && pgfdw_explain_state->remote_plans)
explain_remote_query(mtstate->ps.plan->plan_node_id,
es,
pgfdw_explain_state,
rinfo->ri_RelationDesc->rd_rel->oid,
sql);
}
/*
* postgresExplainDirectModify
* Produce extra output for EXPLAIN of a ForeignScan that modifies
a
* foreign table directly
*/
static void
postgresExplainDirectModify(ForeignScanState *node, ExplainState *es)
{
List *fdw_private;
char *sql;
PgFdwExplainState *pgfdw_explain_state;
fdw_private = ((ForeignScan *) node->ss.ps.plan)->fdw_private;
sql = strVal(list_nth(fdw_private, FdwDirectModifyPrivateUpdateSql));
if (es->verbose)
ExplainPropertyText("Remote SQL", sql, es);
pgfdw_explain_state = GetExplainExtensionState(es, GET_EXTENSION_ID());
if (pgfdw_explain_state && pgfdw_explain_state->remote_plans)
explain_remote_query(node->ss.ps.plan->plan_node_id,
es,
pgfdw_explain_state,
RelationGetRelid(node->ss.ss_currentRelation),
sql);
}
