Re: tsearch2 patch status report

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",
> ALLOCSET_DEFAULT_MINSIZE,
> ALLOCSET_DEFAULT_INITSIZE,
> ALLOCSET_DEFAULT_MAXSIZE);
> }
> 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
EnterpriseDB http://www.enterprisedb.com