Re: generic plans and "initial" pruning - Mailing list pgsql-hackers

Hi Amit,

This is not a full review (sorry!) but here are a few comments.

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(). 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. 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.

-       node->part_prune_info = partpruneinfo;
+

Extra blank line.

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). As noted above, this is really my only design-level concern
about 0001-0003.

Typo: partrtitioned

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

--
Robert Haas
EDB: http://www.enterprisedb.com