Re: Adding OLD/NEW support to RETURNING

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.

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.

> ExecEvalSysVar, slot_getsysattr we have "Assert(attnum < 0);"
> but
> ExecEvalSysVar, while rowIsNull is true, we didn't do "Assert(attnum < 0);"

I don't see much value in that, since we aren't going to evaluate the
attribute if the old/new row is null.

Regards,
Dean