On Mon, Sep 25, 2023 at 9:57 PM Amit Langote <amitlangote09@gmail.com> wrote:
> On Wed, Sep 6, 2023 at 11:20 PM Robert Haas <robertmhaas@gmail.com> wrote:
> > - Is there any point to all of these early exit cases? For example, in
> > ExecInitBitmapAnd, why exit early if initialization fails? Why not
> > just plunge ahead and if initialization failed the caller will notice
> > that and when we ExecEndNode some of the child node pointers will be
> > NULL but who cares? The obvious disadvantage of this approach is that
> > we're doing a bunch of unnecessary initialization, but we're also
> > speeding up the common case where we don't need to abort by avoiding a
> > branch that will rarely be taken. I'm not quite sure what the right
> > thing to do is here.
> I thought about this some and figured that adding the
> is-CachedPlan-still-valid tests in the following places should suffice
> after all:
>
> 1. In InitPlan() right after the top-level ExecInitNode() calls
> 2. In ExecInit*() functions of Scan nodes, right after
> ExecOpenScanRelation() calls
After sleeping on this, I think we do need the checks after all the
ExecInitNode() calls too, because we have many instances of the code
like the following one:
outerPlanState(gatherstate) = ExecInitNode(outerNode, estate, eflags);
tupDesc = ExecGetResultType(outerPlanState(gatherstate));
<some code that dereferences outDesc>
If outerNode is a SeqScan and ExecInitSeqScan() returned early because
ExecOpenScanRelation() detected that plan was invalidated, then
tupDesc would be NULL in this case, causing the code to crash.
Now one might say that perhaps we should only add the
is-CachedPlan-valid test in the instances where there is an actual
risk of such misbehavior, but that could lead to confusion, now or
later. It seems better to add them after every ExecInitNode() call
while we're inventing the notion, because doing so relieves the
authors of future enhancements of the ExecInit*() routines from
worrying about any of this.
Attached 0003 should show how that turned out.
Updated 0002 as mentioned in the previous reply -- setting pointers to
NULL after freeing them more consistently across various ExecEnd*()
routines and using the `if (pointer != NULL)` style over the `if
(pointer)` more consistently.
Updated 0001's commit message to remove the mention of its relation to
any future commits. I intend to push it tomorrow.
Patches 0004 onwards contain changes too, mainly in terms of moving
the code around from one patch to another, but I'll omit the details
of the specific change for now.
--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com
