Repair bogus handling of multi-assignment Params in upper plan levels.

Our support for multiple-set-clauses in UPDATE assumes that the Params
referencing a MULTIEXPR_SUBLINK SubPlan will appear before that SubPlan
in the targetlist of the plan node that calculates the updated row.
(Yeah, it's a hack...)  In some PG branches it's possible that a Result
node gets inserted between the primary calculation of the update tlist
and the ModifyTable node.  setrefs.c did the wrong thing in this case
and left the upper-level Params as Params, causing a crash at runtime.
What it should do is replace them with "outer" Vars referencing the child
plan node's output.  That's a result of careless ordering of operations
in fix_upper_expr_mutator, so we can fix it just by reordering the code.

Fix fix_join_expr_mutator similarly for consistency, even though join
nodes could never appear in such a context.  (In general, it seems
likely to be a bit cheaper to use Vars than Params in such situations
anyway, so this patch might offer a tiny performance improvement.)

The hazard extends back to 9.5 where the MULTIEXPR_SUBLINK stuff
was introduced, so back-patch that far.  However, this may be a live
bug only in 9.6.x and 10.x, as the other branches don't seem to want
to calculate the final tlist below the Result node.  (That plan shape
change between branches might be a mini-bug in itself, but I'm not
really interested in digging into the reasons for that right now.
Still, add a regression test memorializing what we expect there,
so we'll notice if it changes again.)

Per bug report from Eduards Bezverhijs.

Discussion: https://postgr.es/m/b6cd572a-3e44-8785-75e9-c512a5a17a73@tieto.com

diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c
index ec376c05650daec1230a9b0fe6c749ddd6cc84d7..c4b5c4a242c3e2861375e332d12e2665e6528339 100644 (file)
--- a/src/backend/optimizer/plan/setrefs.c
+++ b/src/backend/optimizer/plan/setrefs.c
@@ -2210,8 +2210,6 @@ fix_join_expr_mutator(Node *node, fix_join_expr_context *context)
                /* If not supplied by input plans, evaluate the contained expr */
                return fix_join_expr_mutator((Node *) phv->phexpr, context);
        }
-       if (IsA(node, Param))
-               return fix_param_node(context->root, (Param *) node);
        /* Try matching more complex expressions too, if tlists have any */
        if (context->outer_itlist && context->outer_itlist->has_non_vars)
        {
@@ -2229,6 +2227,9 @@ fix_join_expr_mutator(Node *node, fix_join_expr_context *context)
                if (newvar)
                        return (Node *) newvar;
        }
+       /* Special cases (apply only AFTER failing to match to lower tlist) */
+       if (IsA(node, Param))
+               return fix_param_node(context->root, (Param *) node);
        fix_expr_common(context->root, node);
        return expression_tree_mutator(node,
                                                                   fix_join_expr_mutator,
@@ -2316,6 +2317,16 @@ fix_upper_expr_mutator(Node *node, fix_upper_expr_context *context)
                /* If not supplied by input plan, evaluate the contained expr */
                return fix_upper_expr_mutator((Node *) phv->phexpr, context);
        }
+       /* Try matching more complex expressions too, if tlist has any */
+       if (context->subplan_itlist->has_non_vars)
+       {
+               newvar = search_indexed_tlist_for_non_var(node,
+                                                                                                 context->subplan_itlist,
+                                                                                                 context->newvarno);
+               if (newvar)
+                       return (Node *) newvar;
+       }
+       /* Special cases (apply only AFTER failing to match to lower tlist) */
        if (IsA(node, Param))
                return fix_param_node(context->root, (Param *) node);
        if (IsA(node, Aggref))
@@ -2340,15 +2351,6 @@ fix_upper_expr_mutator(Node *node, fix_upper_expr_context *context)
                }
                /* If no match, just fall through to process it normally */
        }
-       /* Try matching more complex expressions too, if tlist has any */
-       if (context->subplan_itlist->has_non_vars)
-       {
-               newvar = search_indexed_tlist_for_non_var(node,
-                                                                                                 context->subplan_itlist,
-                                                                                                 context->newvarno);
-               if (newvar)
-                       return (Node *) newvar;
-       }
        fix_expr_common(context->root, node);
        return expression_tree_mutator(node,
                                                                   fix_upper_expr_mutator,

diff --git a/src/test/regress/expected/update.out b/src/test/regress/expected/update.out
index 49730ea3c5535fe50d306a7338d45d5d6ced70b8..cb2743fe079a8ec67f96b05479fae998eeb88d7d 100644 (file)
--- a/src/test/regress/expected/update.out
+++ b/src/test/regress/expected/update.out
@@ -169,6 +169,37 @@ SELECT a, b, char_length(c) FROM update_test;
  42 |  12 |       10000
 (4 rows)
 
+-- Check multi-assignment with a Result node to handle a one-time filter.
+EXPLAIN (VERBOSE, COSTS OFF)
+UPDATE update_test t
+  SET (a, b) = (SELECT b, a FROM update_test s WHERE s.a = t.a)
+  WHERE CURRENT_USER = SESSION_USER;
+                               QUERY PLAN                               
+------------------------------------------------------------------------
+ Update on public.update_test t
+   ->  Result
+         Output: ($1), ($2), t.c, ((SubPlan 1 (returns $1,$2))), t.ctid
+         One-Time Filter: ("current_user"() = "session_user"())
+         ->  Seq Scan on public.update_test t
+               Output: $1, $2, t.c, (SubPlan 1 (returns $1,$2)), t.ctid
+               SubPlan 1 (returns $1,$2)
+                 ->  Seq Scan on public.update_test s
+                       Output: s.b, s.a
+                       Filter: (s.a = t.a)
+(10 rows)
+
+UPDATE update_test t
+  SET (a, b) = (SELECT b, a FROM update_test s WHERE s.a = t.a)
+  WHERE CURRENT_USER = SESSION_USER;
+SELECT a, b, char_length(c) FROM update_test;
+  a  | b  | char_length 
+-----+----+-------------
+ 101 | 21 |            
+     |    |            
+  12 | 41 |       10000
+  12 | 42 |       10000
+(4 rows)
+
 -- Test ON CONFLICT DO UPDATE
 INSERT INTO upsert_test VALUES(1, 'Boo');
 -- uncorrelated  sub-select:

diff --git a/src/test/regress/sql/update.sql b/src/test/regress/sql/update.sql
index e0cf5d12a9235a334430cd629f352fc61f7355a2..adfe2da4dae411cba5fad1f4e4b104fc7839d90b 100644 (file)
--- a/src/test/regress/sql/update.sql
+++ b/src/test/regress/sql/update.sql
@@ -88,6 +88,16 @@ UPDATE update_test AS t SET b = update_test.b + 10 WHERE t.a = 10;
 UPDATE update_test SET c = repeat('x', 10000) WHERE c = 'car';
 SELECT a, b, char_length(c) FROM update_test;
 
+-- Check multi-assignment with a Result node to handle a one-time filter.
+EXPLAIN (VERBOSE, COSTS OFF)
+UPDATE update_test t
+  SET (a, b) = (SELECT b, a FROM update_test s WHERE s.a = t.a)
+  WHERE CURRENT_USER = SESSION_USER;
+UPDATE update_test t
+  SET (a, b) = (SELECT b, a FROM update_test s WHERE s.a = t.a)
+  WHERE CURRENT_USER = SESSION_USER;
+SELECT a, b, char_length(c) FROM update_test;
+
 -- Test ON CONFLICT DO UPDATE
 INSERT INTO upsert_test VALUES(1, 'Boo');
 -- uncorrelated  sub-select: