Re: BUG #17606: There is still some glitch in 3f7323cbb fixing failure of MULTIEXPR_SUBLINK - Mailing list pgsql-bugs

I wrote:
> After contemplating that for awhile, by far the least painful way
> seems to be to add the subLinkId to struct SubPlan.  We can get
> away with that in the back branches as long as we add it at the
> end, since SubPlans don't get recorded in the catalogs.

The attached seems to fix it.  While writing a test case I discovered
that the code I'd stolen from ruleutils.c is inadequate for the purpose.
That code was only designed to deal with pre-planning query trees,
so it only expects one Param per targetlist entry --- but cases like
multiple assignments to the same array column can get rolled up into
nests containing several Params (cf. rewriteTargetListIU).  So I abandoned
that tactic in favor of just writing a tree walker.  Andre's gut feeling
that we needed one was right.

            regards, tom lane

diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index 8db09fda63..692b6c1559 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -1719,6 +1719,7 @@ _copySubPlan(const SubPlan *from)
     COPY_NODE_FIELD(args);
     COPY_SCALAR_FIELD(startup_cost);
     COPY_SCALAR_FIELD(per_call_cost);
+    COPY_SCALAR_FIELD(subLinkId);

     return newnode;
 }
diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c
index 092fb37923..337be2f896 100644
--- a/src/backend/nodes/equalfuncs.c
+++ b/src/backend/nodes/equalfuncs.c
@@ -455,6 +455,7 @@ _equalSubPlan(const SubPlan *a, const SubPlan *b)
     COMPARE_NODE_FIELD(args);
     COMPARE_SCALAR_FIELD(startup_cost);
     COMPARE_SCALAR_FIELD(per_call_cost);
+    COMPARE_SCALAR_FIELD(subLinkId);

     return true;
 }
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index 4547eed43b..21ececf0c2 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -1349,6 +1349,7 @@ _outSubPlan(StringInfo str, const SubPlan *node)
     WRITE_NODE_FIELD(args);
     WRITE_FLOAT_FIELD(startup_cost, "%.2f");
     WRITE_FLOAT_FIELD(per_call_cost, "%.2f");
+    WRITE_INT_FIELD(subLinkId);
 }

 static void
diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c
index 62c945b6c5..7976b369ba 100644
--- a/src/backend/nodes/readfuncs.c
+++ b/src/backend/nodes/readfuncs.c
@@ -2536,6 +2536,7 @@ _readSubPlan(void)
     READ_NODE_FIELD(args);
     READ_FLOAT_FIELD(startup_cost);
     READ_FLOAT_FIELD(per_call_cost);
+    READ_INT_FIELD(subLinkId);

     READ_DONE();
 }
diff --git a/src/backend/optimizer/plan/subselect.c b/src/backend/optimizer/plan/subselect.c
index a873d72b3a..7a4c85faf2 100644
--- a/src/backend/optimizer/plan/subselect.c
+++ b/src/backend/optimizer/plan/subselect.c
@@ -86,6 +86,7 @@ static bool subplan_is_hashable(Plan *plan);
 static bool testexpr_is_hashable(Node *testexpr, List *param_ids);
 static bool test_opexpr_is_hashable(OpExpr *testexpr, List *param_ids);
 static bool hash_ok_operator(OpExpr *expr);
+static bool SS_make_multiexprs_unique_walker(Node *node, void *context);
 static bool contain_dml(Node *node);
 static bool contain_dml_walker(Node *node, void *context);
 static bool contain_outer_selfref(Node *node);
