Re: Adding OLD/NEW support to RETURNING

On Mon, 5 Aug 2024 at 12:46, jian he <jian(dot)universality(at)gmail(dot)com> wrote:
>
> took me a while to understand the changes in rewriteHandler.c, rewriteManip.c
> rule over updateable view still works, but I didn't check closely with
> rewriteRuleAction.
> i think I understand rewriteTargetView and subroutines.
>
> * 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.
>
> "the index of the target relation for an INSERT/UPDATE/DELETE/MERGE",
> here, "target relation" I think people may be confused whether it
> refers to view relation or the base relation.
> I think here the target relation is the base relation (rtekind == RTE_RELATION)

Yes, it's the result relation in the rewritten query. I've updated
that comment to try to make that clearer.

Basically, if a replacement Var refers to the new result relation in
the rewritten query, then its varreturningtype needs to be set
correctly. Otherwise, if it refers to some other relation, its
varreturningtype shouldn't be changed, but it does need to be wrapped
in a ReturningExpr node, if the original Var had a non-default
varreturningtype, so that it evaluates as NULL if the old/new row
doesn't exist.

> " to force it to be NULL if the OLD/NEW row doesn't exist."
> i think this happen in execExpr.c?
> maybe
> " to force it to be NULL if the OLD/NEW row doesn't exist, see execExpr.c"

OK, I've updated it to just say that this causes the executor to
return NULL if the old/new row doesn't exist. There are multiple
places in the executor that actually make that happen, so it doesn't
make sense to just refer to one place.

> overall, looks good to me.

Thanks for reviewing.

I'm pretty happy with the patch now, but I was just thinking about the
wholerow case a little more, and I think it's worth changing the way
that's handled.

Previously, if you wrote something like "RETURNING old", and the old
row didn't exist, it would return an all-NULL record (displayed as
something like '(,,,,)'), but I don't think that's really right. I
think it should actually return NULL. I think that's more consistent
with the way "non-existent" is generally handled, for example in a
query like "SELECT t1, t2 FROM t1 OUTER JOIN t2 ON ...".

It's pretty trivial, but it does involve changing code in 2 places
(the first for regular tables, and the second for updatable views):

1. ExecEvalWholeRowVar() now checks EEO_FLAG_OLD_IS_NULL and
EEO_FLAG_NEW_IS_NULL. This makes it more consistent with
ExecEvalSysVar(), so I added the same Asserts.

2. ReplaceVarsFromTargetList() now wraps the RowExpr node created in
the wholerow case in a ReturningExpr. That's consistent with the
function's comment: "if the replacement expression from the targetlist
is not simply a Var referencing result_relation, it is wrapped in a
ReturningExpr node".

Both those changes seem quite natural and consistent, and I think the
resulting test output looks much nicer.

Regards,
Dean