Don't convert Consts into Vars during setrefs.c processing.

While converting expressions in an upper-level plan node so that they
reference Vars and expressions provided by the input plan node(s),
don't convert plain Const items, even if there happens to be a matching
Const in the input.  It's silly to do so because a Var is more expensive to
execute than a Const.  Moreover, converting can fool ExecCheckPlanOutput's
check that an insert or update query inserts nulls into dropped columns,
leading to "query provides a value for a dropped column" errors during
INSERT or UPDATE on a table with a dropped column.  We could solve this
by making that check more complicated, but I don't see the point; this fix
should save a marginal number of cycles, and it also makes for less messy
EXPLAIN output, as shown by the ensuing regression test result changes.

Per report from Pavel Hanák.  I have not incorporated a test case based
on that example, as there doesn't seem to be a simple way of checking
this in isolation without making a bunch of assumptions about other
planner and SQL-function behavior.

Back-patch to 9.6.  This setrefs.c behavior exists much further back,
but there is not currently reason to think that it causes problems
before 9.6.

Discussion: <83shraampf.fsf@is-it.eu>

diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index 2745ad56524a9cda4b553a6eb96b5a24bdbb8547..7339b58deeac511c8aecaa32f2e3d0a36d46fbed 100644 (file)
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -2447,10 +2447,10 @@ select count(c2) w, c2 x, 5 y, 7.0 z from ft1 group by 2, y, 9.0::int order by 2
                                                 QUERY PLAN                                                
 ----------------------------------------------------------------------------------------------------------
  Sort
-   Output: (count(c2)), c2, (5), (7.0), (9)
+   Output: (count(c2)), c2, 5, 7.0, 9
    Sort Key: ft1.c2
    ->  Foreign Scan
-         Output: (count(c2)), c2, (5), (7.0), (9)
+         Output: (count(c2)), c2, 5, 7.0, 9
          Relations: Aggregate on (public.ft1)
          Remote SQL: SELECT count(c2), c2, 5, 7.0, 9 FROM "S 1"."T 1" GROUP BY c2, 5::integer, 9::integer
 (7 rows)
@@ -2499,7 +2499,7 @@ select count(*) from (select c5, count(c1) from ft1 group by c5, sqrt(c2) having
  Aggregate
    Output: count(*)
    ->  Foreign Scan
-         Output: ft1.c5, (NULL::bigint), (sqrt((ft1.c2)::double precision))
+         Output: ft1.c5, NULL::bigint, (sqrt((ft1.c2)::double precision))
          Filter: (((((avg(ft1.c1)) / (avg(ft1.c1))))::double precision * random()) <= '1'::double precision)
          Relations: Aggregate on (public.ft1)
          Remote SQL: SELECT c5, NULL::bigint, sqrt(c2), avg("C 1") FROM "S 1"."T 1" GROUP BY c5, (sqrt(c2)) HAVING ((avg("C 1") < 500::numeric))
@@ -3139,7 +3139,7 @@ select sum(q.a), count(q.b) from ft4 left join (select 13, avg(ft1.c1), sum(ft2.
                ->  Subquery Scan on q
                      Output: q.a, q.b
                      ->  Foreign Scan
-                           Output: (13), (avg(ft1.c1)), (NULL::bigint)
+                           Output: 13, (avg(ft1.c1)), NULL::bigint
                            Relations: Aggregate on ((public.ft2) LEFT JOIN (public.ft1))
                            Remote SQL: SELECT 13, avg(r1."C 1"), NULL::bigint FROM ("S 1"."T 1" r2 LEFT JOIN "S 1"."T 1" r1 ON (((r1."C 1" = r2."C 1"))))
 (16 rows)

diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c
index d10a98396c7f75566ed0064ba9fa392b2bc54d7a..d91bc3b30de5c80f9112f52003cb794de72688fe 100644 (file)
--- a/src/backend/optimizer/plan/setrefs.c
+++ b/src/backend/optimizer/plan/setrefs.c
@@ -1823,6 +1823,19 @@ set_dummy_tlist_references(Plan *plan, int rtoffset)
                Var                *oldvar = (Var *) tle->expr;
                Var                *newvar;
 
+               /*
+                * As in search_indexed_tlist_for_non_var(), we prefer to keep Consts
+                * as Consts, not Vars referencing Consts.  Here, there's no speed
+                * advantage to be had, but it makes EXPLAIN output look cleaner, and
+                * again it avoids confusing the executor.
+                */
+               if (IsA(oldvar, Const))
+               {
+                       /* just reuse the existing TLE node */
+                       output_targetlist = lappend(output_targetlist, tle);
+                       continue;
+               }
+
                newvar = makeVar(OUTER_VAR,
                                                 tle->resno,
                                                 exprType((Node *) oldvar),
@@ -2010,6 +2023,16 @@ search_indexed_tlist_for_non_var(Node *node,
 {
        TargetEntry *tle;
 
+       /*
+        * If it's a simple Const, replacing it with a Var is silly, even if there
+        * happens to be an identical Const below; a Var is more expensive to
+        * execute than a Const.  What's more, replacing it could confuse some
+        * places in the executor that expect to see simple Consts for, eg,
+        * dropped columns.
+        */
+       if (IsA(node, Const))
+               return NULL;
+
        tle = tlist_member(node, itlist->tlist);
        if (tle)
        {

diff --git a/src/test/regress/expected/inherit.out b/src/test/regress/expected/inherit.out
index df7cba6614dc31ea48c8110f03016c67b9111bb3..79e9969d574f5ce03a9d3207722b53109c6b9afe 100644 (file)
--- a/src/test/regress/expected/inherit.out
+++ b/src/test/regress/expected/inherit.out
@@ -1426,7 +1426,7 @@ ORDER BY thousand, tenthous;
    Sort Key: tenk1.thousand, tenk1.tenthous
    ->  Index Only Scan using tenk1_thous_tenthous on tenk1
    ->  Sort
-         Sort Key: (42), (42)
+         Sort Key: 42, 42
          ->  Index Only Scan using tenk1_hundred on tenk1 tenk1_1
 (6 rows)