Neil Conway <nconway@klamath.dyndns.org> writes:
> Thanks Tom. However, after applying the changes noted above, the test
> case still fails (and I'm still scratching my head about what's
> causing the problem). A new version of the patch is attached.
I got curious, so I tried out this version of the patch. Some debugging
demonstrated that the value being pointed to by the default expression's
Const node was getting freed :-(, evidently by the loop at copy.c lines
1014-1018 (as patched). Said loop is really pretty bogus anyway, since
it effectively assumes that every pass-by-ref datatype's input function
always returns a freshly-palloc'd value. (This isn't the only place
that assumes that, if memory serves; but over time I'd like to eliminate
such assumptions.) In the presence of values generated by means other
than datatype input functions, the assumption breaks down completely.
What I would like to do about this is to eliminate the explicit pfree's
entirely; they are both dangerous, as shown by this example, and
inadequate to guarantee no memory leak. (For example, any storage
leaked internally by a datatype conversion function cannot get reclaimed
by this approach.) A much more bulletproof approach in both COPY IN and
COPY OUT would be to run the per-tuple loop in a memory context that we
reset at the top of the loop. Then any palloc'd leaked storage will be
reclaimed at the end of the tuple cycle. COPY IN may as well use the
per-tuple exprcontext's memory context, which we're already using and
resetting. COPY OUT doesn't seem to need an exprcontext at the moment,
but it might be best to use parallel coding anyway.
Brent Verner hasn't submitted any version of his patch yet, but he did
pop up a day or so ago to say that he still intended to do so. Maybe
you should offer to help him whip it into shape ...
regards, tom lane
