Re: Partitioning performance: cache stringToNode() of pg_constraint.ccbin

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