On Tue, Sep 26, 2023 at 10:06 PM Amit Langote <amitlangote09@gmail.com> wrote:
> 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.
Pushed that one. Here are the rebased patches.
0001 seems ready to me, but I'll wait a couple more days for others to
weigh in. Just to highlight a kind of change that others may have
differing opinions on, consider this hunk from the patch:
- MemoryContextDelete(node->aggcontext);
+ if (node->aggcontext != NULL)
+ {
+ MemoryContextDelete(node->aggcontext);
+ node->aggcontext = NULL;
+ }
...
+ ExecEndNode(outerPlanState(node));
+ outerPlanState(node) = NULL;
So the patch wants to enhance the consistency of setting the pointer
to NULL after freeing part. Robert mentioned his preference for doing
it in the patch, which I agree with.
--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com