Re: ModifyTable overheads in generic plans

On Mon, Nov 2, 2020 at 10:19 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> On 30/10/2020 08:13, Amit Langote wrote:
> > /*
> >  * Perform WITH CHECK OPTIONS check, if any.
> >  */
> > static void
> > ExecProcessWithCheckOptions(ModifyTableState *mtstate, ResultRelInfo *resultRelInfo,
> >                                                       TupleTableSlot *slot, WCOKind wco_kind)
> > {
> >       ModifyTable *node = (ModifyTable *) mtstate->ps.plan;
> >       EState *estate = mtstate->ps.state;
> >
> >       if (node->withCheckOptionLists == NIL)
> >               return;
> >
> >       /* Initialize expression state if not already done. */
> >       if (resultRelInfo->ri_WithCheckOptions == NIL)
> >       {
> >               int             whichrel = resultRelInfo - mtstate->resultRelInfo;
> >               List   *wcoList;
> >               List   *wcoExprs = NIL;
> >               ListCell   *ll;
> >
> >               Assert(whichrel >= 0 && whichrel < mtstate->mt_nplans);
> >               wcoList = (List *) list_nth(node->withCheckOptionLists, whichrel);
> >               foreach(ll, wcoList)
> >               {
> >                       WithCheckOption *wco = (WithCheckOption *) lfirst(ll);
> >                       ExprState  *wcoExpr = ExecInitQual((List *) wco->qual,
> >                                                                                          &mtstate->ps);
> >
> >                       wcoExprs = lappend(wcoExprs, wcoExpr);
> >               }
> >
> >               resultRelInfo->ri_WithCheckOptions = wcoList;
> >               resultRelInfo->ri_WithCheckOptionExprs = wcoExprs;
> >       }
> >
> >       /*
> >        * ExecWithCheckOptions() will skip any WCOs which are not of the kind
> >        * we are looking for at this point.
> >        */
> >       ExecWithCheckOptions(wco_kind, resultRelInfo, slot, estate);
> > }
>
> Can we do this initialization in ExecGetResultRelation()? That would
> seem much more straightforward. Is there any advantage to delaying it
> here? And same thing with the junk filter and the RETURNING list.
>
> (/me reads patch further) I presume that's what you referred to in the
> commit message:
>
> >     Also, extend this lazy initialization approach to some of the
> >     individual fields of ResultRelInfo such that even for the result
> >     relations that are initialized, those fields are only initialized on
> >     first access.  While no performance improvement is to be expected
> >     there, it can lead to a simpler initialization logic of the
> >     ResultRelInfo itself, because the conditions for whether a given
> >     field is needed or not tends to look confusing.  One side-effect
> >     of this is that any "SubPlans" referenced in the expressions of
> >     those fields are also lazily initialized and hence changes the
> >     output of EXPLAIN (without ANALYZE) in some regression tests.
>
>
> I'm now curious what the initialization logic would look like, if we
> initialized those fields in ExecGetResultRelation(). At a quick glance
> on the conditions on when those initializations are done in the patch
> now, it would seem pretty straightforward. If the target list contains
> any junk columns, initialize junk filter, and if
> ModifyTable->returningLists is set, initialize RETURNING list. Maybe I'm
> missing something.

Yeah, it's not that complicated to initialize those things in
ExecGetResultRelation().  In fact, ExecGetResultRelation() (or its
subroutine ExecBuildResultRelation()) housed those initializations in
the earlier versions of this patch, but I changed that after our
discussion about being lazy about initializing as much stuff as we
can.  Maybe I should revert that?

-- 
Amit Langote
EDB: http://www.enterprisedb.com