Re: generic plans and "initial" pruning - Mailing list pgsql-hackers
From | Robert Haas |
---|---|
Subject | Re: generic plans and "initial" pruning |
Date | |
Msg-id | CA+TgmobO_6irkJGkzkxHTR=kza_CG+0idAhFUWqGfXCVQQmuPg@mail.gmail.com Whole thread Raw |
In response to | Re: generic plans and "initial" pruning (Amit Langote <amitlangote09@gmail.com>) |
Responses |
Re: generic plans and "initial" pruning
|
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
pgsql-hackers by date: