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-26T13:06:12Z
Lists: pgsql-hackers

Attachments

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

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.