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 ---------------------------(end of broadcast)--------------------------- TIP 4: Have you searched our list archives? http://archives.postgresql.org