From: Tom Lane <tgl@sss.pgh.pa.us>
To: pgsql-hackers@lists.postgresql.org
Subject: Re: Bogus EXPLAIN results with column aliases for mismatched
 partitions
In-reply-to: <4257.1575229116@sss.pgh.pa.us>
References: <12424.1575168015@sss.pgh.pa.us> <4257.1575229116@sss.pgh.pa.us>
Comments: In-reply-to Tom Lane <tgl@sss.pgh.pa.us>
	message dated "Sun, 01 Dec 2019 14:38:36 -0500"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <10294.1575235920.0@sss.pgh.pa.us>
Date: Sun, 01 Dec 2019 16:34:38 -0500
Message-ID: <10395.1575236078@sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <10294.1575235920.1@sss.pgh.pa.us>

I wrote:
> Now, there is another thing that set_rtable_names() is doing that
> postgres_fdw doesn't do, which is to unique-ify the chosen alias
> names by adding "_NN" if the querytree contains multiple uses of
> the same table alias.  I don't see any practical way for postgres_fdw
> to match that behavior, since it lacks global information about the
> query.  If we wanted to fix it, what we'd likely need to do is
> postpone creation of the relation_name strings until EXPLAIN,
> providing some way for postgres_fdw to ask ruleutils.c what alias
> it'd assigned to a particular RTE.

Hmmm ... so actually, that isn't *quite* as bad as I thought:
ExplainState does already expose that information, so we just need
to rearrange how postgres_fdw does things.  Here's a revised proposed
patch, which exposes (and fixes) several additional test cases where
the Relations: string was previously visibly out of sync with what
ruleutils was using for Var names.

BTW, the existing code always schema-qualifies the relation names,
on the rather lame grounds that it's producing the string without
knowing whether EXPLAIN VERBOSE will be specified.  In this code,
the verbose flag is available so it would be trivial to make the
output conform to EXPLAIN's normal policy.  I didn't change that
here because there'd be a bunch more test output diffs of no
intellectual interest.  Should we change it, or leave well enough
alone?

			regards, tom lane


------- =_aaaaaaaaaa0
Content-Type: text/x-diff;
	name="fix-alias-usage-in-postgres_fdw-explain-2.patch";
	charset="us-ascii"
Content-ID: <10294.1575235920.2@sss.pgh.pa.us>
Content-Description: fix-alias-usage-in-postgres_fdw-explain-2.patch
Content-Transfer-Encoding: quoted-printable

diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/post=
gres_fdw/expected/postgres_fdw.out
index 14180ee..d276545 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -1348,7 +1348,7 @@ SELECT t1.c1, ss.a, ss.b FROM (SELECT c1 FROM ft4 WH=
ERE c1 between 50 and 60) t1
 -------------------------------------------------------------------------=
--------------------------------------------------------------------------=
--------------------------------------------------------------------------=
--------------------------------------------------------------------------=
--------------------------------------------------------------------------=
--------------------------------------------------------------------------=
---------------------------------------------------------
  Foreign Scan
    Output: ft4.c1, ft4_1.c1, ft5.c1
-   Relations: (public.ft4) FULL JOIN ((public.ft4) FULL JOIN (public.ft5)=
)
+   Relations: (public.ft4) FULL JOIN ((public.ft4 ft4_1) FULL JOIN (publi=
c.ft5))
    Remote SQL: SELECT s4.c1, s10.c1, s10.c2 FROM ((SELECT c1 FROM "S 1"."=
T 3" WHERE ((c1 >=3D 50)) AND ((c1 <=3D 60))) s4(c1) FULL JOIN (SELECT s8.=
c1, s9.c1 FROM ((SELECT c1 FROM "S 1"."T 3" WHERE ((c1 >=3D 50)) AND ((c1 =
<=3D 60))) s8(c1) FULL JOIN (SELECT c1 FROM "S 1"."T 4" WHERE ((c1 >=3D 50=
)) AND ((c1 <=3D 60))) s9(c1) ON (((s8.c1 =3D s9.c1)))) WHERE (((s8.c1 IS =
NULL) OR (s8.c1 IS NOT NULL)))) s10(c1, c2) ON (((s4.c1 =3D s10.c1)))) ORD=
ER BY s4.c1 ASC NULLS LAST, s10.c1 ASC NULLS LAST, s10.c2 ASC NULLS LAST
 (4 rows)
 =

