On 26 October 2010 03:42, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Brendan Jurd <direvus@gmail.com> writes:
>> Thanks for the hint; I found that the attached patch resolved my
>> specific segfault, but I am wondering whether it goes far enough.
>
> Well, it definitely doesn't go far enough, because the invalItems list
> has to be restored to its original state if we fail to inline at some
> point after calling eval_const_expressions.
Point taken.
> I thought about whether we need something more general, but for the
> moment I think this is sufficient; eval_const_expressions has only
> very limited reason to examine the PlannerInfo data at all, and less
> reason to modify it. Copying the whole structure would be overkill.
> Moreover, it wouldn't do anything to improve modularity anyhow: this
> function would still have to know which parts of the structure to copy
> back to the top level, and which not. So it'd still need to know
> quite a bit about exactly what eval_const_expressions is doing.
That makes sense to me.
This whole business of passing around global pointers while switching
memory contexts seems like an optimal breeding-ground for bugs. It
would be nice to come up with a more ... well, "global" way to manage
PlannerGlobal. To me it suggests something along the lines of a
dedicated MemoryContext in which PlannerGlobal and all its members
live, and you operate on PlannerGlobal by calling methods, rather than
by directly twiddling its pointers.
But, that is probably way over the top for this, given its narrow area
of effect.
Cheers,
BJ