On Thu, Aug 07, 2014 at 09:39:57AM -0400, Robert Haas wrote:
> On Wed, Aug 6, 2014 at 9:35 PM, Bruce Momjian <bruce@momjian.us> wrote:
> > On Sun, Jan 12, 2014 at 12:53:40PM -0500, Noah Misch wrote:
> >> Further study revealed a defect in the patch's memory management, and I have
> >> not gotten around to correcting that.
> >
> > I talked to Noah and he can't continue on this item. Can someone else
> > work on it?
>
> Well, it would be helpful if he could describe the defect he found, so
> that the next person doesn't have to guess.
Quite right. Primarily, a leak: FreeTupleDesc() did a mere pfree() on a node
tree, leaking all but the root node. Perhaps the best fix there is to give
each TupleDesc a memory context and then simplify FreeTupleDesc() to a context
deletion. That will tend to waste more memory, but I haven't thought of
something clearly better.
The patch passes a relcache-owned node tree to eval_const_expressions() via
ExecPrepareExpr(). If that stands, I should add a comment to the effect that
eval_const_expressions() must not modify in place the original tree or
reference it from the result tree. The comment at expression_planner()
implies that's already true. I know of no exceptions, but I have not audited.
How reasonable is reliance on continued conformance to that rule? An
alternative is to have the relcache always return a node tree copy, like it
does in RelationGetIndexExpressions(). That sacrifices about 1/4 of the
performance gain.
--
Noah Misch
EnterpriseDB http://www.enterprisedb.com