Horton

Re: generic plans and "initial" pruning

Amit Langote <amitlangote09@gmail.com>

View thread View on postgr.es
From: Amit Langote <amitlangote09@gmail.com>
To: Robert Haas <robertmhaas@gmail.com>
Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>, Andres Freund <andres@anarazel.de>, Daniel Gustafsson <daniel@yesql.se>, David Rowley <dgrowleyml@gmail.com>, Jacob Champion <jchampion@timescale.com>, PostgreSQL Hackers <pgsql-hackers@postgresql.org>, Thom Brown <thom@linux.com>, Tom Lane <tgl@sss.pgh.pa.us>
Date: 2023-11-20T04:29:53Z
Lists: pgsql-hackers

Attachments

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

Commits

  1. Stamp 19beta1.

  2. Revert "Don't lock partitions pruned by initial pruning"

  3. Ensure first ModifyTable rel initialized if all are pruned

  4. Fix bug in cbc127917 to handle nested Append correctly

  5. Remove unstable test suite added by 525392d57

  6. Don't lock partitions pruned by initial pruning

  7. Fix an oversight in cbc127917 to handle MERGE correctly

  8. Track unpruned relids to avoid processing pruned relations

  9. Perform runtime initial pruning outside ExecInitNode()

  10. Move PartitionPruneInfo out of plan nodes into PlannedStmt

  11. Fix setrefs.c's failure to do expression processing on prune steps.

  12. Remove obsolete executor cleanup code

  13. Revert "Move PartitionPruneInfo out of plan nodes into PlannedStmt"

  14. Move PartitioPruneInfo out of plan nodes into PlannedStmt

  15. Refactor and cleanup runtime partition prune code a little

  16. Remove some unnecessary fields from Plan trees.

  17. Remove more redundant relation locking during executor startup.

  18. Shut down Gather's children before shutting down Gather itself.