@@ -2084,7 +2084,7 @@ SELECT t1c1, avg(t1c1 + t2c1) FROM (SELECT t1.c1, t2=
.c1 FROM ft1 t1 JOIN ft2 t2
                                  Remote SQL: SELECT r1."C 1", r2."C 1" FR=
OM ("S 1"."T 1" r1 INNER JOIN "S 1"."T 1" r2 ON (((r1."C 1" =3D r2."C 1"))=
))
                            ->  Foreign Scan
                                  Output: t1_1.c1, t2_1.c1
-                                 Relations: (public.ft1 t1) INNER JOIN (p=
ublic.ft2 t2)
+                                 Relations: (public.ft1 t1_1) INNER JOIN =
(public.ft2 t2_1)
                                  Remote SQL: SELECT r1."C 1", r2."C 1" FR=
OM ("S 1"."T 1" r1 INNER JOIN "S 1"."T 1" r2 ON (((r1."C 1" =3D r2."C 1"))=
))
 (20 rows)
 =

@@ -3230,7 +3230,7 @@ select count(*), x.b from ft1, (select c2 a, sum(c1)=
 b from ft1 group by c2) x w
                            Output: x.b, x.a
                            ->  Foreign Scan
                                  Output: ft1_1.c2, (sum(ft1_1.c1))
-                                 Relations: Aggregate on (public.ft1)
+                                 Relations: Aggregate on (public.ft1 ft1_=
1)
                                  Remote SQL: SELECT c2, sum("C 1") FROM "=
S 1"."T 1" GROUP BY 1
 (21 rows)
 =

@@ -8480,15 +8480,15 @@ ANALYZE fprt2_p2;
 -- inner join three tables
 EXPLAIN (COSTS OFF)
 SELECT t1.a,t2.b,t3.c FROM fprt1 t1 INNER JOIN fprt2 t2 ON (t1.a =3D t2.b=
) INNER JOIN fprt1 t3 ON (t2.b =3D t3.a) WHERE t1.a % 25 =3D0 ORDER BY 1,2=
,3;
-                                                     QUERY PLAN          =
                                           =

--------------------------------------------------------------------------=
-------------------------------------------
+                                                        QUERY PLAN       =
                                                 =

+-------------------------------------------------------------------------=
-------------------------------------------------
  Sort
    Sort Key: t1.a, t3.c
    ->  Append
          ->  Foreign Scan
                Relations: ((public.ftprt1_p1 t1) INNER JOIN (public.ftprt=
2_p1 t2)) INNER JOIN (public.ftprt1_p1 t3)
          ->  Foreign Scan
-               Relations: ((public.ftprt1_p2 t1) INNER JOIN (public.ftprt=
2_p2 t2)) INNER JOIN (public.ftprt1_p2 t3)
+               Relations: ((public.ftprt1_p2 t1_1) INNER JOIN (public.ftp=
rt2_p2 t2_1)) INNER JOIN (public.ftprt1_p2 t3_1)
 (7 rows)
 =

 SELECT t1.a,t2.b,t3.c FROM fprt1 t1 INNER JOIN fprt2 t2 ON (t1.a =3D t2.b=
) INNER JOIN fprt1 t3 ON (t2.b =3D t3.a) WHERE t1.a % 25 =3D0 ORDER BY 1,2=
,3;
@@ -8507,7 +8507,7 @@ SELECT t1.a,t2.b,t2.c FROM fprt1 t1 LEFT JOIN (SELEC=
T * FROM fprt2 WHERE a < 10)
 -------------------------------------------------------------------------=
--------------------------------------------------------------------------=
--------------------------------------------------------------------------=
-----------------------
  Foreign Scan
    Output: t1.a, ftprt2_p1.b, ftprt2_p1.c
-   Relations: (public.ftprt1_p1 t1) LEFT JOIN (public.ftprt2_p1 fprt2)
+   Relations: (public.ftprt1_p1 t1) LEFT JOIN (public.ftprt2_p1)
    Remote SQL: SELECT r5.a, r6.b, r6.c FROM (public.fprt1_p1 r5 LEFT JOIN=
 public.fprt2_p1 r6 ON (((r5.a =3D r6.b)) AND ((r5.b =3D r6.a)) AND ((r6.a=
 < 10)))) WHERE ((r5.a < 10)) ORDER BY r5.a ASC NULLS LAST, r6.b ASC NULLS=
 LAST, r6.c ASC NULLS LAST
 (4 rows)
 =

@@ -8561,15 +8561,15 @@ SELECT t1.wr, t2.wr FROM (SELECT t1 wr, a FROM fpr=
t1 t1 WHERE t1.a % 25 =3D 0) t1
 -- join with lateral reference
 EXPLAIN (COSTS OFF)
 SELECT t1.a,t1.b FROM fprt1 t1, LATERAL (SELECT t2.a, t2.b FROM fprt2 t2 =
WHERE t1.a =3D t2.b AND t1.b =3D t2.a) q WHERE t1.a%25 =3D 0 ORDER BY 1,2;
-                                   QUERY PLAN                            =
        =

--------------------------------------------------------------------------=
--------
+                                     QUERY PLAN                          =
            =

+-------------------------------------------------------------------------=
------------
  Sort
    Sort Key: t1.a, t1.b
    ->  Append
          ->  Foreign Scan
                Relations: (public.ftprt1_p1 t1) INNER JOIN (public.ftprt2=
_p1 t2)
          ->  Foreign Scan
-               Relations: (public.ftprt1_p2 t1) INNER JOIN (public.ftprt2=
_p2 t2)
+               Relations: (public.ftprt1_p2 t1_1) INNER JOIN (public.ftpr=
t2_p2 t2_1)
 (7 rows)
 =

 SELECT t1.a,t1.b FROM fprt1 t1, LATERAL (SELECT t2.a, t2.b FROM fprt2 t2 =
WHERE t1.a =3D t2.b AND t1.b =3D t2.a) q WHERE t1.a%25 =3D 0 ORDER BY 1,2;
@@ -8689,17 +8689,17 @@ SELECT a, sum(b), min(b), count(*) FROM pagg_tab G=
ROUP BY a HAVING avg(b) < 22 O
 SET enable_partitionwise_aggregate TO true;
 EXPLAIN (COSTS OFF)
 SELECT a, sum(b), min(b), count(*) FROM pagg_tab GROUP BY a HAVING avg(b)=
 < 22 ORDER BY 1;
-                              QUERY PLAN                              =

-----------------------------------------------------------------------
+                         QUERY PLAN                          =

+-------------------------------------------------------------
  Sort
    Sort Key: fpagg_tab_p1.a
    ->  Append
          ->  Foreign Scan
-               Relations: Aggregate on (public.fpagg_tab_p1 pagg_tab)
+               Relations: Aggregate on (public.fpagg_tab_p1)
          ->  Foreign Scan
-               Relations: Aggregate on (public.fpagg_tab_p2 pagg_tab)
+               Relations: Aggregate on (public.fpagg_tab_p2)
          ->  Foreign Scan
-               Relations: Aggregate on (public.fpagg_tab_p3 pagg_tab)
+               Relations: Aggregate on (public.fpagg_tab_p3)
 (9 rows)
 =

 SELECT a, sum(b), min(b), count(*) FROM pagg_tab GROUP BY a HAVING avg(b)=
 < 22 ORDER BY 1;
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/po=
stgres_fdw.c
index fa14296..9e7d9f8 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -12,6 +12,8 @@
  */
 #include "postgres.h"
 =

+#include <limits.h>
+
 #include "access/htup_details.h"
 #include "access/sysattr.h"
 #include "access/table.h"
@@ -574,9 +576,6 @@ postgresGetForeignRelSize(PlannerInfo *root,
 	PgFdwRelationInfo *fpinfo;
 	ListCell   *lc;
 	RangeTblEntry *rte =3D planner_rt_fetch(baserel->relid, root);
-	const char *namespace;
-	const char *relname;
-	const char *refname;
 =

 	/*
 	 * We use PgFdwRelationInfo to pass various information to subsequent
@@ -719,21 +718,11 @@ postgresGetForeignRelSize(PlannerInfo *root,
 	}
 =

 	/*
-	 * Set the name of relation in fpinfo, while we are constructing it here=
.
-	 * It will be used to build the string describing the join relation in
-	 * EXPLAIN output. We can't know whether VERBOSE option is specified or
-	 * not, so always schema-qualify the foreign table name.
+	 * fpinfo->relation_name gets the numeric rangetable index of the foreig=
n
+	 * table RTE.  (If this query gets EXPLAIN'd, we'll convert that to a
+	 * human-readable string at that time.)
 	 */
-	fpinfo->relation_name =3D makeStringInfo();
-	namespace =3D get_namespace_name(get_rel_namespace(foreigntableid));
-	relname =3D get_rel_name(foreigntableid);
-	refname =3D rte->eref->aliasname;
-	appendStringInfo(fpinfo->relation_name, "%s.%s",
-					 quote_identifier(namespace),
-					 quote_identifier(relname));
-	if (*refname && strcmp(refname, relname) !=3D 0)
-		appendStringInfo(fpinfo->relation_name, " %s",
-						 quote_identifier(rte->eref->aliasname));
+	fpinfo->relation_name =3D psprintf("%u", baserel->relid);
 =

 	/* No outer and inner relations. */
 	fpinfo->make_outerrel_subquery =3D false;
@@ -1376,7 +1365,7 @@ postgresGetForeignPlan(PlannerInfo *root,
 							 makeInteger(fpinfo->fetch_size));
 	if (IS_JOIN_REL(foreignrel) || IS_UPPER_REL(foreignrel))
 		fdw_private =3D lappend(fdw_private,
-							  makeString(fpinfo->relation_name->data));
+							  makeString(fpinfo->relation_name));
 =

 	/*
 	 * Create the ForeignScan node for the given relation.
@@ -2528,20 +2517,81 @@ postgresEndDirectModify(ForeignScanState *node)
 static void
 postgresExplainForeignScan(ForeignScanState *node, ExplainState *es)
 {
-	List	   *fdw_private;
-	char	   *sql;
-	char	   *relations;
-
-	fdw_private =3D ((ForeignScan *) node->ss.ps.plan)->fdw_private;
+	ForeignScan *plan =3D castNode(ForeignScan, node->ss.ps.plan);
+	List	   *fdw_private =3D plan->fdw_private;
 =

 	/*
 	 * Add names of relation handled by the foreign scan when the scan is a
-	 * join
+	 * join.  The input looks something like "(1) LEFT JOIN (2)", and we mus=
t
+	 * replace the digit strings with the correct relation names.  We do tha=
t
+	 * here, not when the plan is created, because we can't know what aliase=
s
+	 * ruleutils.c will assign at plan creation time.
 	 */
 	if (list_length(fdw_private) > FdwScanPrivateRelations)
 	{
-		relations =3D strVal(list_nth(fdw_private, FdwScanPrivateRelations));
-		ExplainPropertyText("Relations", relations, es);
+		StringInfo	relations;
+		char	   *rawrelations;
+		char	   *ptr;
+		int			minrti,
+					rtoffset;
+
+		rawrelations =3D 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_relids.  (We expect all the relids in
+		 * the join will have been offset by the same amount.)
+		 */
+		minrti =3D INT_MAX;
+		ptr =3D rawrelations;
+		while (*ptr)
+		{
+			if (isdigit((unsigned char) *ptr))
+			{
+				int			rti =3D strtol(ptr, &ptr, 10);
+
+				if (rti < minrti)
+					minrti =3D rti;
+			}
+			else
+				ptr++;
+		}
+		rtoffset =3D bms_next_member(plan->fs_relids, -1) - minrti;
+
+		/* Now we can translate the string */
+		relations =3D makeStringInfo();
+		ptr =3D rawrelations;
+		while (*ptr)
+		{
+			if (isdigit((unsigned char) *ptr))
+			{
+				int			rti =3D strtol(ptr, &ptr, 10);
+				RangeTblEntry *rte;
+				char	   *namespace;
+				char	   *relname;
+				char	   *refname;
+
+				rti +=3D rtoffset;
+				Assert(bms_is_member(rti, plan->fs_relids));
+				rte =3D rt_fetch(rti, es->rtable);
+				Assert(rte->rtekind =3D=3D RTE_RELATION);
+				namespace =3D get_namespace_name(get_rel_namespace(rte->relid));
+				relname =3D get_rel_name(rte->relid);
+				appendStringInfo(relations, "%s.%s",
+								 quote_identifier(namespace),
+								 quote_identifier(relname));
+				refname =3D (char *) list_nth(es->rtable_names, rti - 1);
+				if (refname && strcmp(refname, relname) !=3D 0)
+					appendStringInfo(relations, " %s",
+									 quote_identifier(refname));
+			}
+			else
+				appendStringInfoChar(relations, *ptr++);
+		}
+		ExplainPropertyText("Relations", relations->data, es);
 	}
 =

 	/*
@@ -2549,6 +2599,8 @@ postgresExplainForeignScan(ForeignScanState *node, E=
xplainState *es)
 	 */
 	if (es->verbose)
 	{
+		char	   *sql;
+
 		sql =3D strVal(list_nth(fdw_private, FdwScanPrivateSelectSql));
 		ExplainPropertyText("Remote SQL", sql, es);
 	}
@@ -5171,13 +5223,14 @@ foreign_join_ok(PlannerInfo *root, RelOptInfo *joi=
nrel, JoinType jointype,
 =

 	/*
 	 * Set the string describing this join relation to be used in EXPLAIN
-	 * output of corresponding ForeignScan.
+	 * output of corresponding ForeignScan.  Note that the decoration we add
+	 * to the base relation names mustn't include any digits, or it'll confu=
se
+	 * postgresExplainForeignScan.
 	 */
-	fpinfo->relation_name =3D makeStringInfo();
-	appendStringInfo(fpinfo->relation_name, "(%s) %s JOIN (%s)",
-					 fpinfo_o->relation_name->data,
-					 get_jointype_name(fpinfo->jointype),
-					 fpinfo_i->relation_name->data);
+	fpinfo->relation_name =3D psprintf("(%s) %s JOIN (%s)",
+									 fpinfo_o->relation_name,
+									 get_jointype_name(fpinfo->jointype),
+									 fpinfo_i->relation_name);
 =

 	/*
 	 * Set the relation index.  This is defined as the position of this
@@ -5723,11 +5776,12 @@ foreign_grouping_ok(PlannerInfo *root, RelOptInfo =
*grouped_rel,
 =

 	/*
 	 * Set the string describing this grouped relation to be used in EXPLAIN
-	 * output of corresponding ForeignScan.
+	 * output of corresponding ForeignScan.  Note that the decoration we add
+	 * to the base relation name mustn't include any digits, or it'll confus=
e
+	 * postgresExplainForeignScan.
 	 */
-	fpinfo->relation_name =3D makeStringInfo();
-	appendStringInfo(fpinfo->relation_name, "Aggregate on (%s)",
-					 ofpinfo->relation_name->data);
+	fpinfo->relation_name =3D psprintf("Aggregate on (%s)",
+									 ofpinfo->relation_name);
 =

 	return true;
 }
diff --git a/contrib/postgres_fdw/postgres_fdw.h b/contrib/postgres_fdw/po=
stgres_fdw.h
index bdefe0c..ea05287 100644
--- a/contrib/postgres_fdw/postgres_fdw.h
+++ b/contrib/postgres_fdw/postgres_fdw.h
@@ -87,11 +87,14 @@ typedef struct PgFdwRelationInfo
 	int			fetch_size;		/* fetch size for this remote table */
 =

 	/*
-	 * Name of the relation while EXPLAINing ForeignScan. It is used for joi=
n
-	 * relations but is set for all relations. For join relation, the name
-	 * indicates which foreign tables are being joined and the join type use=
d.
+	 * Name of the relation, for use while EXPLAINing ForeignScan.  It is us=
ed
+	 * for join and upper relations but is set for all relations.  For a bas=
e
+	 * relation, this is really just the RT index as a string; we convert th=
at
+	 * while producing EXPLAIN output.  For join and upper relations, the na=
me
+	 * indicates which base foreign tables are included and the join type or
+	 * aggregation type used.
 	 */
-	StringInfo	relation_name;
+	char	   *relation_name;
 =

 	/* Join information */
 	RelOptInfo *outerrel;

------- =_aaaaaaaaaa0--