Tom,
* Stephen Frost (sfrost@snowman.net) wrote:
> Second, there are a couple of bugs (or at least, I'll characterize
> them that way) where we're pfree'ing a list which has been passed to
> list_concat(). That's not strictly legal as either argument passed to
> list_concat() could be returned and so one might end up free'ing the
> list pointer that was just returned. Those pfree's are commented out
> in this patch, but really should probably just be removed or replaced
> with 'right' code (if it's critical to free this stuff).
A couple of these pfree's were added to simplify_or_arguments() in
commit 56c88772911b4e4c8fbb86d8687d95c3acd38a4f and the other was added
to transformFromClauseItem() in a4996a895399a4b0363c7dace71fc6ce8acbc196.
The two cases in clauses.c are pretty specific and documented:
List *subargs = list_copy(((BoolExpr *) arg)->args);
/* overly tense code to avoid leaking unused list header */ if (!unprocessed_args) unprocessed_args =
subargs; else { List *oldhdr = unprocessed_args;
unprocessed_args = list_concat(subargs, unprocessed_args); pfree(oldhdr); }
which really makes me wonder if it's a pretty important cleanup due to
how this call can end up getting nested pretty deeply and the number of
potential loops. On the surface, it looks like this whole chunk could
be reduced to a single list_concat() call. With the new list
implementation, we'll probably want to review this area and make sure
that we don't end up leaking anything through the list_copy/list_concat
usage pattern above. Other thoughts?
In transformFromClauseItem(), the pfree is:
pfree(r_relnamespace); /* free unneeded list header */
which appears to be more general-purpose cleanup that could be safely
removed.
Thanks,
Stephen