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
