I'm still not real happy about Hiroshi's proposed reimplementation of
copyObject. It would make copyObject much slower and more memory-
hungry, which I think is a bad thing -- especially since we may use
copyObject more than now if we redesign memory allocation as was
discussed a few weeks ago.
I looked at the specific problem he's reporting (coredump involving
a SELECT inside a plpgsql function), and I believe I understand it;
it's a pretty straightforward order-of-operations bug in copyfuncs.c.
Specifically, CopyPlanFields() tries to construct the new plan node's
list of subplans, but you can't do that until the node-type-specific
fields have all been set, since they may contain references to subplans.
As it stands, a copied plan will have a subPlan list that is missing
some of its subplans.
A simple localized fix would be to rearrange operations inside
copyfuncs.c so that the SS_pull_subplan operation is not done until
the end of the node-type-specific routines for copying plan nodes.
More generally, however, I think that the very existence of the subPlan
list is a bug. It violates the notion that "pointer equality shouldn't
matter" because the subPlan list *must* contain pointers to the actual
member objects of a plan node's other substructure.
Moreover the subPlan list isn't even buying us much --- it wouldn't be
that expensive to scan the substructure directly for plan-type nodes
when necessary, exactly as SS_pull_subplan does. Arguably this would
take less time than is expended on memory allocation to build the
explicit subPlan list. So I suggest getting rid of the subPlan list
entirely, rather than hacking on copyfuncs.
Plans also have an "initPlan" list that seems to have the same kind of
problem. I don't really understand the difference between initPlans
and subPlans --- plannodes.h says List *initPlan; /* Init Plan nodes (un-correlated expr
* subselects) */ List *subPlan; /* Other SubPlan nodes */
which doesn't convey a whole lot to my mind. Does anyone understand
why this distinction is made? Is it really necessary?
regards, tom lane
