Re: Bogus EXPLAIN results with column aliases for mismatched partitions

Поиск
Список
Период
Сортировка
От Tom Lane
Тема Re: Bogus EXPLAIN results with column aliases for mismatched partitions
Дата
Msg-id 10395.1575236078@sss.pgh.pa.us
обсуждение исходный текст
Ответ на Re: Bogus EXPLAIN results with column aliases for mismatched partitions  (Tom Lane <tgl@sss.pgh.pa.us>)
Ответы Re: Bogus EXPLAIN results with column aliases for mismatched partitions  (Etsuro Fujita <etsuro.fujita@gmail.com>)
Список pgsql-hackers
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

diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_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 WHERE 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 (public.ft5))
    Remote SQL: SELECT s4.c1, s10.c1, s10.c2 FROM ((SELECT c1 FROM "S 1"."T 3" WHERE ((c1 >= 50)) AND ((c1 <= 60)))
s4(c1)FULL JOIN (SELECT s8.c1, s9.c1 FROM ((SELECT c1 FROM "S 1"."T 3" WHERE ((c1 >= 50)) AND ((c1 <= 60))) s8(c1) FULL
JOIN(SELECT c1 FROM "S 1"."T 4" WHERE ((c1 >= 50)) AND ((c1 <= 60))) s9(c1) ON (((s8.c1 = s9.c1)))) WHERE (((s8.c1 IS
NULL)OR (s8.c1 IS NOT NULL)))) s10(c1, c2) ON (((s4.c1 = s10.c1)))) ORDER 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" FROM ("S 1"."T 1" r1 INNER JOIN "S 1"."T 1" r2
ON(((r1."C 1" = r2."C 1")))) 
                            ->  Foreign Scan
                                  Output: t1_1.c1, t2_1.c1
