Re: Adding OLD/NEW support to RETURNING

On Fri, Aug 2, 2024 at 6:13 PM Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
>
> On Fri, 2 Aug 2024 at 08:25, jian he <jian(dot)universality(at)gmail(dot)com> wrote:
> >
> > if (resultRelInfo->ri_projectReturning && (processReturning || saveOld))
> > {
> > }
> >
> > "saveOld" imply "resultRelInfo->ri_projectReturning"
> > we can simplified it as
> >
> > if (processReturning || saveOld))
> > {
> > }
> >
>
> No, because processReturning can be true when
> resultRelInfo->ri_projectReturning is NULL (no RETURNING list). So we
> do still need to check that resultRelInfo->ri_projectReturning is
> non-NULL.
>
> > for projectReturning->pi_state.flags,
> > we don't use EEO_FLAG_OLD_IS_NULL, EEO_FLAG_NEW_IS_NULL
> > in ExecProcessReturning, we can do the following way.
> >
> > /* Make old/new tuples available to ExecProject, if required */
> > if (oldSlot)
> > econtext->ecxt_oldtuple = oldSlot;
> > else if (projectReturning->pi_state.flags & EEO_FLAG_HAS_OLD)
> > econtext->ecxt_oldtuple = ExecGetAllNullSlot(estate, resultRelInfo);
> > else
> > econtext->ecxt_oldtuple = NULL; /* No references to OLD columns */
> >
> > if (newSlot)
> > econtext->ecxt_newtuple = newSlot;
> > else if (projectReturning->pi_state.flags & EEO_FLAG_HAS_NEW)
> > econtext->ecxt_newtuple = ExecGetAllNullSlot(estate, resultRelInfo);
> > else
> > econtext->ecxt_newtuple = NULL; /* No references to NEW columns */
> >
> > /*
> > * Tell ExecProject whether or not the OLD/NEW rows exist (needed for any
> > * ReturningExpr nodes).
> > */
> > 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;
> >
>
> I'm not sure I understand your point. It's true that
> EEO_FLAG_OLD_IS_NULL and EEO_FLAG_NEW_IS_NULL aren't used directly in
> ExecProcessReturning(), but they are used in stuff called from
> ExecProject().
>
> If the point was just to swap those 2 code blocks round, then OK, I
> guess maybe it reads a little better that way round, though it doesn't
> really make any difference either way.

sorry for confusion. I mean "swap those 2 code blocks round".
I think it will make it more readable, because you first check
projectReturning->pi_state.flags
with EEO_FLAG_HAS_NEW, EEO_FLAG_HAS_OLD
then change it.

> I did notice that that comment should mention that ExecEvalSysVar()
> also uses these flags, so I've updated it to do so.
>
> > @@ -2620,6 +2620,13 @@ transformWholeRowRef(ParseState *pstate,
> > * point, there seems no harm in expanding it now rather than during
> > * planning.
> > *
> > + * Note that if the nsitem is an OLD/NEW alias for the target RTE (as can
> > + * appear in a RETURNING list), its alias won't match the target RTE's
> > + * alias, but we still want to make a whole-row Var here rather than a
> > + * RowExpr, for consistency with direct references to the target RTE, and
> > + * so that any dropped columns are handled correctly. Thus we also check
> > + * p_returning_type here.
> > + *
> > makeWholeRowVar and subroutines only related to pg_type, but dropped
> > column info is in pg_attribute.
> > I don't understand "so that any dropped columns are handled correctly".
> >
>
> The nsitem contains references to dropped columns, so if you expanded
> it as a RowExpr, you'd end up with mismatched columns and it would
> fail (somewhere under ParseFuncOrColumn(), from transformColumnRef(),
> I think). There's a regression test case in returning.sql that covers
> that.
play around with it, get it.

if (nsitem->p_names == nsitem->p_rte->eref ||
nsitem->p_returning_type != VAR_RETURNING_DEFAULT)
else
{
expandRTE(nsitem->p_rte, nsitem->p_rtindex, sublevels_up,
nsitem->p_returning_type, location, false, NULL, &fields);
}
The ELSE branch expandRTE include_dropped argument is false.
that makes the ELSE branch unable to deal with dropped columns.

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)

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

overall, looks good to me.