Re: Adding OLD/NEW support to RETURNING

On Fri, 19 Jul 2024 at 01:11, jian he <jian.universality@gmail.com> wrote:
>
> hi.
> I have some minor questions, but overall it just works.

Thanks for the review!

> in ExecEvalSysVar, we can add Asserts
> Assert(state->flags & EEO_FLAG_HAS_OLD || state->flags & EEO_FLAG_HAS_NEW);
> if I understand it correctly.

OK. I think it's probably worth coding defensively here, so I have
added more specific Asserts, based on the actual varreturningtype (and
I didn't really like that old "if" condition anyway, so I've rewritten
it as a switch).

> 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)
> ```

Good point. I agree, it's worth ensuring that we don't call
contain_vars_returning_old_or_new() multiple times (or at all, if we
don't need to).

> + * 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.
> + *
> 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.

No, I think it's slightly more accurate to say that the old row
doesn't exist for INSERT and the new row doesn't exist for DELETE. The
end result is that all the values will be NULL, so in that sense it's
the same as the old/new row being NULL, or being an all-NULL tuple.

> 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).

OK, I have rewritten and expanded upon that a bit to try to make it
clearer. I also decided that this discussion really belongs in the
output_expression description, rather than under output_alias.

Thanks again for the review. Updated patch attached.

Regards,
Dean