Re: Adding OLD/NEW support to RETURNING - Mailing list pgsql-hackers

On Mon, Mar 18, 2024 at 6:48 PM Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>
> On Tue, 12 Mar 2024 at 18:21, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> >
> > Updated version attached tidying up a couple of things and fixing another bug:
> >
>
> Rebased version attached, on top of c649fa24a4 (MERGE ... RETURNING support).
>


hi, some minor issues I found out.

+/*
+ * ReplaceReturningVarsFromTargetList -
+ * replace RETURNING list Vars with items from a targetlist
+ *
+ * This is equivalent to calling ReplaceVarsFromTargetList() with a
+ * nomatch_option of REPLACEVARS_REPORT_ERROR, but with the added effect of
+ * copying varreturningtype onto any Vars referring to new_result_relation,
+ * allowing RETURNING OLD/NEW to work in the rewritten query.
+ */
+
+typedef struct
+{
+ ReplaceVarsFromTargetList_context rv_con;
+ int new_result_relation;
+} ReplaceReturningVarsFromTargetList_context;
+
+static Node *
+ReplaceReturningVarsFromTargetList_callback(Var *var,
+ replace_rte_variables_context *context)
+{
+ ReplaceReturningVarsFromTargetList_context *rcon =
(ReplaceReturningVarsFromTargetList_context *) context->callback_arg;
+ Node   *newnode;
+
+ newnode = ReplaceVarsFromTargetList_callback(var, context);
+
+ if (var->varreturningtype != VAR_RETURNING_DEFAULT)
+ SetVarReturningType((Node *) newnode, rcon->new_result_relation,
+ var->varlevelsup, var->varreturningtype);
+
+ return newnode;
+}
+
+Node *
+ReplaceReturningVarsFromTargetList(Node *node,
+   int target_varno, int sublevels_up,
+   RangeTblEntry *target_rte,
+   List *targetlist,
+   int new_result_relation,
+   bool *outer_hasSubLinks)
+{
+ ReplaceReturningVarsFromTargetList_context context;
+
+ context.rv_con.target_rte = target_rte;
+ context.rv_con.targetlist = targetlist;
+ context.rv_con.nomatch_option = REPLACEVARS_REPORT_ERROR;
+ context.rv_con.nomatch_varno = 0;
+ context.new_result_relation = new_result_relation;
+
+ return replace_rte_variables(node, target_varno, sublevels_up,
+ ReplaceReturningVarsFromTargetList_callback,
+ (void *) &context,
+ outer_hasSubLinks);
+}

the ReplaceReturningVarsFromTargetList related comment
should be placed right above the function ReplaceReturningVarsFromTargetList,
not above ReplaceReturningVarsFromTargetList_context?

struct ReplaceReturningVarsFromTargetList_context adds some comments
about new_result_relation would be great.


/* INDEX_VAR is handled by default case */
this comment appears in execExpr.c and execExprInterp.c.
need to move to default case's switch default case?


/*
 * set_deparse_context_plan - Specify Plan node containing expression
 *
 * When deparsing an expression in a Plan tree, we might have to resolve
 * OUTER_VAR, INNER_VAR, or INDEX_VAR references.  To do this, the caller must
 * provide the parent Plan node.
...
*/
does the comment in set_deparse_context_plan need to be updated?

+ * buildNSItemForReturning -
+ * add a ParseNamespaceItem for the OLD or NEW alias in RETURNING.
+ */
+static void
+addNSItemForReturning(ParseState *pstate, const char *aliasname,
+  VarReturningType returning_type)
comment "buildNSItemForReturning" should be "addNSItemForReturning"?


  * results.  If include_dropped is true then empty strings and NULL constants
  * (not Vars!) are returned for dropped columns.
  *
- * rtindex, sublevels_up, and location are the varno, varlevelsup, and location
- * values to use in the created Vars.  Ordinarily rtindex should match the
- * actual position of the RTE in its rangetable.
+ * rtindex, sublevels_up, returning_type, and location are the varno,
+ * varlevelsup, varreturningtype, and location values to use in the created
+ * Vars.  Ordinarily rtindex should match the actual position of the RTE in
+ * its rangetable.
we already updated the comment in expandRTE.
but it seems we only do RTE_RELATION, some part of RTE_FUNCTION.
do we need
`
varnode->varreturningtype = returning_type;
`
for other `rte->rtekind` when there is a makeVar?

(I don't understand this part, in the case where rte->rtekind is
RTE_SUBQUERY, if I add  `varnode->varreturningtype = returning_type;`
the tests still pass.