GiST header cleanup
This patch moves GiST implementation details from gist.h into a new
header file, gist_private.h. gist.h should only contain APIs that are
exposed to clients writing GiST extensions -- where possible we should
avoid backward-incompatible changes to those APIs, so it makes sense to
keep that API in a separate file.
Other related changes:
- pruned down the list of unnecessary includes in gist.h; as a result I
had to add a missing #include <float.h> to contrib/cube/cube.c
- remove isAttByVal(), which is no longer used
- remove declaration of _gistdump(), which is never defined
I noticed that GISTNStrategies is defined, but never used; instead there
is a literal "100" in include/catalog/pg_am.h. Does anyone see a reason
to keep GISTNStrategies around? Alternatively, should pg_am.h include
gist.h and reference GISTNStrategies instead of using "100"?
All of contrib/ continues to compile without warnings with this patch; I
haven't tried externally maintained GiST extensions, but they may need a
bit of #include tweaking.
Barring any objections I'll apply this later today or tomorrow.
-Neil
Attachments:
gist_include_reorg-1.patchtext/x-patch; name=gist_include_reorg-1.patchDownload+168-153
Neil Conway <neilc@samurai.com> writes:
This patch moves GiST implementation details from gist.h into a new
header file, gist_private.h.
Sounds reasonable. The other index AMs could possibly benefit from the
same thing --- in particular I believe nbtree.h is included by quite a
lot of places that really only need to know btree strategy and support
procedure numbers.
One objection: I think the GiST amproc numbers (GIST_CONSISTENT_PROC
and friends) *are* part of the API and should be in the public header,
even if they happen not to be used by any C code at the moment. They
are certainly well known at the SQL level, and the btree precedent
suggests people will want them at the C level too.
I noticed that GISTNStrategies is defined, but never used; instead there
is a literal "100" in include/catalog/pg_am.h. Does anyone see a reason
to keep GISTNStrategies around? Alternatively, should pg_am.h include
gist.h and reference GISTNStrategies instead of using "100"?
GISTNStrategies seems inherently bogus, since there's no essential limit
on the number of strategies in a gist index. I'd get rid of it.
The "100" in pg_am.h is pretty nasty too, because it is on the one hand
theoretically insufficient and on the other hand in practice way too
much. (This at least leads to wasted space in relcache entries for gist
indexes, and probably defeats error checks to some extent as well.)
It might be interesting someday to think about a way to let this number
be specified per-opclass instead of per-AM, for those AMs where such a
thing makes sense (only GiST at the moment). I'm not terribly excited
about it though... for now I'm willing to live with "100". Anyone who
needs more can poke their local copy of pg_am.
regards, tom lane
Patch applied.
Tom Lane wrote:
One objection: I think the GiST amproc numbers (GIST_CONSISTENT_PROC
and friends) *are* part of the API and should be in the public header,
even if they happen not to be used by any C code at the moment.
Ok, I've moved these back to gist.h
GISTNStrategies seems inherently bogus, since there's no essential limit
on the number of strategies in a gist index. I'd get rid of it.
Done.
The "100" in pg_am.h is pretty nasty too, because it is on the one hand
theoretically insufficient and on the other hand in practice way too
much.
Yeah, I agree this is pretty ugly, but I'm not planning to fix it any
time soon, either...
-Neil