Re: generic plans and "initial" pruning

Robert,

On Fri, Oct 11, 2024 at 5:15 AM Robert Haas <robertmhaas@gmail.com> wrote:
>
> Hi Amit,
>
> This is not a full review (sorry!) but here are a few comments.

Thank you for taking a look.

> In general, I don't have a problem with this direction. I thought
> Tom's previous proposal of abandoning ExecInitNode() in medias res if
> we discover that we need to replan was doable and I still think that,
> but ISTM that this approach needs to touch less code, because
> abandoning ExecInitNode() partly through means we could have leftover
> state to clean up in any node in the PlanState tree, and as we've
> discussed, ExecEndNode() isn't necessarily prepared to clean up a
> PlanState tree that was only partially processed by ExecInitNode().

I will say that I feel more comfortable committing and be responsible
for the refactoring I'm proposing in 0001-0003 than the changes
required to take locks during ExecInitNode(), as seen in the patches
up to version v52..

> As
> far as I can see in the time I've spent looking at this today, 0001
> looks pretty unobjectionable (with some exceptions that I've noted
> below). I also think 0003 looks pretty safe. It seems like partition
> pruning moves backward across a pretty modest amount of code that does
> pretty well-defined things. Basically, initialization-time pruning now
> happens before other types of node initialization, and before setting
> up row marks. I do however find the changes in 0002 to be less
> obviously correct and less obviously safe; see below for some notes
> about that.
>
> In 0001, the name root_parent_relids doesn't seem very clear to me,
> and neither does the explanation of what it does. You say
> "'root_parent_relids' identifies the relation to which both the parent
> plan and the PartitionPruneInfo given by 'part_prune_index' belong."
> But it's a set, so what does it mean to identify "the" relation? It's
> a set of relations, not just one.

The intention is to ensure that the bitmapset in PartitionPruneInfo
corresponds to the apprelids bitmapset in the Append or MergeAppend
node that owns the PartitionPruneInfo. Essentially, root_parent_relids
is used to cross-check that both sets align, ensuring that the pruning
logic applies to the same relations as the parent plan.

> And why does the name include the
> word "root"? It's neither the PlannerGlobal object, which we often
> call root, nor is it the root of the partitioning hierarchy. To me, it
> looks like it's just the set of relids that we can potentially prune.
> I don't see why this isn't just called "relids", like the field from
> which it's copied:
>
> +       pruneinfo->root_parent_relids = parentrel->relids;
>
> It just doesn't seem very root-y or very parent-y.

Maybe just "relids" suffices with a comment updated like this:

 * relids               RelOptInfo.relids of the parent plan node (e.g. Append
 *                      or MergeAppend) to which his PartitionPruneInfo node
 *                      belongs. Used to ensure that the pruning logic matches
 *                      the parent plan's apprelids.

> -       node->part_prune_info = partpruneinfo;
> +
>
> Extra blank line.

Fixed.

> In 0002, the handling of ExprContexts seems a little bit hard to
> understand. Sometimes we're using the PlanState's ExprContext, and
> sometimes we're using a separate context owned by the
> PartitionedRelPruningData's context, and it's not exactly clear why
> that is or what the consequences are. Likewise I wouldn't mind some
> more comments or explanation in the commit message of the changes in
> this patch related to EState objects. I can't help wondering if the
> changes here could have either semantic implications (like expression
> evaluation can produce different results than before) or performance
> implications (because we create objects that we didn't previously
> create).

I have taken another look at whether there's any real need to use
separate ExprContexts for initial and runtime pruning and ISTM there
isn't, so we can make "exec" pruning use the same ExprContext as what
"init" would have used.  There *is* a difference however in how we
initializing the partition key expressions for initial and runtime
pruning, but it's not problematic to use the same ExprContext.

I'll update the commentary a bit more.

> Typo: partrtitioned

Fixed.

> Regrettably, I have not looked seriously at 0004 and 0005, so I can't
> comment on those.

Ok, I'm updating 0005 to change how the CachedPlan is handled when it
becomes invalid during InitPlan(). Currently (v56), a separate
transient CachedPlan is created for the query being initialized when
invalidation occurs. However, it seems better to update the original
CachedPlan in place to avoid extra bookkeeping for transient plans—an
approach Robert suggested in an off-list discussion.

Will post a new version next week.

-- 
Thanks, Amit Langote