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

On Thu, Aug 1, 2024 at 7:33 PM Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>
> On Mon, 29 Jul 2024 at 11:22, Dean Rasheed <dean.a.rasheed@gmail.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);"