From da8f3ebf30bef9c950595dc0d1f03bce2b1b8563 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Wed, 2 Nov 2016 14:32:13 -0400 Subject: [PATCH] Don't convert Consts into Vars during setrefs.c processing. MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit 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> --- .../postgres_fdw/expected/postgres_fdw.out | 8 +++---- src/backend/optimizer/plan/setrefs.c | 23 +++++++++++++++++++ src/test/regress/expected/inherit.out | 2 +- 3 files changed, 28 insertions(+), 5 deletions(-) diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out index 2745ad5652..7339b58dee 100644 --- 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 d10a98396c..d91bc3b30d 100644 --- 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 df7cba6614..79e9969d57 100644 --- 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) -- 2.40.0