Re: patch: tsearch - some memory diet

On Mon, Oct 4, 2010 at 2:05 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
> 2010/10/4 Robert Haas <robertmhaas(at)gmail(dot)com>:
>> On Oct 3, 2010, at 7:02 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> 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.
>>
>> Yep. We certainly would need to convince ourselves that this is correct before applying it, and that is all kinds of non-obvious.
>>
>
> what is good documentation?
>
> This patch doesn't do more, than it removes palloc overhead on just
> one structure of TSearch2 ispell dictionary. It isn't related to some
> static variable - the most important is fact so this memory is
> unallocated by dropping of memory context.

After looking at this a bit more, I don't think it's too hard to fix
up the comments so that they reflect what's actually going on here.
For this patch to be correct, the only thing we really need to believe
is that no one is going to try to pfree an SPNode, which seems like
something we ought to be able to convince ourselves of. I don't see
how the fact that some of the memory may get allocated out of a
palloc'd chunk from context X rather than from context X directly can
really cause any problems otherwise. The existing code already
depends on the unstated assumption that the static variable will get
reset once per dictionary; we're not making that any worse.

I think it would be cleaner to get rid of checkTmpCtx() and instead
have dispell_init() set up and tear down the temporary context,
leaving NULL behind in the global variable after it's torn down,
perhaps by having spell.c publish an API like this:

void NISetupForDictionaryLoad();
void NICleanupAfterDictionaryLoad();

...but I don't really see why that has to be done as part of this patch.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company