Re: Adding OLD/NEW support to RETURNING

On Mon, 12 Aug 2024 at 07:51, jian he <jian(dot)universality(at)gmail(dot)com> wrote:
>
> took me a while to understand how the returning clause Var nodes
> correctly reference the relations RT index.
> mainly in set_plan_references, set_plan_refs and
> set_returning_clause_references.
>
> do you think we need do something in
> set_returning_clause_references->build_tlist_index_other_vars
> to make sure that
> if the topplan->targetlist associated Var's varreturningtype is not default,
> then the var->varno must equal to resultRelation.
> because set_plan_references is almost at the end of standard_planner,
> before that things may change.

Hmm, well actually the check has to go in fix_join_expr_mutator().
It's really a "shouldn't happen" error, a little similar to other
checks in setrefs.c that elog errors, so I guess it's probably worth
double-checking this too.

> /*
> * Tell ExecProject whether or not the OLD/NEW rows exist (needed for any
> * ReturningExpr nodes and ExecEvalSysVar).
> */
> if (oldSlot == NULL)
> projectReturning->pi_state.flags |= EEO_FLAG_OLD_IS_NULL;
> else
> projectReturning->pi_state.flags &= ~EEO_FLAG_OLD_IS_NULL;
> if (newSlot == NULL)
> projectReturning->pi_state.flags |= EEO_FLAG_NEW_IS_NULL;
> else
> projectReturning->pi_state.flags &= ~EEO_FLAG_NEW_IS_NULL;
>
> ExecEvalWholeRowVar also uses this information, comment needs to be
> slightly adjusted?

Ah, yes.

> simialr to
> https://git.postgresql.org/cgit/postgresql.git/commit/?id=2bb969f3998489e5dc4fe9f2a61185b43581975d
> do you think it's necessary to
> errmsg("%s cannot be specified multiple times", "NEW"),
> errmsg("%s cannot be specified multiple times", "OLD"),

OK, I guess so.

> + /*
> + * Add the OLD and NEW aliases to the query namespace, for use in
> + * expressions in the RETURNING list.
> + */
>
> the only case we don't do addNSItemForReturning is when there is
> really a RTE called "new" or "old".
> Even if the returning list doesn't specify "new" or "old", like
> "returning 1", we still do addNSItemForReturning.
> Do you think it's necessary in ReturningClause add two booleans
> "hasold", "hasnew".
> so if becomes
> + if (qry->returningOld && hasold)
> + addNSItemForReturning(pstate, qry->returningOld, VAR_RETURNING_OLD);
> + if (qry->returningNew && hasnew)
> + addNSItemForReturning(pstate, qry->returningNew, VAR_RETURNING_NEW);
>
> that means in gram.y
> returning_clause:
> RETURNING returning_with_clause target_list
> {
> ReturningClause *n = makeNode(ReturningClause);
>
> n->options = $2;
> n->exprs = $3;
> $$ = n;
> }
>
> n->exprs will have 3 branches: NEW.expr, OLD.expr, expr.
> I guess overall we can save some cycles?

No, I think that would add a whole lot of unnecessary extra
complication, because n->exprs can contain any arbitrary expressions,
including subqueries, which would make it very hard for gram.y to tell
if there really was a Var referencing old/new at a particular query
level, and it would probably end up adding more cycles than it saved
later on, as well as being quite error-prone.

Regards,
Dean