Horton

Re: generic plans and "initial" pruning

Robert Haas <robertmhaas@gmail.com>

View thread View on postgr.es
From: Robert Haas <robertmhaas@gmail.com>
To: Amit Langote <amitlangote09@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: 2024-08-14T19:23:05Z
Lists: pgsql-hackers 
On Mon, Aug 12, 2024 at 8:54 AM Amit Langote <amitlangote09@gmail.com> wrote:
> 1. I went through many iterations of the changes to ExecInitNode() to
> return a partially initialized PlanState tree when it detects that the
> CachedPlan was invalidated after locking a child table and to
> ExecEndNode() to account for the PlanState tree sometimes being
> partially initialized, but it still seems fragile and bug-prone to me.
> It might be because this approach is fundamentally hard to get right
> or I haven't invested enough effort in becoming more confident in its
> robustness.

Can you give some examples of what's going wrong, or what you think
might go wrong?

I didn't think there was a huge problem here based on previous
discussion, but I could very well be missing some important challenge.

> 2. Refactoring needed due to the ExecutorStart() API change especially
> that pertaining to portals does not seem airtight.  I'm especially
> worried about moving the ExecutorStart() call for the
> PORTAL_MULTI_QUERY case from where it is currently to PortalStart().
> That requires additional bookkeeping in PortalData and I am not
> totally sure that the snapshot handling changes after that move are
> entirely correct.

Here again, it would help to see exactly what you had to do and what
consequences you think it might have. But it sounds like you're
talking about moving ExecutorStart() from PortalStart() to PortalRun()
and I agree that sounds like it might have user-visible behavioral
consequences that we don't want.

> 3. The need to add *back* the fields to store the RT indexes of
> relations that are not looked at by ExecInitNode() traversal such as
> root partitioned tables and non-leaf partitions.

I don't remember exactly why we removed those or what the benefit was,
so I'm not sure how big of a problem it is if we have to put them
back.

> About #1, I tend to agree with David that adding complexity around
> PlanState tree construction may not be a good idea, because we might
> want to rethink Plan initialization code and data structures in the
> not too distant future.

Like Tom, I don't really buy this. There might be a good reason not to
do this in ExecutorStart(), but the hypothetical possibility that we
might want to change something and that this patch might make it
harder is not it.

-- 
Robert Haas
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.