Re: Adding OLD/NEW support to RETURNING

On Thu, Aug 1, 2024 at 7:33 PM Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
>
> On Mon, 29 Jul 2024 at 11:22, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
> >
> > Trivial rebase, following c7301c3b6f.
> >
>
> Rebased version, forced by a7f107df2b. Evaluating the input parameters
> of correlated SubPlans in the referencing ExprState simplifies this
> patch in a couple of places, since it no longer has to worry about
> copying ExprState flags to a new ExprState.
>

hi. some minor issues.

saveOld = changingPart && resultRelInfo->ri_projectReturning &&
resultRelInfo->ri_projectReturning->pi_state.flags & EEO_FLAG_HAS_OLD;
if (resultRelInfo->ri_projectReturning && (processReturning || saveOld))
{
}

"saveOld" imply "resultRelInfo->ri_projectReturning"
we can simplified it as

if (processReturning || saveOld))
{
}

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;

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

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