@@ -335,6 +336,7 @@ build_subplan(PlannerInfo *root, Plan *plan, PlannerInfo *subroot,
      */
     splan = makeNode(SubPlan);
     splan->subLinkType = subLinkType;
+    splan->subLinkId = subLinkId;
     splan->testexpr = NULL;
     splan->paramIds = NIL;
     get_first_col_type(plan, &splan->firstColType, &splan->firstColTypmod,
@@ -858,12 +860,17 @@ hash_ok_operator(OpExpr *expr)
  * SubPlans, inheritance_planner() must call this to assign new, unique Param
  * IDs to the cloned MULTIEXPR_SUBLINKs' output parameters.  See notes in
  * ExecScanSubPlan.
+ *
+ * We do not need to re-map Param IDs for MULTIEXPR_SUBLINK plans that are
+ * initplans, because those don't have input parameters that could cause
+ * confusion.  Such initplans will not appear in the targetlist anyway, but
+ * they still complicate matters because the surviving regular subplans might
+ * now not have consecutive subLinkIds.
  */
 void
 SS_make_multiexprs_unique(PlannerInfo *root, PlannerInfo *subroot)
 {
-    List       *new_multiexpr_params = NIL;
-    int            offset;
+    List       *param_mapping = NIL;
     ListCell   *lc;

     /*
@@ -876,6 +883,9 @@ SS_make_multiexprs_unique(PlannerInfo *root, PlannerInfo *subroot)
         SubPlan    *splan;
         Plan       *plan;
         List       *params;
+        int            oldId,
+                    newId;
+        ListCell   *lc2;

         if (!IsA(tent->expr, SubPlan))
             continue;
@@ -898,86 +908,77 @@ SS_make_multiexprs_unique(PlannerInfo *root, PlannerInfo *subroot)
                                           &splan->setParam);

         /*
-         * We will append the replacement-Params lists to
-         * root->multiexpr_params, but for the moment just make a local list.
-         * Since we lack easy access here to the original subLinkId, we have
-         * to fall back on the slightly shaky assumption that the MULTIEXPR
-         * SubPlans appear in the targetlist in subLinkId order.  This should
-         * be safe enough given the way that the parser builds the targetlist
-         * today.  I wouldn't want to rely on it going forward, but since this
-         * code has a limited lifespan it should be fine.  We can partially
-         * protect against problems with assertions below.
+         * Append the new replacement-Params list to root->multiexpr_params.
+         * Then its index in that list becomes the new subPlanId of the
+         * SubPlan.
          */
-        new_multiexpr_params = lappend(new_multiexpr_params, params);
+        root->multiexpr_params = lappend(root->multiexpr_params, params);
+        oldId = splan->subLinkId;
+        newId = list_length(root->multiexpr_params);
+        Assert(newId > oldId);
+        splan->subLinkId = newId;
+
+        /*
+         * Add a mapping entry to param_mapping so that we can update the
+         * associated Params below.  Leave zeroes in the list for any
+         * subLinkIds we don't encounter; those must have been converted to
+         * initplans.
+         */
+        while (list_length(param_mapping) <= oldId)
+            param_mapping = lappend_int(param_mapping, 0);
+        lc2 = list_nth_cell(param_mapping, oldId);
+        lfirst_int(lc2) = newId;
     }

     /*
-     * Now we must find the Param nodes that reference the MULTIEXPR outputs
-     * and update their sublink IDs so they'll reference the new outputs.
-     * Fortunately, those too must be in the cloned targetlist, but they could
-     * be buried under FieldStores and SubscriptingRefs and CoerceToDomains
-     * (cf processIndirection()), and underneath those there could be an
-     * implicit type coercion.
+     * Unless all the MULTIEXPRs were converted to initplans, we must now find
+     * the Param nodes that reference the MULTIEXPR outputs and update their
+     * sublink IDs so they'll reference the new outputs.  While such Params
+     * must be in the cloned targetlist, they could be buried under stuff such
+     * as FieldStores and SubscriptingRefs and type coercions.
      */
-    offset = list_length(root->multiexpr_params);
+    if (param_mapping != NIL)
+        SS_make_multiexprs_unique_walker((Node *) subroot->parse->targetList,
+                                         (void *) param_mapping);
+}

-    foreach(lc, subroot->parse->targetList)
+/*
+ * Locate PARAM_MULTIEXPR Params in an expression tree, and update as needed.
+ * (We can update-in-place because the tree was already copied.)
+ */
+static bool
+SS_make_multiexprs_unique_walker(Node *node, void *context)
+{
+    if (node == NULL)
+        return false;
+    if (IsA(node, Param))
     {
-        TargetEntry *tent = (TargetEntry *) lfirst(lc);
-        Node       *expr;
-        Param       *p;
+        Param       *p = (Param *) node;
+        List       *param_mapping = (List *) context;
         int            subqueryid;
         int            colno;
+        int            newId;

-        expr = (Node *) tent->expr;
-        while (expr)
-        {
-            if (IsA(expr, FieldStore))
-            {
-                FieldStore *fstore = (FieldStore *) expr;
-
-                expr = (Node *) linitial(fstore->newvals);
-            }
-            else if (IsA(expr, SubscriptingRef))
-            {
-                SubscriptingRef *sbsref = (SubscriptingRef *) expr;
-
-                if (sbsref->refassgnexpr == NULL)
-                    break;
-
-                expr = (Node *) sbsref->refassgnexpr;
-            }
-            else if (IsA(expr, CoerceToDomain))
-            {
-                CoerceToDomain *cdomain = (CoerceToDomain *) expr;
-
-                if (cdomain->coercionformat != COERCE_IMPLICIT_CAST)
-                    break;
-                expr = (Node *) cdomain->arg;
-            }
-            else
-                break;
-        }
-        expr = strip_implicit_coercions(expr);
-        if (expr == NULL || !IsA(expr, Param))
-            continue;
-        p = (Param *) expr;
         if (p->paramkind != PARAM_MULTIEXPR)
-            continue;
+            return false;
         subqueryid = p->paramid >> 16;
         colno = p->paramid & 0xFFFF;
-        Assert(subqueryid > 0 &&
-               subqueryid <= list_length(new_multiexpr_params));
-        Assert(colno > 0 &&
-               colno <= list_length((List *) list_nth(new_multiexpr_params,
-                                                      subqueryid - 1)));
-        subqueryid += offset;
-        p->paramid = (subqueryid << 16) + colno;
-    }

-    /* Finally, attach new replacement lists to the global list */
-    root->multiexpr_params = list_concat(root->multiexpr_params,
-                                         new_multiexpr_params);
+        /*
+         * If subqueryid doesn't have a mapping entry, it must refer to an
+         * initplan, so don't change the Param.
+         */
+        Assert(subqueryid > 0);
+        if (subqueryid >= list_length(param_mapping))
+            return false;
+        newId = list_nth_int(param_mapping, subqueryid);
+        if (newId == 0)
+            return false;
+        p->paramid = (newId << 16) + colno;
+        return false;
+    }
+    return expression_tree_walker(node, SS_make_multiexprs_unique_walker,
+                                  context);
 }


