GiST: memory allocation, cleanup

From: Neil Conway <neilc(at)samurai(dot)com>
To: pgsql-patches <pgsql-patches(at)postgresql(dot)org>
Cc: Oleg Bartunov <oleg(at)sai(dot)msu(dot)su>
Subject: GiST: memory allocation, cleanup
Date: 2004-11-05 05:41:00
Message-ID: 1099633260.10449.100.camel@localhost.localdomain
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-patches

Attached is a patch that makes some cleanups and improvements to GiST,
as well as a few semi-related cleanups. Changes:

- previously, GiST did not make any use of the backend's memory context
infrastructure. This made implementing indexes using GiST unnecessarily
difficult and fragile: if your implementation of a GiST method leaked
memory, this could result in a potentially large memory leak in
PostgreSQL itself. This patch changes GiST so that all invocations of
used-defined GiST methods are done within a short-lived memory context.
In practice, this means GiST implementors need not use pfree, unless
they are doing a _lot_ of allocations in their method implementations.

As part of this work, memory contexts were also used to cleanup a lot of
ugly and difficult to maintain memory management code in gist.c itself.

QUESTION: given a ScanKey for an index scan, GiST overwrites the
ScanKey's sk_func to point to the user-defined Consistent method
(gistrescan() in gistscan.c). When doing a GiST index scan we can ensure
that the sk_func is invoked in a short-lived memory context
(gistfindnext() in gistget.c). Is it safe to assume that anywhere else
in the backend that invokes the ScanKey's sk_func, it is done in a
short-lived memory context?

- previously, src/include/access/gist.h included both the "public" API
that is intended for use by GiST implementors as well as declarations
for the GiST implementation itself. I've created a new header file
"gist_private.h" and moved the latter declarations there.

- remove some "extern" keywords from function definitions in
contrib/btree_gist, and do some more minor code cleanup there

- minor GiST documentation cleanup

- mark the array that indicates NULLs that is passed to
index_formtuple() as 'const', and fix the resulting fallout

- after doing an index build, there was previously a long comment and a
few lines of code that used the # of index tuples and heap tuples
(computed by the index build) to update the optimizer's statistics. This
code was duplicated 4 times (once for each type of index); I added a
function called IndexUpdateStats() in catalog/index.c and changed each
index's build function to call that function.

- in GiST, move the definition of a bunch of scope-local variables to
the most tightly-enclosing scope. In other words, code like:

{
int a, b;

a = ...;
if (a)
{
b = ...;
}
}

was changed to move the definition of "b" into the inner scope. This is
per good style, IMHO (and it is consistent with the rest of the
PostgreSQL source).

- moved gistfreestack() to gistscan.c and made it "static", since it is
only used there

- cleaned up various other things in gist.c: added some comments,
basically tried to impose some sanity

Comments welcome. I would like to apply this to 8.1 at some point after
we branch for 8.0. I thought I would post this patch to get some
feedback before starting on further GiST work (speculatively, adding
support for concurrency and making it WAL-safe).

-Neil

Attachment Content-Type Size
gist_work-1.patch.gz application/x-gzip 17.1 KB

Responses

Browse pgsql-patches by date

  From Date Subject
Next Message Oleg Bartunov 2004-11-05 09:38:09 Re: contrib/ sparse code cleanup
Previous Message Neil Conway 2004-11-05 04:21:01 contrib/ sparse code cleanup