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-09-28T08:26:27Z
Lists: pgsql-hackers

Attachments

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

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.