Tom Lane wrote:
> I've applied version 0.58 of the patch with a lot of further
> editorializing.  I feel fairly confident now in the code that interfaces
> between tsearch and the rest of the system, but a lot of the
> lowest-level "guts" of tsearch (mainly in src/backend/tsearch/*.c and
> src/backend/utils/adt/ts*.c) left my eyes glazing over.  Perhaps someone
> else can make an extra review pass over that stuff.

I'm skimming through tsearch code, trying to understand it. I'd like to
see more comments, at least function-comments explaining what each
function does, what the arguments are etc. I can try to add them as I go
as well, and send a patch.

The memory management of the init functions looks weird. In spell.c,
there's this piece of code:

> /*
>  * during initialization dictionary requires a lot
>  * of memory, so it will use temporary context
>  */
> static MemoryContext tmpCtx = NULL;
> #define tmpalloc(sz)  MemoryContextAlloc(tmpCtx, (sz))
> #define tmpalloc0(sz)  MemoryContextAllocZero(tmpCtx, (sz))
> static void
> checkTmpCtx(void)
> {
>       if (CurrentMemoryContext->firstchild == NULL)
>       {
>               tmpCtx = AllocSetContextCreate(CurrentMemoryContext,
> "Ispell dictionary init context",
>       }
>       else
>               tmpCtx = CurrentMemoryContext->firstchild;
> }

checkTmpCtx is called by all the initialization functions in spell.c. I
believe it's assumed that if firstchild exists, it's a previously
allocated "init context". But there isn't actually any guarantee that
the CurrentMemoryContext doesn't already have a child context, in which
case we would use whatever the first child context happens to be. And at
least dispell_init calls
MemoryContextDeleteChildren(CurrentMemoryContext), again with no
guarantee that there isn't other unrelated child contexts. I think
dispell_init should create the new context before calling the functions
in spell.c, and destroy it at the end. I can submit a patch, unless I'm
missing something.

More comments as I get further...

PS. Nice to see tsearch in core!

  Heikki Linnakangas

---------------------------(end of broadcast)---------------------------
TIP 4: Have you searched our list archives?


Reply via email to