Re: Adding OLD/NEW support to RETURNING

On Sat, Jul 13, 2024 at 1:22 AM Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>
> On Wed, 26 Jun 2024 at 12:18, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> >
> > I've added a new elog() error check to
> > adjust_appendrel_attrs_mutator(), similar to the existing one for
> > varnullingrels, to report if we ever attempt to apply a non-default
> > varreturningtype to a non-Var, which should never be possible, but
> > seems worth checking. (non-Var expressions should only occur if we've
> > flattened a UNION ALL query, so shouldn't apply to the target relation
> > of a data-modifying query with RETURNING.)
> >
>
> New version attached, updating an earlier comment in
> adjust_appendrel_attrs_mutator() that I had missed.
>


hi.
I have some minor questions, but overall it just works.

@@ -4884,6 +5167,18 @@ ExecEvalSysVar(ExprState *state, ExprEva
 {
  Datum d;

+ /* if OLD/NEW row doesn't exist, OLD/NEW system attribute is NULL */
+ if ((op->d.var.varreturningtype == VAR_RETURNING_OLD &&
+ state->flags & EEO_FLAG_OLD_IS_NULL) ||
+ (op->d.var.varreturningtype == VAR_RETURNING_NEW &&
+ state->flags & EEO_FLAG_NEW_IS_NULL))
+ {
+ *op->resvalue = (Datum) 0;
+ *op->resnull = true;
+
+ return;
+ }
+
in ExecEvalSysVar, we can add Asserts
Assert(state->flags & EEO_FLAG_HAS_OLD || state->flags & EEO_FLAG_HAS_NEW);
if I understand it correctly.


in make_modifytable,
contain_vars_returning_old_or_new((Node *) root->parse->returningList))
this don't need to go through the loop
```
foreach(lc, resultRelations)
```


+ * In addition, the caller must provide result_relation, the index of the
+ * target relation for an INSERT/UPDATE/DELETE/MERGE.  This is needed to
+ * handle any OLD/NEW RETURNING list Vars referencing target_varno.  When such
+ * Vars are expanded, varreturningtype is copied onto any replacement Vars
+ * that reference result_relation.  In addition, if the replacement expression
+ * from the targetlist is not simply a Var referencing result_relation, we
+ * wrap it in a ReturningExpr node, to force it to be NULL if the OLD/NEW row
+ * doesn't exist.
+ *
  * outer_hasSubLinks works the same as for replace_rte_variables().
  */
@@ -1657,6 +1736,7 @@ typedef struct
 {
  RangeTblEntry *target_rte;
  List   *targetlist;
+ int result_relation;
  ReplaceVarsNoMatchOption nomatch_option;
  int nomatch_varno;
 } ReplaceVarsFromTargetList_context;

"to force it to be NULL if the OLD/NEW row doesn't exist."
I am slightly confused.
i think you mean: "to force it to be NULL if the OLD/NEW row will be
resulting null."
For INSERT,  the old row is all null, for DELETE, the new row is all null.



in sql-update.html
"An unqualified column name or * causes new values to be returned. The
same applies to columns qualified using the target table name or
alias. "
"The same", I think, refers "causes new values to be returned", but I
i am not so sure.
(apply to sql-insert.sql-delete, sql-merge).