Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Sep 7, 2010 at 1:30 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> In the particular case here, the dictionary structures could probably
>> safely use such a context type, but I'm not sure it's worth bothering
>> if the long-term plan is to implement a precompiler. �There would be
>> no need for this after the precompiled representation is installed,
>> because that'd just be one big hunk of memory anyway.
> Rather than inventing something more complex, I'm inclined to say we
> should just go ahead and apply this more or less as Pavel wrote it. I
> haven't tried to reproduce Pavel's results, but I assume that they are
> accurate and that's a pretty big savings for a pretty trivial amount
> of code. If it gets thrown away later when/if someone codes up a
> precompiler, no harm done.
Actually, my fear about it is that it will get in the way of whoever
tries to implement a precompiler, because it confuses the memory
management quite a lot. It's not at all apparent that the code is even
safe as-is, because it's depending on the unstated assumption that that
static variable will get reset once per dictionary. The documentation
is horrible: it doesn't really explain what the patch is doing, and what
it does say is wrong. It's not the case that that memory will never be
freed, so it's critical that the allocs happen in the dictCtx of the
dictionary currently being built, and not get piggybacked onto the
memory allocated for some previous dictionary. I *think* that it's not
actively broken as-is, but it's sure fragile and badly documented.
(Of course, the same charge could be leveled against the tmpCtx hack
that's already there; but that seems like a poor excuse for making
things even messier.)
regards, tom lane