Re: Adding OLD/NEW support to RETURNING - Mailing list pgsql-hackers

On Fri, Aug 2, 2024 at 6:13 PM Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>
> On Fri, 2 Aug 2024 at 08:25, jian he <jian.universality@gmail.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.