Re: Adding OLD/NEW support to RETURNING

hi.

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.

/*
* 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?

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"),

+ /*
+ * Scan RETURNING WITH(...) options for OLD/NEW alias names. Complain if
+ * there is any conflict with existing relations.
+ */
+ foreach_node(ReturningOption, option, returningClause->options)
+ {
+ if (refnameNamespaceItem(pstate, NULL, option->name, -1, NULL))
+ ereport(ERROR,
+ errcode(ERRCODE_DUPLICATE_ALIAS),
+ errmsg("table name \"%s\" specified more than once",
+ option->name),
+ parser_errposition(pstate, option->location));
+
+ if (option->isNew)
+ {
+ if (qry->returningNew != NULL)
+ ereport(ERROR,
+ errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("NEW cannot be specified multiple times"),
+ parser_errposition(pstate, option->location));
+ qry->returningNew = option->name;
+ }
+ else
+ {
+ if (qry->returningOld != NULL)
+ ereport(ERROR,
+ errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("OLD cannot be specified multiple times"),
+ parser_errposition(pstate, option->location));
+ qry->returningOld = option->name;
+ }
+ }
+
+ /*
+ * If no OLD/NEW aliases specified, use "old"/"new" unless masked by
+ * existing relations.
+ */
+ if (qry->returningOld == NULL &&
+ refnameNamespaceItem(pstate, NULL, "old", -1, NULL) == NULL)
+ qry->returningOld = "old";
+ if (qry->returningNew == NULL &&
+ refnameNamespaceItem(pstate, NULL, "new", -1, NULL) == NULL)
+ qry->returningNew = "new";
+
+ /*
+ * Add the OLD and NEW aliases to the query namespace, for use in
+ * expressions in the RETURNING list.
+ */
+ save_nslen = list_length(pstate->p_namespace);
+ if (qry->returningOld)
+ addNSItemForReturning(pstate, qry->returningOld, VAR_RETURNING_OLD);
+ if (qry->returningNew)
+ addNSItemForReturning(pstate, qry->returningNew, VAR_RETURNING_NEW);

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?