Re: [PATCHES] GiST header cleanup

[Neil Conway]

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
---(end of broadcast)---
TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]

Re: [PATCHES] GiST header cleanup

[Tom Lane]

Neil Conway <[EMAIL PROTECTED]> 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

---(end of broadcast)---
TIP 3: if posting/reading through Usenet, please send an appropriate
  subscribe-nomail command to [EMAIL PROTECTED] so that your
  message can get through to the mailing list cleanly

[PATCHES] GiST header cleanup

[Neil Conway]

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  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
Index: contrib/cube/cube.c
===
RCS file: /var/lib/cvs/pgsql/contrib/cube/cube.c,v
retrieving revision 1.18
diff -c -r1.18 cube.c
*** contrib/cube/cube.c	21 Oct 2004 19:28:33 -	1.18
--- contrib/cube/cube.c	17 May 2005 01:54:22 -
***
*** 6,11 
--- 6,12 
  
  #include "postgres.h"
  
+ #include 
  #include 
  
  #include "access/gist.h"
Index: src/backend/access/gist/gist.c
===
RCS file: /var/lib/cvs/pgsql/src/backend/access/gist/gist.c,v
retrieving revision 1.116
diff -c -r1.116 gist.c
*** src/backend/access/gist/gist.c	17 May 2005 00:59:30 -	1.116
--- src/backend/access/gist/gist.c	17 May 2005 01:49:26 -
***
*** 15,21 
  #include "postgres.h"
  
  #include "access/genam.h"
! #include "access/gist.h"
  #include "access/gistscan.h"
  #include "access/heapam.h"
  #include "catalog/index.h"
--- 15,21 
  #include "postgres.h"
  
  #include "access/genam.h"
! #include "access/gist_private.h"
  #include "access/gistscan.h"
  #include "access/heapam.h"
  #include "catalog/index.h"
***
*** 26,35 
  
  #undef GIST_PAGEADDITEM
  
! #define ATTSIZE(datum, TupDesc, i, isnull) \
  	( \
  		(isnull) ? 0 : \
! 			att_addlength(0, (TupDesc)->attrs[(i)-1]->attlen, (datum)) \
  	)
  
  /* result's status */
--- 26,35 
  
  #undef GIST_PAGEADDITEM
  
! #define ATTSIZE(datum, tupdesc, i, isnull) \
  	( \
  		(isnull) ? 0 : \
! 			att_addlength(0, (tupdesc)->attrs[(i)-1]->attlen, (datum)) \
  	)
  
  /* result's status */
Index: src/backend/access/gist/gistget.c
===
RCS file: /var/lib/cvs/pgsql/src/backend/access/gist/gistget.c,v
retrieving revision 1.46
diff -c -r1.46 gistget.c
*** src/backend/access/gist/gistget.c	17 May 2005 00:59:30 -	1.46
--- src/backend/access/gist/gistget.c	17 May 2005 01:28:51 -
***
*** 14,20 
   */
  #include "postgres.h"
  
! #include "access/gist.h"
  #include "executor/execdebug.h"
  #include "utils/memutils.h"
  
--- 14,21 
   */
  #include "postgres.h"
  
! #include "access/gist_private.h"
! #include "access/itup.h"
  #include "executor/execdebug.h"
  #include "utils/memutils.h"
  
Index: src/backend/access/gist/gistscan.c
===
RCS file: /var/lib/cvs/pgsql/src/backend/access/gist/gistscan.c,v
retrieving revision 1.57
diff -c -r1.57 gistscan.c
*** src/backend/access/gist/gistscan.c	17 May 2005 00:59:30 -	1.57
--- src/backend/access/gist/gistscan.c	17 May 2005 01:16:46 -
***
*** 15,21 
  #include "postgres.h"
  
  #include "access/genam.h"
! #include "access/gist.h"
  #include "access/gistscan.h"
  #include "utils/memutils.h"
  #include "utils/resowner.h"
--- 15,21 
  #include "postgres.h"
  
  #include "access/genam.h"
! #include "access/gist_private.h"
  #include "access/gistscan.h"
  #include "utils/memutils.h"
  #include "utils/resowner.h"
Index: src/backend/access/transam/rmgr.c
===
RCS file: /var/lib/cvs/pgsql/src/backend/access/transam/rmgr.c,v
retrieving revision 1.16
diff -c -r1.16 rmgr.c
*** src/backend/access/transam/rmgr.c	29 Aug 2004 21:08:47 -	1.16
--- src/backend/access/transam/rmgr.c	17 May 2005 01:19:48 -
***
*** 8,14 
  #include "postgres.h"
  
  #include "access/clog.h"
! #include "access/gist.h"
  #include "access/hash.h"
  #include "access/heapam.h"
  #include "access/nbtree.h"
--- 8,14 
  #include "postgres.h"
  
  #include "access/clog.h"
! #include "access/gist_private.h"
  #include "access/hash.h"
  #include "access/heapam.h"
  #include "access/nbtree.h"
Index: src/include/access/gist.h
==