At 2026-07-09 02:12:36, "dinesh salve" <[email protected]> wrote:
> As discussed offline, also added a regression test that joins two foreign
> tables living on different foreign servers (loopback and loopback2), which
> exercises REMOTE_PLANS over multiple fdw connections.
Hi,
Few comments for v4-0001.
1.
--- a/src/include/commands/explain_state.h
+++ b/src/include/commands/explain_state.h
@@ -45,6 +45,7 @@ typedef struct ExplainWorkersState
typedef struct ExplainState
{
StringInfo str; /* output buffer */
+
/* options */
bool verbose; /* be verbose */
bool analyze; /* print actual times */
Is this blank line in explain_state.h necessary?
2.
diff --git a/contrib/postgres_fdw/postgres_fdw.c
b/contrib/postgres_fdw/postgres_fdw.c
index 0ea72a6..ff8c76e 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
......
+static void
+postgresExplainStatement(int plan_node_id,
+ ExplainState *es,
+ PgFdwExplainState *pgfdw_explain_state,
+ PGconn *conn,
+ char *sql)
+{
+ PGresult *volatile res = NULL;
+ StringInfoData explain_sql;
+ int remote_version = PQserverVersion(conn);
+
+ /*
+ * GENERIC_PLAN required because deparsed SQL may contain $n placeholders;
+ * only available from PG 16.
+ */
+ if (remote_version < 160000)
+ ereport(ERROR,
+ errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("EXPLAIN option REMOTE_PLANS requires a remote server version of 16 or
later"),
+ errdetail("The remote server version is %d.", remote_version));
+
+ PG_TRY();
+ {
+ int numrows,
+ i;
+ PgFdwExplainRemotePlans *explain = (PgFdwExplainRemotePlans *)
palloc(sizeof(PgFdwExplainRemotePlans));
+ ListCell *lc;
+
+ initStringInfo(&explain_sql);
+ initStringInfo(&explain->explain_plan);
The StringInfoData explain_sql; declared here is uninitialized. While the risk
is low, it could lead to subtle problems.
This pattern also does not align with existing usage of StringInfoData
throughout the codebase.
I recommend initializing it before entering the PG_TRY block.
Best regards,
--
Yilin Zhang
