On Thu, Sep 28, 2023 at 5:26 PM Amit Langote <amitlangote09@gmail.com> wrote:
> On Tue, Sep 26, 2023 at 10:06 PM Amit Langote <amitlangote09@gmail.com> wrote:
> > 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.
Rebased.
I haven't been able to reproduce and debug a crash reported by cfbot
that I see every now and then:
https://cirrus-ci.com/task/5673432591892480?logs=cores#L0
[22:46:12.328] Program terminated with signal SIGSEGV, Segmentation fault.
[22:46:12.328] Address not mapped to object.
[22:46:12.838] #0 afterTriggerInvokeEvents
(events=events@entry=0x836db0460, firing_id=1,
estate=estate@entry=0x842eec100, delete_ok=<optimized out>) at
../src/backend/commands/trigger.c:4656
[22:46:12.838] #1 0x00000000006c67a8 in AfterTriggerEndQuery
(estate=estate@entry=0x842eec100) at
../src/backend/commands/trigger.c:5085
[22:46:12.838] #2 0x000000000065bfba in CopyFrom (cstate=0x836df9038)
at ../src/backend/commands/copyfrom.c:1293
...
While a patch in this series does change
src/backend/commands/trigger.c, I'm not yet sure about its relation
with the backtrace shown there.
--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com