-                                 Relations: (public.ft1 t1) INNER JOIN (public.ft2 t2)
+                                 Relations: (public.ft1 t1_1) INNER JOIN (public.ft2 t2_1)
                                  Remote SQL: SELECT r1."C 1", r2."C 1" FROM ("S 1"."T 1" r1 INNER JOIN "S 1"."T 1" r2
ON(((r1."C 1" = 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 = t2.b) INNER JOIN fprt1 t3 ON (t2.b = t3.a) WHERE
t1.a% 25 =0 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.ftprt2_p1 t2)) INNER JOIN (public.ftprt1_p1 t3)
          ->  Foreign Scan
-               Relations: ((public.ftprt1_p2 t1) INNER JOIN (public.ftprt2_p2 t2)) INNER JOIN (public.ftprt1_p2 t3)
+               Relations: ((public.ftprt1_p2 t1_1) INNER JOIN (public.ftprt2_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 = t2.b) INNER JOIN fprt1 t3 ON (t2.b = t3.a) WHERE
t1.a% 25 =0 ORDER BY 1,2,3; 
@@ -8507,7 +8507,7 @@ SELECT t1.a,t2.b,t2.c FROM fprt1 t1 LEFT JOIN (SELECT * 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 = r6.b)) AND
((r5.b= 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
NULLSLAST 
 (4 rows)

@@ -8561,15 +8561,15 @@ SELECT t1.wr, t2.wr FROM (SELECT t1 wr, a FROM fprt1 t1 WHERE t1.a % 25 = 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 = t2.b AND t1.b = t2.a) q WHERE
t1.a%25= 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.ftprt2_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 = t2.b AND t1.b = t2.a) q WHERE
t1.a%25= 0 ORDER BY 1,2; 
@@ -8689,17 +8689,17 @@ SELECT a, sum(b), min(b), count(*) FROM pagg_tab GROUP 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/postgres_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 = 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 foreign
+     * table RTE.  (If this query gets EXPLAIN'd, we'll convert that to a
+     * human-readable string at that time.)
      */
-    fpinfo->relation_name = makeStringInfo();
-    namespace = get_namespace_name(get_rel_namespace(foreigntableid));
-    relname = get_rel_name(foreigntableid);
-    refname = rte->eref->aliasname;
-    appendStringInfo(fpinfo->relation_name, "%s.%s",
-                     quote_identifier(namespace),
-                     quote_identifier(relname));
-    if (*refname && strcmp(refname, relname) != 0)
-        appendStringInfo(fpinfo->relation_name, " %s",
-                         quote_identifier(rte->eref->aliasname));
+    fpinfo->relation_name = psprintf("%u", baserel->relid);

     /* No outer and inner relations. */
     fpinfo->make_outerrel_subquery = false;
@@ -1376,7 +1365,7 @@ postgresGetForeignPlan(PlannerInfo *root,
                              makeInteger(fpinfo->fetch_size));
     if (IS_JOIN_REL(foreignrel) || IS_UPPER_REL(foreignrel))
         fdw_private = 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 = ((ForeignScan *) node->ss.ps.plan)->fdw_private;
+    ForeignScan *plan = castNode(ForeignScan, node->ss.ps.plan);
+    List       *fdw_private = 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 must
+     * replace the digit strings 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)
     {
-        relations = strVal(list_nth(fdw_private, FdwScanPrivateRelations));
-        ExplainPropertyText("Relations", relations, es);
+        StringInfo    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_relids.  (We expect all the relids in
+         * the join will have been offset by the same amount.)
+         */
+        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_relids, -1) - minrti;
+
+        /* Now we can translate the string */
+        relations = makeStringInfo();
+        ptr = rawrelations;
+        while (*ptr)
+        {
+            if (isdigit((unsigned char) *ptr))
+            {
+                int            rti = strtol(ptr, &ptr, 10);
+                RangeTblEntry *rte;
+                char       *namespace;
+                char       *relname;
+                char       *refname;
+
+                rti += rtoffset;
+                Assert(bms_is_member(rti, plan->fs_relids));
+                rte = rt_fetch(rti, es->rtable);
+                Assert(rte->rtekind == RTE_RELATION);
+                namespace = get_namespace_name(get_rel_namespace(rte->relid));
+                relname = get_rel_name(rte->relid);
+                appendStringInfo(relations, "%s.%s",
+                                 quote_identifier(namespace),
+                                 quote_identifier(relname));
+                refname = (char *) list_nth(es->rtable_names, rti - 1);
+                if (refname && strcmp(refname, relname) != 0)
+                    appendStringInfo(relations, " %s",
+                                     quote_identifier(refname));
+            }
+            else
+                appendStringInfoChar(relations, *ptr++);
+        }
+        ExplainPropertyText("Relations", relations->data, es);
     }

     /*
@@ -2549,6 +2599,8 @@ postgresExplainForeignScan(ForeignScanState *node, ExplainState *es)
      */
     if (es->verbose)
     {
+        char       *sql;
+
         sql = strVal(list_nth(fdw_private, FdwScanPrivateSelectSql));
         ExplainPropertyText("Remote SQL", sql, es);
     }
@@ -5171,13 +5223,14 @@ foreign_join_ok(PlannerInfo *root, RelOptInfo *joinrel, 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 confuse
+     * postgresExplainForeignScan.
      */
-    fpinfo->relation_name = 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 = 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 confuse
+     * postgresExplainForeignScan.
      */
-    fpinfo->relation_name = makeStringInfo();
-    appendStringInfo(fpinfo->relation_name, "Aggregate on (%s)",
-                     ofpinfo->relation_name->data);
+    fpinfo->relation_name = psprintf("Aggregate on (%s)",
+                                     ofpinfo->relation_name);

     return true;
 }
diff --git a/contrib/postgres_fdw/postgres_fdw.h b/contrib/postgres_fdw/postgres_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 join
-     * relations but is set for all relations. For join relation, the name
-     * indicates which foreign tables are being joined and the join type used.
+     * Name of the relation, for use while EXPLAINing ForeignScan.  It is used
+     * for join and upper relations but is set for all relations.  For a base
+     * relation, this is really just the RT index as a string; we convert that
+     * while producing EXPLAIN output.  For join and upper relations, the name
+     * 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;

В списке pgsql-hackers по дате отправления:

Предыдущее
От: Tom Lane
Дата:
Сообщение: Re: Bogus EXPLAIN results with column aliases for mismatched partitions
Следующее
От: Andrew Dunstan
Дата:
Сообщение: Re: Allow 'sslkey' and 'sslcert' in postgres_fdw user mappings