Re: Problems with "pg.dropped" column after upgrade 9.5 to 9.6 - Mailing list pgsql-bugs
From | Tom Lane |
---|---|
Subject | Re: Problems with "pg.dropped" column after upgrade 9.5 to 9.6 |
Date | |
Msg-id | 12846.1478101936@sss.pgh.pa.us Whole thread Raw |
In response to | Re: Problems with "pg.dropped" column after upgrade 9.5 to 9.6 (Michael Paquier <michael.paquier@gmail.com>) |
Responses |
Re: Problems with "pg.dropped" column after upgrade 9.5 to 9.6
|
List | pgsql-bugs |
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) {
pgsql-bugs by date: