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?