@@ -1110,6 +1111,7 @@ SS_process_ctes(PlannerInfo *root)
          */
         splan = makeNode(SubPlan);
         splan->subLinkType = CTE_SUBLINK;
+        splan->subLinkId = 0;
         splan->testexpr = NULL;
         splan->paramIds = NIL;
         get_first_col_type(plan, &splan->firstColType, &splan->firstColTypmod,
@@ -3075,6 +3077,7 @@ SS_make_initplan_from_plan(PlannerInfo *root,
      */
     node = makeNode(SubPlan);
     node->subLinkType = EXPR_SUBLINK;
+    node->subLinkId = 0;
     node->plan_id = list_length(root->glob->subplans);
     node->plan_name = psprintf("InitPlan %d (returns $%d)",
                                node->plan_id, prm->paramid);
diff --git a/src/include/nodes/primnodes.h b/src/include/nodes/primnodes.h
index d73be2ad46..cfc4f5a9a7 100644
--- a/src/include/nodes/primnodes.h
+++ b/src/include/nodes/primnodes.h
@@ -731,6 +731,8 @@ typedef struct SubPlan
     /* Estimated execution costs: */
     Cost        startup_cost;    /* one-time setup cost */
     Cost        per_call_cost;    /* cost for each subplan evaluation */
+    /* Copied from original SubLink, but placed at end for ABI stability */
+    int            subLinkId;        /* ID (1..n); 0 if not MULTIEXPR */
 } SubPlan;

 /*
diff --git a/src/test/regress/expected/inherit.out b/src/test/regress/expected/inherit.out
index ed99bdde7f..658fed7922 100644
--- a/src/test/regress/expected/inherit.out
+++ b/src/test/regress/expected/inherit.out
@@ -1717,23 +1717,36 @@ reset enable_bitmapscan;
 --
 -- Check handling of MULTIEXPR SubPlans in inherited updates
 --
-create table inhpar(f1 int, f2 text[]);
+create table inhpar(f1 int, f2 text[], f3 int);
 insert into inhpar select generate_series(1,10);
 create table inhcld() inherits(inhpar);
 insert into inhcld select generate_series(11,10000);
 vacuum analyze inhcld;
 vacuum analyze inhpar;
 explain (verbose, costs off)
-update inhpar set (f1, f2[1]) = (select p2.unique2, p2.stringu1
-                                 from int4_tbl limit 1)
+update inhpar set
+  (f1, f2[1])    = (select p2.unique2, p2.stringu1 from int4_tbl limit 1),
+  (f2[2], f2[3]) = (select 'x', 'y' from int4_tbl limit 1),
+  (f3, f2[4])    = (select p2.unique2, p2.stringu1 from int4_tbl limit 1),
+  (f2[5], f2[6]) = (select 'x', 'y' from int4_tbl limit 1)
 from onek p2 where inhpar.f1 = p2.unique1;
-                                          QUERY PLAN
------------------------------------------------------------------------------------------------
+                                                                                                       QUERY PLAN
                                                                                                  

+-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
  Update on public.inhpar
    Update on public.inhpar
    Update on public.inhcld inhpar_1
+   InitPlan 2 (returns $4,$5)
+     ->  Limit
+           Output: 'x'::text, 'y'::text
+           ->  Seq Scan on public.int4_tbl int4_tbl_1
+                 Output: 'x'::text, 'y'::text
+   InitPlan 4 (returns $10,$11)
+     ->  Limit
+           Output: 'x'::text, 'y'::text
+           ->  Seq Scan on public.int4_tbl int4_tbl_3
+                 Output: 'x'::text, 'y'::text
    ->  Merge Join
-         Output: $4, inhpar.f2[1] := $5, (SubPlan 1 (returns $2,$3)), inhpar.ctid, p2.ctid
+         Output: $12, (((((inhpar.f2[1] := $13)[2] := $4)[3] := $5)[4] := $15)[5] := $10)[6] := $11, $14, (SubPlan 1
(returns$2,$3)), NULL::record, (SubPlan 3 (returns $8,$9)), NULL::record, inhpar.ctid, p2.ctid 
          Merge Cond: (p2.unique1 = inhpar.f1)
          ->  Index Scan using onek_unique1 on public.onek p2
                Output: p2.unique2, p2.stringu1, p2.ctid, p2.unique1
@@ -1747,8 +1760,13 @@ from onek p2 where inhpar.f1 = p2.unique1;
                  Output: (p2.unique2), (p2.stringu1)
                  ->  Seq Scan on public.int4_tbl
                        Output: p2.unique2, p2.stringu1
+         SubPlan 3 (returns $8,$9)
+           ->  Limit
+                 Output: (p2.unique2), (p2.stringu1)
+                 ->  Seq Scan on public.int4_tbl int4_tbl_2
+                       Output: p2.unique2, p2.stringu1
    ->  Hash Join
-         Output: $6, inhpar_1.f2[1] := $7, (SubPlan 1 (returns $2,$3)), inhpar_1.ctid, p2.ctid
+         Output: $16, (((((inhpar_1.f2[1] := $17)[2] := $4)[3] := $5)[4] := $19)[5] := $10)[6] := $11, $18, (SubPlan 1
(returns$2,$3)), NULL::record, (SubPlan 3 (returns $8,$9)), NULL::record, inhpar_1.ctid, p2.ctid 
          Hash Cond: (inhpar_1.f1 = p2.unique1)
          ->  Seq Scan on public.inhcld inhpar_1
                Output: inhpar_1.f2, inhpar_1.ctid, inhpar_1.f1
@@ -1756,10 +1774,13 @@ from onek p2 where inhpar.f1 = p2.unique1;
                Output: p2.unique2, p2.stringu1, p2.ctid, p2.unique1
                ->  Seq Scan on public.onek p2
                      Output: p2.unique2, p2.stringu1, p2.ctid, p2.unique1
-(27 rows)
+(42 rows)

-update inhpar set (f1, f2[1]) = (select p2.unique2, p2.stringu1
-                                 from int4_tbl limit 1)
+update inhpar set
+  (f1, f2[1])    = (select p2.unique2, p2.stringu1 from int4_tbl limit 1),
+  (f2[2], f2[3]) = (select 'x', 'y' from int4_tbl limit 1),
+  (f3, f2[4])    = (select p2.unique2, p2.stringu1 from int4_tbl limit 1),
+  (f2[5], f2[6]) = (select 'x', 'y' from int4_tbl limit 1)
 from onek p2 where inhpar.f1 = p2.unique1;
 drop table inhpar cascade;
 NOTICE:  drop cascades to table inhcld
diff --git a/src/test/regress/sql/inherit.sql b/src/test/regress/sql/inherit.sql
index 7d3813704e..a4a33a3da8 100644
--- a/src/test/regress/sql/inherit.sql
+++ b/src/test/regress/sql/inherit.sql
@@ -632,7 +632,7 @@ reset enable_bitmapscan;
 --
 -- Check handling of MULTIEXPR SubPlans in inherited updates
 --
-create table inhpar(f1 int, f2 text[]);
+create table inhpar(f1 int, f2 text[], f3 int);
 insert into inhpar select generate_series(1,10);
 create table inhcld() inherits(inhpar);
 insert into inhcld select generate_series(11,10000);
@@ -640,11 +640,17 @@ vacuum analyze inhcld;
 vacuum analyze inhpar;

 explain (verbose, costs off)
-update inhpar set (f1, f2[1]) = (select p2.unique2, p2.stringu1
-                                 from int4_tbl limit 1)
+update inhpar set
+  (f1, f2[1])    = (select p2.unique2, p2.stringu1 from int4_tbl limit 1),
+  (f2[2], f2[3]) = (select 'x', 'y' from int4_tbl limit 1),
+  (f3, f2[4])    = (select p2.unique2, p2.stringu1 from int4_tbl limit 1),
+  (f2[5], f2[6]) = (select 'x', 'y' from int4_tbl limit 1)
 from onek p2 where inhpar.f1 = p2.unique1;
-update inhpar set (f1, f2[1]) = (select p2.unique2, p2.stringu1
-                                 from int4_tbl limit 1)
+update inhpar set
+  (f1, f2[1])    = (select p2.unique2, p2.stringu1 from int4_tbl limit 1),
+  (f2[2], f2[3]) = (select 'x', 'y' from int4_tbl limit 1),
+  (f3, f2[4])    = (select p2.unique2, p2.stringu1 from int4_tbl limit 1),
+  (f2[5], f2[6]) = (select 'x', 'y' from int4_tbl limit 1)
 from onek p2 where inhpar.f1 = p2.unique1;

 drop table inhpar cascade;