Re: Adding OLD/NEW support to RETURNING

On Mon, 25 Mar 2024 at 00:00, jian he <jian(dot)universality(at)gmail(dot)com> wrote:
>
> hi, some minor issues I found out.
>
> the ReplaceReturningVarsFromTargetList related comment
> should be placed right above the function ReplaceReturningVarsFromTargetList,
> not above ReplaceReturningVarsFromTargetList_context?

Hmm, well there are a mix of possible styles for this kind of
function. Sometimes the outer function comes first, immediately after
the function comment, and then the callback function comes after that.
That has the advantage that all documentation comments related to the
top-level input arguments are next to the function that takes them.
Also, this ordering means that you naturally read it in the order in
which it is initially executed.

The other style, putting the callback function first has the advantage
that you can more immediately see what the function does, since it's
usually the callback that contains the interesting logic.

rewriteManip.c has examples of both styles, but in this case, since
ReplaceReturningVarsFromTargetList() is similar to
ReplaceVarsFromTargetList(), I opted to copy its style.

> struct ReplaceReturningVarsFromTargetList_context adds some comments
> about new_result_relation would be great.

I substantially rewrote that function in the v6 patch. As part of
that, I renamed "new_result_relation" to "new_target_varno", which
more closely matches the existing "target_varno" argument, and I added
comments about what it's for to the top-level function comment block.

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

No, I think it's fine as it is. Its current placement is where you
might otherwise expect to find a "case INDEX_VAR:" block of code, and
it's explaining why there isn't one there, and where to look instead.

Moving it into the switch default case would lose that effect, and I
think it would reduce the code's readability.

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

In the v6 patch, I moved the code change from
set_deparse_context_plan() down into set_deparse_plan(), because I
thought that would catch more cases, but thinking about it some more,
that wasn't necessary, since it won't change when moving up and down
the ancestor tree. So in v7, I've moved it back and updated the
comment.

> + * 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"?

Yes, well spotted. Fixed in v7.

> [in expandRTE()]
>
> - * 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.

In the v6 patch, I already added code to ensure that it's set in all
cases, though I don't think it's strictly necessary. returning_type
can only have a non-default value for the target RTE, which can't
currently be any of those other RTE kinds, but nonetheless it seemed
better from a consistency point-of-view, and to make it more
future-proof.

v7 patch attached, with those updates.

Regards,
Dean