On Mon, Oct 4, 2010 at 2:05 AM, Pavel Stehule <pavel.steh...@gmail.com> wrote: > 2010/10/4 Robert Haas <robertmh...@gmail.com>: >> On Oct 3, 2010, at 7:02 PM, Tom Lane <t...@sss.pgh.pa.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
