Re: Problems with "pg.dropped" column after upgrade 9.5 to 9.6

Michael Paquier <michael.paquier@gmail.com> writes:
> I can reproduce the regression on master and 9.6:
> DROP TABLE aa;
> CREATE TABLE aa (a int, b int, c int);
> CREATE OR REPLACE FUNCTION _test_sql_update(in do_update boolean)
>     RETURNS VOID LANGUAGE sql VOLATILE AS $$
>           update aa
>            set a = NULL
>           where do_update;
>     $$;
> ALTER TABLE aa DROP COLUMN b;

OK, I looked into this enough to identify exactly where it broke.
The targetlist as set up by preptlist.c looks like

    null Const    -- value being assigned to a
    null Const    -- placeholder for dropped b column
    Var for c    -- to copy updated row's previous c value forward
    Var for ctid    -- since ModifyTable will need the row TID (resjunk)

The plan tree needs to contain a SeqScan on aa, returning at least values
for c and ctid, plus a Result node that will gate execution based on the
"do_update" parameter, and then a ModifyTable on top.  For safety's sake,
the ModifyTable wants to check that the output of its child node matches
the table's current actual rowtype; in particular it wants to see that
the value being put into b is null, since it knows that column is dropped.

In previous releases, the plan tree returned by create_plan() was just
the SeqScan and Result, and the tlists that those output contained just
"c" and "ctid", ie only the required Vars.  Then grouping_planner pasted
the final tlist onto just the top plan node (the Result).

As of 9.6, create_plan is aware that it ought to return the final tlist,
and it sets up the SeqScan to do that --- then, more or less as an
afterthought (see create_gating_plan), it pastes a Result on top
producing the same tlist.  So at this point we have Const,Const,Var,Var
in both plan nodes, and you'd think that should still be fine.  But it
isn't, because setrefs.c processes the Result's tlist to replace Vars
in it with references to the outputs of the child SeqScan.  And it will
also notice that the Consts match expressions in the child, and replace
them with Vars.  So now the Result's tlist contains four Vars referencing
the child output columns, and that makes ExecCheckPlanOutput unhappy.

The fact that this didn't happen before seems to hinge entirely on the
fact that in prior versions create_plan always returned tlists containing
only Vars, so that there was no opportunity for setrefs.c to match a Const
to lower-level output unless there was more than one level of Plan node
stuck on top of that result within planner.c --- and in an INSERT or
UPDATE, there would be nothing stuck on top except the ModifyTable.

So I'm pretty tempted to fix it as per attached, that is, teach setrefs.c
that it's silly to try to convert a Const into a Var.  It is sane to
replace more-complicated expressions with Vars, because that means we save
re-evaluating those expressions.  But a quick look at ExecEvalConst and
ExecEvalVar shows that a Const is strictly cheaper to execute than a Var,
so doing such a conversion is actually a pessimization.

This doesn't seem to make for any changes in the core regression tests,
but postgres_fdw notices, eg here:

***************
*** 2450,2456 ****
     Output: (count(c2)), c2, (5), (7.0), (9)
     Sort Key: ft1.c2
     ->  Foreign Scan
!          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)
--- 2450,2456 ----
     Output: (count(c2)), c2, (5), (7.0), (9)
     Sort Key: ft1.c2
     ->  Foreign Scan
!          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)

Losing the indirection doesn't seem like a problem, but I wonder if
anyone's concerned about it.

            regards, tom lane

diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c
index d10a983..cd10794 100644
*** a/src/backend/optimizer/plan/setrefs.c
--- b/src/backend/optimizer/plan/setrefs.c
*************** search_indexed_tlist_for_non_var(Node *n
*** 2010,2015 ****
--- 2010,2024 ----
  {
      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.  What's more, doing so 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)
      {