Re: [PATCHES] relscan.h split
Alvaro Herrera <[EMAIL PROTECTED]> writes: > Tom Lane wrote: >> Also, it seemed like some of those .c files had no business poking into >> the scan structs anyway; particularly contrib. Did you check whether >> the inclusions could be avoided? > Not really, unless we were to provide something a routine that returns > the current block of a scan. Current buffer you mean. That wouldn't be a bad idea --- the wart I added to genam.c the other day to recheck the current tuple could be replaced with that, I think, though it wouldn't really be much less warty. BTW I think your change in pgstattuple.c is probably dangerous: if the relation gets extended between where heap_beginscan_strat sets rs_nblocks and where you put RelationGetNumberOfBlocks, I think the block counting will get messed up. That one really does need access to the internals. Other than that, this looks pretty sane to me. regards, tom lane -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] relscan.h split
Tom Lane wrote: > Perhaps a better idea would be to put the opaque-pointer typedefs into > heapam.h and genam.h respectively, and then see where you could remove > inclusions of relscan.h. Hmm, this seems to be closely equivalent. Patch attached. I also moved SysScanDescData from genam.h to relscan.h. > Also, it seemed like some of those .c files had no business poking into > the scan structs anyway; particularly contrib. Did you check whether > the inclusions could be avoided? Not really, unless we were to provide something a routine that returns the current block of a scan. There are a few occurrences of this: /* must hold a buffer lock to call HeapTupleSatisfiesUpdate */ LockBuffer(scan->rs_cbuf, BUFFER_LOCK_SHARE); which of course need the definition. Maybe providing it is not a bad idea, because that kind of coding is used in the backend too. -- Alvaro Herrerahttp://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support Index: contrib/pgrowlocks/pgrowlocks.c === RCS file: /home/alvherre/cvs/pgsql/contrib/pgrowlocks/pgrowlocks.c,v retrieving revision 1.10 diff -c -p -r1.10 pgrowlocks.c *** contrib/pgrowlocks/pgrowlocks.c 12 May 2008 00:00:43 - 1.10 --- contrib/pgrowlocks/pgrowlocks.c 14 Jun 2008 23:16:20 - *** *** 26,31 --- 26,32 #include "access/heapam.h" #include "access/multixact.h" + #include "access/relscan.h" #include "access/xact.h" #include "catalog/namespace.h" #include "funcapi.h" Index: contrib/pgstattuple/pgstattuple.c === RCS file: /home/alvherre/cvs/pgsql/contrib/pgstattuple/pgstattuple.c,v retrieving revision 1.35 diff -c -p -r1.35 pgstattuple.c *** contrib/pgstattuple/pgstattuple.c 16 May 2008 17:31:17 - 1.35 --- contrib/pgstattuple/pgstattuple.c 14 Jun 2008 23:20:41 - *** *** 29,34 --- 29,35 #include "access/heapam.h" #include "access/htup.h" #include "access/nbtree.h" + #include "access/relscan.h" #include "catalog/namespace.h" #include "funcapi.h" #include "miscadmin.h" *** pgstat_heap(Relation rel, FunctionCallIn *** 262,268 /* Disable syncscan because we assume we scan from block zero upwards */ scan = heap_beginscan_strat(rel, SnapshotAny, 0, NULL, true, false); ! nblocks = scan->rs_nblocks; /* # blocks to be scanned */ /* scan the relation */ while ((tuple = heap_getnext(scan, ForwardScanDirection)) != NULL) --- 263,269 /* Disable syncscan because we assume we scan from block zero upwards */ scan = heap_beginscan_strat(rel, SnapshotAny, 0, NULL, true, false); ! nblocks = RelationGetNumberOfBlocks(rel); /* # blocks to be scanned */ /* scan the relation */ while ((tuple = heap_getnext(scan, ForwardScanDirection)) != NULL) Index: src/backend/access/gin/ginget.c === RCS file: /home/alvherre/cvs/pgsql/src/backend/access/gin/ginget.c,v retrieving revision 1.16 diff -c -p -r1.16 ginget.c *** src/backend/access/gin/ginget.c 16 May 2008 16:31:01 - 1.16 --- src/backend/access/gin/ginget.c 14 Jun 2008 23:05:11 - *** *** 15,20 --- 15,21 #include "postgres.h" #include "access/gin.h" + #include "access/relscan.h" #include "catalog/index.h" #include "miscadmin.h" #include "storage/bufmgr.h" Index: src/backend/access/gin/ginscan.c === RCS file: /home/alvherre/cvs/pgsql/src/backend/access/gin/ginscan.c,v retrieving revision 1.14 diff -c -p -r1.14 ginscan.c *** src/backend/access/gin/ginscan.c 16 May 2008 16:31:01 - 1.14 --- src/backend/access/gin/ginscan.c 14 Jun 2008 23:05:01 - *** *** 14,21 #include "postgres.h" - #include "access/genam.h" #include "access/gin.h" #include "pgstat.h" #include "storage/bufmgr.h" #include "utils/memutils.h" --- 14,21 #include "postgres.h" #include "access/gin.h" + #include "access/relscan.h" #include "pgstat.h" #include "storage/bufmgr.h" #include "utils/memutils.h" Index: src/backend/access/gist/gistget.c === RCS file: /home/alvherre/cvs/pgsql/src/backend/access/gist/gistget.c,v retrieving revision 1.73 diff -c -p -r1.73 gistget.c *** src/backend/access/gist/gistget.c 12 May 2008 00:00:44 - 1.73 --- src/backend/access/gist/gistget.c 14 Jun 2008 22:53:35 - *** *** 15,20 --- 15,21 #include "postgres.h" #include "access/gist_private.h" + #include "access/relscan.h" #include "executor/execdebug.h" #include "miscadmin.h" #include "pgstat.h" Index: src/backend/access/gist/gistscan.c === RCS file: /home/alvher
Re: [PATCHES] relscan.h split
Alvaro Herrera <[EMAIL PROTECTED]> writes: > I propose the following patch which moves the struct definitions to a > separate new header relscan_internal.h. This seems a little bizarre, seeing that there is almost nothing in relscan.h except those structs. Perhaps a better idea would be to put the opaque-pointer typedefs into heapam.h and genam.h respectively, and then see where you could remove inclusions of relscan.h. Also, it seemed like some of those .c files had no business poking into the scan structs anyway; particularly contrib. Did you check whether the inclusions could be avoided? regards, tom lane -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
[PATCHES] relscan.h split
Hi, relscan.h is very widely used -- in particular it is included by some headers that want the IndexScanDesc and HeapScanDesc definitions in prototypes. However, most of the time they are just passing the struct through; they don't need to see the actual Heap/IndexScanDescData definitions. I propose the following patch which moves the struct definitions to a separate new header relscan_internal.h. Files that actually need the definitions can include the new header. They are not that many -- I count 22 inclusions, all of them in .c files. Headers only include the .h file, which has the benefit that since it is a lean file, it needn't include all the other headers needed by the struct declaration. Zdenek says that this patch changes the number of times certain headers are opened (data gathered using dtrace): new old diff src/include/access/skey.h 347 465 -118 src/include/utils/rel.h 851 921 -70 src/include/access/relscan.h340 360 -20 So it doesn't have a tremendous impact, but it does have some. -- Alvaro Herrerahttp://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support Index: contrib/pgrowlocks/pgrowlocks.c === RCS file: /home/alvherre/Code/cvs/pgsql/contrib/pgrowlocks/pgrowlocks.c,v retrieving revision 1.10 diff -c -p -r1.10 pgrowlocks.c *** contrib/pgrowlocks/pgrowlocks.c 12 May 2008 00:00:43 - 1.10 --- contrib/pgrowlocks/pgrowlocks.c 9 Jun 2008 01:49:31 - *** *** 26,31 --- 26,32 #include "access/heapam.h" #include "access/multixact.h" + #include "access/relscan_internal.h" #include "access/xact.h" #include "catalog/namespace.h" #include "funcapi.h" Index: contrib/pgstattuple/pgstattuple.c === RCS file: /home/alvherre/Code/cvs/pgsql/contrib/pgstattuple/pgstattuple.c,v retrieving revision 1.35 diff -c -p -r1.35 pgstattuple.c *** contrib/pgstattuple/pgstattuple.c 16 May 2008 17:31:17 - 1.35 --- contrib/pgstattuple/pgstattuple.c 9 Jun 2008 01:49:44 - *** *** 29,34 --- 29,35 #include "access/heapam.h" #include "access/htup.h" #include "access/nbtree.h" + #include "access/relscan_internal.h" #include "catalog/namespace.h" #include "funcapi.h" #include "miscadmin.h" Index: src/backend/access/gin/ginget.c === RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/access/gin/ginget.c,v retrieving revision 1.16 diff -c -p -r1.16 ginget.c *** src/backend/access/gin/ginget.c 16 May 2008 16:31:01 - 1.16 --- src/backend/access/gin/ginget.c 8 Jun 2008 23:58:24 - *** *** 15,20 --- 15,21 #include "postgres.h" #include "access/gin.h" + #include "access/relscan_internal.h" #include "catalog/index.h" #include "miscadmin.h" #include "storage/bufmgr.h" Index: src/backend/access/gin/ginscan.c === RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/access/gin/ginscan.c,v retrieving revision 1.14 diff -c -p -r1.14 ginscan.c *** src/backend/access/gin/ginscan.c 16 May 2008 16:31:01 - 1.14 --- src/backend/access/gin/ginscan.c 8 Jun 2008 23:59:31 - *** *** 16,21 --- 16,22 #include "access/genam.h" #include "access/gin.h" + #include "access/relscan_internal.h" #include "pgstat.h" #include "storage/bufmgr.h" #include "utils/memutils.h" Index: src/backend/access/gist/gistget.c === RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/access/gist/gistget.c,v retrieving revision 1.73 diff -c -p -r1.73 gistget.c *** src/backend/access/gist/gistget.c 12 May 2008 00:00:44 - 1.73 --- src/backend/access/gist/gistget.c 8 Jun 2008 23:55:58 - *** *** 15,20 --- 15,21 #include "postgres.h" #include "access/gist_private.h" + #include "access/relscan_internal.h" #include "executor/execdebug.h" #include "miscadmin.h" #include "pgstat.h" Index: src/backend/access/gist/gistscan.c === RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/access/gist/gistscan.c,v retrieving revision 1.69 diff -c -p -r1.69 gistscan.c *** src/backend/access/gist/gistscan.c 12 May 2008 00:00:44 - 1.69 --- src/backend/access/gist/gistscan.c 8 Jun 2008 23:56:23 - *** *** 17,22 --- 17,23 #include "access/genam.h" #include "access/gist_private.