relscan_details.h

Started by Alvaro Herreraover 12 years ago12 messages

alvherre@2ndquadrant.com

over 12 years ago

1 attachment(s)

relscan.h is a bit of an unfortunate header because it requires a lot of
other headers to compile, and is also required by execnodes.h. This
quick patch removes the struct definitions from that file, moving them
into a new relscan_details.h file; the reworked relscan.h does not need
any other header, freeing execnodes.h from needlessly including lots of
unnecessary stuff all over the place. Only the files that really need
the struct definition include the new file, and as seen in the patch,
they are quite few.

execnodes.h is included by 147 backend source files; relscan_details.h
only 27. So the exposure area is reduced considerably. This patch also
modifies a few other backend source files to add one or both of genam.h
and heapam.h, which were previously included through execnodes.h.

This is probably missing tweaks for contrib/. The following modules
would need to include relscan_details.h: sepgsql dblink pgstattuple
pgrowlocks. I expect the effect on third-party modules to be much less
than by the htup_details.h change.

--
ï¿½lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Attachments:

relscan-details.patchtext/x-diff; charset=us-asciiDownload

diff --git a/src/backend/access/gin/ginget.c b/src/backend/access/gin/ginget.c
index cb17d38..1dfa888 100644
--- a/src/backend/access/gin/ginget.c
+++ b/src/backend/access/gin/ginget.c
@@ -15,7 +15,7 @@
 #include "postgres.h"
 
 #include "access/gin_private.h"
-#include "access/relscan.h"
+#include "access/relscan_details.h"
 #include "miscadmin.h"
 #include "utils/datum.h"
 #include "utils/memutils.h"
diff --git a/src/backend/access/gin/ginscan.c b/src/backend/access/gin/ginscan.c
index afee2db..d22724f 100644
--- a/src/backend/access/gin/ginscan.c
+++ b/src/backend/access/gin/ginscan.c
@@ -15,7 +15,7 @@
 #include "postgres.h"
 
 #include "access/gin_private.h"
-#include "access/relscan.h"
+#include "access/relscan_details.h"
 #include "pgstat.h"
 #include "utils/memutils.h"
 #include "utils/rel.h"
diff --git a/src/backend/access/gist/gistget.c b/src/backend/access/gist/gistget.c
index e97ab8f..c58152d 100644
--- a/src/backend/access/gist/gistget.c
+++ b/src/backend/access/gist/gistget.c
@@ -15,7 +15,7 @@
 #include "postgres.h"
 
 #include "access/gist_private.h"
-#include "access/relscan.h"
+#include "access/relscan_details.h"
 #include "miscadmin.h"
 #include "pgstat.h"
 #include "utils/builtins.h"
diff --git a/src/backend/access/gist/gistscan.c b/src/backend/access/gist/gistscan.c
index b5553ff..e7e6614 100644
--- a/src/backend/access/gist/gistscan.c
+++ b/src/backend/access/gist/gistscan.c
@@ -16,7 +16,7 @@
 
 #include "access/gist_private.h"
 #include "access/gistscan.h"
-#include "access/relscan.h"
+#include "access/relscan_details.h"
 #include "utils/memutils.h"
 #include "utils/rel.h"
 
diff --git a/src/backend/access/hash/hash.c b/src/backend/access/hash/hash.c
index 8895f58..3f52a61 100644
--- a/src/backend/access/hash/hash.c
+++ b/src/backend/access/hash/hash.c
@@ -19,7 +19,7 @@
 #include "postgres.h"
 
 #include "access/hash.h"
-#include "access/relscan.h"
+#include "access/relscan_details.h"
 #include "catalog/index.h"
 #include "commands/vacuum.h"
 #include "optimizer/cost.h"
diff --git a/src/backend/access/hash/hashscan.c b/src/backend/access/hash/hashscan.c
index 8c2918f..54e91f3 100644
--- a/src/backend/access/hash/hashscan.c
+++ b/src/backend/access/hash/hashscan.c
@@ -16,7 +16,7 @@
 #include "postgres.h"
 
 #include "access/hash.h"
-#include "access/relscan.h"
+#include "access/relscan_details.h"
 #include "utils/memutils.h"
 #include "utils/rel.h"
 #include "utils/resowner.h"
diff --git a/src/backend/access/hash/hashsearch.c b/src/backend/access/hash/hashsearch.c
index 91661ba..9ab44d0 100644
--- a/src/backend/access/hash/hashsearch.c
+++ b/src/backend/access/hash/hashsearch.c
@@ -15,7 +15,7 @@
 #include "postgres.h"
 
 #include "access/hash.h"
-#include "access/relscan.h"
+#include "access/relscan_details.h"
 #include "miscadmin.h"
 #include "pgstat.h"
 #include "utils/rel.h"
diff --git a/src/backend/access/hash/hashutil.c b/src/backend/access/hash/hashutil.c
index aa071d9..a7d5cca 100644
--- a/src/backend/access/hash/hashutil.c
+++ b/src/backend/access/hash/hashutil.c
@@ -16,7 +16,7 @@
 
 #include "access/hash.h"
 #include "access/reloptions.h"
-#include "access/relscan.h"
+#include "access/relscan_details.h"
 #include "utils/lsyscache.h"
 #include "utils/rel.h"
 
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index ead3d69..f599383 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -44,7 +44,7 @@
 #include "access/heapam_xlog.h"
 #include "access/hio.h"
 #include "access/multixact.h"
-#include "access/relscan.h"
+#include "access/relscan_details.h"
 #include "access/sysattr.h"
 #include "access/transam.h"
 #include "access/tuptoaster.h"
diff --git a/src/backend/access/index/genam.c b/src/backend/access/index/genam.c
index 15debed..cacd0b6 100644
--- a/src/backend/access/index/genam.c
+++ b/src/backend/access/index/genam.c
@@ -19,7 +19,7 @@
 
 #include "postgres.h"
 
-#include "access/relscan.h"
+#include "access/relscan_details.h"
 #include "access/transam.h"
 #include "catalog/index.h"
 #include "lib/stringinfo.h"
diff --git a/src/backend/access/index/indexam.c b/src/backend/access/index/indexam.c
index b878155..90f388d 100644
--- a/src/backend/access/index/indexam.c
+++ b/src/backend/access/index/indexam.c
@@ -65,7 +65,7 @@
 
 #include "postgres.h"
 
-#include "access/relscan.h"
+#include "access/relscan_details.h"
 #include "access/transam.h"
 #include "catalog/index.h"
 #include "pgstat.h"
diff --git a/src/backend/access/nbtree/nbtree.c b/src/backend/access/nbtree/nbtree.c
index 073190f..f69a8d1 100644
--- a/src/backend/access/nbtree/nbtree.c
+++ b/src/backend/access/nbtree/nbtree.c
@@ -20,7 +20,7 @@
 
 #include "access/heapam_xlog.h"
 #include "access/nbtree.h"
-#include "access/relscan.h"
+#include "access/relscan_details.h"
 #include "catalog/index.h"
 #include "commands/vacuum.h"
 #include "storage/indexfsm.h"
diff --git a/src/backend/access/nbtree/nbtsearch.c b/src/backend/access/nbtree/nbtsearch.c
index ac98589..9bb592c 100644
--- a/src/backend/access/nbtree/nbtsearch.c
+++ b/src/backend/access/nbtree/nbtsearch.c
@@ -16,7 +16,7 @@
 #include "postgres.h"
 
 #include "access/nbtree.h"
-#include "access/relscan.h"
+#include "access/relscan_details.h"
 #include "miscadmin.h"
 #include "pgstat.h"
 #include "storage/predicate.h"
diff --git a/src/backend/access/nbtree/nbtutils.c b/src/backend/access/nbtree/nbtutils.c
index 352c77c..fcf98e4 100644
--- a/src/backend/access/nbtree/nbtutils.c
+++ b/src/backend/access/nbtree/nbtutils.c
@@ -19,7 +19,7 @@
 
 #include "access/nbtree.h"
 #include "access/reloptions.h"
-#include "access/relscan.h"
+#include "access/relscan_details.h"
 #include "miscadmin.h"
 #include "utils/array.h"
 #include "utils/lsyscache.h"
diff --git a/src/backend/access/spgist/spgscan.c b/src/backend/access/spgist/spgscan.c
index c9d3cb6..c365b6e 100644
--- a/src/backend/access/spgist/spgscan.c
+++ b/src/backend/access/spgist/spgscan.c
@@ -15,7 +15,7 @@
 
 #include "postgres.h"
 
-#include "access/relscan.h"
+#include "access/relscan_details.h"
 #include "access/spgist_private.h"
 #include "miscadmin.h"
 #include "storage/bufmgr.h"
diff --git a/src/backend/bootstrap/bootstrap.c b/src/backend/bootstrap/bootstrap.c
index d23dc45..5f472b3 100644
--- a/src/backend/bootstrap/bootstrap.c
+++ b/src/backend/bootstrap/bootstrap.c
@@ -21,6 +21,8 @@
 #include <getopt.h>
 #endif
 
+#include "access/genam.h"
+#include "access/heapam.h"
 #include "access/htup_details.h"
 #include "bootstrap/bootstrap.h"
 #include "catalog/index.h"
diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c
index fe17c96..1abe4b6 100644
--- a/src/backend/catalog/dependency.c
+++ b/src/backend/catalog/dependency.c
@@ -14,7 +14,10 @@
  */
 #include "postgres.h"
 
+#include "access/genam.h"
+#include "access/heapam.h"
 #include "access/htup_details.h"
+#include "access/sysattr.h"
 #include "access/xact.h"
 #include "catalog/dependency.h"
 #include "catalog/heap.h"
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index 64ca312..bb4dd9b 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -29,6 +29,8 @@
  */
 #include "postgres.h"
 
+#include "access/genam.h"
+#include "access/heapam.h"
 #include "access/htup_details.h"
 #include "access/multixact.h"
 #include "access/sysattr.h"
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index b73ee4f..f91b57d 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -24,7 +24,7 @@
 #include <unistd.h>
 
 #include "access/multixact.h"
-#include "access/relscan.h"
+#include "access/relscan_details.h"
 #include "access/sysattr.h"
 #include "access/transam.h"
 #include "access/visibilitymap.h"
diff --git a/src/backend/catalog/indexing.c b/src/backend/catalog/indexing.c
index 880155e..328ecc6 100644
--- a/src/backend/catalog/indexing.c
+++ b/src/backend/catalog/indexing.c
@@ -15,6 +15,7 @@
  */
 #include "postgres.h"
 
+#include "access/genam.h"
 #include "access/htup_details.h"
 #include "catalog/index.h"
 #include "catalog/indexing.h"
diff --git a/src/backend/catalog/objectaddress.c b/src/backend/catalog/objectaddress.c
index 4d22f3a..02e288c 100644
--- a/src/backend/catalog/objectaddress.c
+++ b/src/backend/catalog/objectaddress.c
@@ -15,6 +15,8 @@
 
 #include "postgres.h"
 
+#include "access/genam.h"
+#include "access/heapam.h"
 #include "access/htup_details.h"
 #include "access/sysattr.h"
 #include "catalog/catalog.h"
diff --git a/src/backend/catalog/pg_proc.c b/src/backend/catalog/pg_proc.c
index 05a550e..1868b58 100644
--- a/src/backend/catalog/pg_proc.c
+++ b/src/backend/catalog/pg_proc.c
@@ -14,6 +14,7 @@
  */
 #include "postgres.h"
 
+#include "access/heapam.h"
 #include "access/htup_details.h"
 #include "access/xact.h"
 #include "catalog/dependency.h"
diff --git a/src/backend/catalog/toasting.c b/src/backend/catalog/toasting.c
index 385d64d..4962f60 100644
--- a/src/backend/catalog/toasting.c
+++ b/src/backend/catalog/toasting.c
@@ -14,6 +14,7 @@
  */
 #include "postgres.h"
 
+#include "access/heapam.h"
 #include "access/tuptoaster.h"
 #include "access/xact.h"
 #include "catalog/dependency.h"
diff --git a/src/backend/commands/alter.c b/src/backend/commands/alter.c
index b62ec70..817b87c 100644
--- a/src/backend/commands/alter.c
+++ b/src/backend/commands/alter.c
@@ -14,6 +14,7 @@
  */
 #include "postgres.h"
 
+#include "access/heapam.h"
 #include "access/htup_details.h"
 #include "access/sysattr.h"
 #include "catalog/dependency.h"
diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze.c
index 9845b0b..0c5a55d 100644
--- a/src/backend/commands/analyze.c
+++ b/src/backend/commands/analyze.c
@@ -16,6 +16,8 @@
 
 #include <math.h>
 
+#include "access/genam.h"
+#include "access/heapam.h"
 #include "access/multixact.h"
 #include "access/transam.h"
 #include "access/tupconvert.h"
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index f6a5bfe..e6f9347 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -18,7 +18,7 @@
 #include "postgres.h"
 
 #include "access/multixact.h"
-#include "access/relscan.h"
+#include "access/relscan_details.h"
 #include "access/rewriteheap.h"
 #include "access/transam.h"
 #include "access/tuptoaster.h"
diff --git a/src/backend/commands/constraint.c b/src/backend/commands/constraint.c
index f2cdc27..ded9067 100644
--- a/src/backend/commands/constraint.c
+++ b/src/backend/commands/constraint.c
@@ -13,6 +13,8 @@
  */
 #include "postgres.h"
 
+#include "access/genam.h"
+#include "access/heapam.h"
 #include "catalog/index.h"
 #include "commands/trigger.h"
 #include "executor/executor.h"
diff --git a/src/backend/commands/createas.c b/src/backend/commands/createas.c
index a3509d8..159ee8b 100644
--- a/src/backend/commands/createas.c
+++ b/src/backend/commands/createas.c
@@ -23,6 +23,7 @@
  */
 #include "postgres.h"
 
+#include "access/heapam.h"
 #include "access/reloptions.h"
 #include "access/htup_details.h"
 #include "access/sysattr.h"
diff --git a/src/backend/commands/event_trigger.c b/src/backend/commands/event_trigger.c
index 328e2a8..7ef7e4b 100644
--- a/src/backend/commands/event_trigger.c
+++ b/src/backend/commands/event_trigger.c
@@ -13,6 +13,7 @@
  */
 #include "postgres.h"
 
+#include "access/heapam.h"
 #include "access/htup_details.h"
 #include "access/xact.h"
 #include "catalog/dependency.h"
diff --git a/src/backend/commands/extension.c b/src/backend/commands/extension.c
index 798c92a..a611067 100644
--- a/src/backend/commands/extension.c
+++ b/src/backend/commands/extension.c
@@ -27,6 +27,8 @@
 #include <limits.h>
 #include <unistd.h>
 
+#include "access/genam.h"
+#include "access/heapam.h"
 #include "access/htup_details.h"
 #include "access/sysattr.h"
 #include "access/xact.h"
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 902daa0..a4d9f95 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -15,6 +15,8 @@
 
 #include "postgres.h"
 
+#include "access/genam.h"
+#include "access/heapam.h"
 #include "access/htup_details.h"
 #include "access/reloptions.h"
 #include "access/xact.h"
diff --git a/src/backend/commands/matview.c b/src/backend/commands/matview.c
index 238ccc7..82091e2 100644
--- a/src/backend/commands/matview.c
+++ b/src/backend/commands/matview.c
@@ -14,6 +14,8 @@
  */
 #include "postgres.h"
 
+#include "access/genam.h"
+#include "access/heapam.h"
 #include "access/htup_details.h"
 #include "access/multixact.h"
 #include "access/xact.h"
diff --git a/src/backend/commands/schemacmds.c b/src/backend/commands/schemacmds.c
index 1d13ba0..097b42c 100644
--- a/src/backend/commands/schemacmds.c
+++ b/src/backend/commands/schemacmds.c
@@ -14,8 +14,8 @@
  */
 #include "postgres.h"
 
-#include "access/htup_details.h"
 #include "access/heapam.h"
+#include "access/htup_details.h"
 #include "access/xact.h"
 #include "catalog/catalog.h"
 #include "catalog/dependency.h"
diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c
index ddfaf3b..87efea7 100644
--- a/src/backend/commands/sequence.c
+++ b/src/backend/commands/sequence.c
@@ -14,6 +14,7 @@
  */
 #include "postgres.h"
 
+#include "access/heapam.h"
 #include "access/htup_details.h"
 #include "access/multixact.h"
 #include "access/transam.h"
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index adc74dd..282c068 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -19,7 +19,7 @@
 #include "access/heapam_xlog.h"
 #include "access/multixact.h"
 #include "access/reloptions.h"
-#include "access/relscan.h"
+#include "access/relscan_details.h"
 #include "access/sysattr.h"
 #include "access/xact.h"
 #include "catalog/catalog.h"
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index 791f336..52d92b0 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -37,6 +37,7 @@
  */
 #include "postgres.h"
 
+#include "access/heapam.h"
 #include "access/htup_details.h"
 #include "access/sysattr.h"
 #include "access/transam.h"
diff --git a/src/backend/executor/execUtils.c b/src/backend/executor/execUtils.c
index 39e3b2e..2c939b4 100644
--- a/src/backend/executor/execUtils.c
+++ b/src/backend/executor/execUtils.c
@@ -42,7 +42,7 @@
 
 #include "postgres.h"
 
-#include "access/relscan.h"
+#include "access/relscan_details.h"
 #include "access/transam.h"
 #include "catalog/index.h"
 #include "executor/execdebug.h"
diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c
index 45292b2..cb2379b 100644
--- a/src/backend/executor/nodeBitmapHeapscan.c
+++ b/src/backend/executor/nodeBitmapHeapscan.c
@@ -35,7 +35,7 @@
  */
 #include "postgres.h"
 
-#include "access/relscan.h"
+#include "access/relscan_details.h"
 #include "access/transam.h"
 #include "executor/execdebug.h"
 #include "executor/nodeBitmapHeapscan.h"
diff --git a/src/backend/executor/nodeBitmapIndexscan.c b/src/backend/executor/nodeBitmapIndexscan.c
index d4c23a2..e25bd0d 100644
--- a/src/backend/executor/nodeBitmapIndexscan.c
+++ b/src/backend/executor/nodeBitmapIndexscan.c
@@ -21,6 +21,7 @@
  */
 #include "postgres.h"
 
+#include "access/genam.h"
 #include "executor/execdebug.h"
 #include "executor/nodeBitmapIndexscan.h"
 #include "executor/nodeIndexscan.h"
diff --git a/src/backend/executor/nodeIndexonlyscan.c b/src/backend/executor/nodeIndexonlyscan.c
index 2f30c55..f1711d0 100644
--- a/src/backend/executor/nodeIndexonlyscan.c
+++ b/src/backend/executor/nodeIndexonlyscan.c
@@ -24,7 +24,7 @@
  */
 #include "postgres.h"
 
-#include "access/relscan.h"
+#include "access/relscan_details.h"
 #include "access/visibilitymap.h"
 #include "executor/execdebug.h"
 #include "executor/nodeIndexonlyscan.h"
diff --git a/src/backend/executor/nodeIndexscan.c b/src/backend/executor/nodeIndexscan.c
index f1062f1..9a49395 100644
--- a/src/backend/executor/nodeIndexscan.c
+++ b/src/backend/executor/nodeIndexscan.c
@@ -25,7 +25,7 @@
 #include "postgres.h"
 
 #include "access/nbtree.h"
-#include "access/relscan.h"
+#include "access/relscan_details.h"
 #include "executor/execdebug.h"
 #include "executor/nodeIndexscan.h"
 #include "optimizer/clauses.h"
diff --git a/src/backend/executor/nodeLockRows.c b/src/backend/executor/nodeLockRows.c
index 5b5c705..ececab4 100644
--- a/src/backend/executor/nodeLockRows.c
+++ b/src/backend/executor/nodeLockRows.c
@@ -21,6 +21,7 @@
 
 #include "postgres.h"
 
+#include "access/heapam.h"
 #include "access/htup_details.h"
 #include "access/xact.h"
 #include "executor/executor.h"
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 15f5dcc..1948c59 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -37,6 +37,7 @@
 
 #include "postgres.h"
 
+#include "access/heapam.h"
 #include "access/htup_details.h"
 #include "access/xact.h"
 #include "commands/trigger.h"
diff --git a/src/backend/executor/nodeSeqscan.c b/src/backend/executor/nodeSeqscan.c
index 366e784..4a831f2 100644
--- a/src/backend/executor/nodeSeqscan.c
+++ b/src/backend/executor/nodeSeqscan.c
@@ -24,7 +24,7 @@
  */
 #include "postgres.h"
 
-#include "access/relscan.h"
+#include "access/relscan_details.h"
 #include "executor/execdebug.h"
 #include "executor/nodeSeqscan.h"
 #include "utils/rel.h"
diff --git a/src/backend/executor/nodeTidscan.c b/src/backend/executor/nodeTidscan.c
index 316a4ed..41720dd 100644
--- a/src/backend/executor/nodeTidscan.c
+++ b/src/backend/executor/nodeTidscan.c
@@ -24,6 +24,7 @@
  */
 #include "postgres.h"
 
+#include "access/heapam.h"
 #include "access/sysattr.h"
 #include "catalog/pg_type.h"
 #include "executor/execdebug.h"
diff --git a/src/backend/parser/parse_relation.c b/src/backend/parser/parse_relation.c
index 39922d3..7ac2256 100644
--- a/src/backend/parser/parse_relation.c
+++ b/src/backend/parser/parse_relation.c
@@ -16,6 +16,7 @@
 
 #include <ctype.h>
 
+#include "access/heapam.h"
 #include "access/htup_details.h"
 #include "access/sysattr.h"
 #include "catalog/heap.h"
diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index 19d19e5f..8baaaf2 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -26,6 +26,8 @@
 
 #include "postgres.h"
 
+#include "access/genam.h"
+#include "access/heapam.h"
 #include "access/htup_details.h"
 #include "access/reloptions.h"
 #include "catalog/dependency.h"
diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
index 8a9a703..55e7a10 100644
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -13,6 +13,7 @@
  */
 #include "postgres.h"
 
+#include "access/heapam.h"
 #include "access/sysattr.h"
 #include "catalog/pg_type.h"
 #include "commands/trigger.h"
diff --git a/src/backend/storage/ipc/procsignal.c b/src/backend/storage/ipc/procsignal.c
index c4b5d01..8a116ea 100644
--- a/src/backend/storage/ipc/procsignal.c
+++ b/src/backend/storage/ipc/procsignal.c
@@ -25,6 +25,7 @@
 #include "storage/shmem.h"
 #include "storage/sinval.h"
 #include "tcop/tcopprot.h"
+#include "storage/shmem.h"
 
 
 /*
diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c
index 65edc1f..c3265c8 100644
--- a/src/backend/utils/adt/ri_triggers.c
+++ b/src/backend/utils/adt/ri_triggers.c
@@ -30,7 +30,9 @@
 
 #include "postgres.h"
 
+#include "access/heapam.h"
 #include "access/htup_details.h"
+#include "access/xact.h"
 #include "access/sysattr.h"
 #include "access/xact.h"
 #include "catalog/pg_collation.h"
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 9a1d12e..f66a6fc 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -18,6 +18,8 @@
 #include <unistd.h>
 #include <fcntl.h>
 
+#include "access/genam.h"
+#include "access/heapam.h"
 #include "access/htup_details.h"
 #include "access/sysattr.h"
 #include "catalog/dependency.h"
diff --git a/src/backend/utils/adt/selfuncs.c b/src/backend/utils/adt/selfuncs.c
index edfd843..14261ad 100644
--- a/src/backend/utils/adt/selfuncs.c
+++ b/src/backend/utils/adt/selfuncs.c
@@ -100,7 +100,9 @@
 #include <ctype.h>
 #include <math.h>
 
+#include "access/genam.h"
 #include "access/gin.h"
+#include "access/heapam.h"
 #include "access/htup_details.h"
 #include "access/sysattr.h"
 #include "catalog/index.h"
diff --git a/src/backend/utils/adt/xml.c b/src/backend/utils/adt/xml.c
index 25ab79b..45e2778 100644
--- a/src/backend/utils/adt/xml.c
+++ b/src/backend/utils/adt/xml.c
@@ -67,6 +67,7 @@
 #endif
 #endif   /* USE_LIBXML */
 
+#include "access/heapam.h"
 #include "access/htup_details.h"
 #include "catalog/namespace.h"
 #include "catalog/pg_type.h"
diff --git a/src/backend/utils/cache/catcache.c b/src/backend/utils/cache/catcache.c
index c467f11..9d925b9 100644
--- a/src/backend/utils/cache/catcache.c
+++ b/src/backend/utils/cache/catcache.c
@@ -17,7 +17,7 @@
 #include "access/genam.h"
 #include "access/hash.h"
 #include "access/heapam.h"
-#include "access/relscan.h"
+#include "access/relscan_details.h"
 #include "access/sysattr.h"
 #include "access/tuptoaster.h"
 #include "access/valid.h"
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index b4cc6ad..c38e428 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -30,6 +30,8 @@
 #include <fcntl.h>
 #include <unistd.h>
 
+#include "access/genam.h"
+#include "access/heapam.h"
 #include "access/htup_details.h"
 #include "access/multixact.h"
 #include "access/reloptions.h"
diff --git a/src/backend/utils/fmgr/funcapi.c b/src/backend/utils/fmgr/funcapi.c
index 6347a8f..7c0ce83 100644
--- a/src/backend/utils/fmgr/funcapi.c
+++ b/src/backend/utils/fmgr/funcapi.c
@@ -13,6 +13,7 @@
  */
 #include "postgres.h"
 
+#include "access/heapam.h"
 #include "access/htup_details.h"
 #include "catalog/namespace.h"
 #include "catalog/pg_proc.h"
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index 2c7f0f1..ae780e1 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -19,6 +19,7 @@
 #include <fcntl.h>
 #include <unistd.h>
 
+#include "access/genam.h"
 #include "access/heapam.h"
 #include "access/htup_details.h"
 #include "access/sysattr.h"
diff --git a/src/include/access/genam.h b/src/include/access/genam.h
index a800041..c1d0186 100644
--- a/src/include/access/genam.h
+++ b/src/include/access/genam.h
@@ -14,6 +14,7 @@
 #ifndef GENAM_H
 #define GENAM_H
 
+#include "access/relscan.h"
 #include "access/sdir.h"
 #include "access/skey.h"
 #include "nodes/tidbitmap.h"
@@ -79,10 +80,6 @@ typedef struct IndexBulkDeleteResult
 /* Typedef for callback function to determine if a tuple is bulk-deletable */
 typedef bool (*IndexBulkDeleteCallback) (ItemPointer itemptr, void *state);
 
-/* struct definitions appear in relscan.h */
-typedef struct IndexScanDescData *IndexScanDesc;
-typedef struct SysScanDescData *SysScanDesc;
-
 /*
  * Enumeration specifying the type of uniqueness check to perform in
  * index_insert().
diff --git a/src/include/access/heapam.h b/src/include/access/heapam.h
index 0d40398..a7febc6 100644
--- a/src/include/access/heapam.h
+++ b/src/include/access/heapam.h
@@ -14,6 +14,7 @@
 #ifndef HEAPAM_H
 #define HEAPAM_H
 
+#include "access/relscan.h"
 #include "access/sdir.h"
 #include "access/skey.h"
 #include "nodes/primnodes.h"
@@ -94,9 +95,6 @@ extern Relation heap_openrv_extended(const RangeVar *relation,
 
 #define heap_close(r,l)  relation_close(r,l)
 
-/* struct definition appears in relscan.h */
-typedef struct HeapScanDescData *HeapScanDesc;
-
 /*
  * HeapScanIsValid
  *		True iff the heap scan is valid.
diff --git a/src/include/access/relscan.h b/src/include/access/relscan.h
index 3a86ca4..cc46914 100644
--- a/src/include/access/relscan.h
+++ b/src/include/access/relscan.h
@@ -1,7 +1,7 @@
 /*-------------------------------------------------------------------------
  *
  * relscan.h
- *	  POSTGRES relation scan descriptor definitions.
+ *	  POSTGRES relation scan descriptor struct declarations.
  *
  *
  * Portions Copyright (c) 1996-2013, PostgreSQL Global Development Group
@@ -14,95 +14,14 @@
 #ifndef RELSCAN_H
 #define RELSCAN_H
 
-#include "access/genam.h"
-#include "access/heapam.h"
-#include "access/htup_details.h"
-#include "access/itup.h"
-#include "access/tupdesc.h"
+/* struct definitions appear in relscan_details.h */
+typedef struct HeapScanDescData HeapScanDescData;
+typedef struct HeapScanDescData *HeapScanDesc;
 
+typedef struct IndexScanDescData IndexScanDescData;
+typedef struct IndexScanDescData *IndexScanDesc;
 
-typedef struct HeapScanDescData
-{
-	/* scan parameters */
-	Relation	rs_rd;			/* heap relation descriptor */
-	Snapshot	rs_snapshot;	/* snapshot to see */
-	int			rs_nkeys;		/* number of scan keys */
-	ScanKey		rs_key;			/* array of scan key descriptors */
-	bool		rs_bitmapscan;	/* true if this is really a bitmap scan */
-	bool		rs_pageatatime; /* verify visibility page-at-a-time? */
-	bool		rs_allow_strat; /* allow or disallow use of access strategy */
-	bool		rs_allow_sync;	/* allow or disallow use of syncscan */
-	bool		rs_temp_snap;	/* unregister snapshot at scan end? */
+typedef struct SysScanDescData SysScanDescData;
+typedef struct SysScanDescData *SysScanDesc;
 
-	/* state set up at initscan time */
-	BlockNumber rs_nblocks;		/* number of blocks to scan */
-	BlockNumber rs_startblock;	/* block # to start at */
-	BufferAccessStrategy rs_strategy;	/* access strategy for reads */
-	bool		rs_syncscan;	/* report location to syncscan logic? */
-
-	/* scan current state */
-	bool		rs_inited;		/* false = scan not init'd yet */
-	HeapTupleData rs_ctup;		/* current tuple in scan, if any */
-	BlockNumber rs_cblock;		/* current block # in scan, if any */
-	Buffer		rs_cbuf;		/* current buffer in scan, if any */
-	/* NB: if rs_cbuf is not InvalidBuffer, we hold a pin on that buffer */
-	ItemPointerData rs_mctid;	/* marked scan position, if any */
-
-	/* these fields only used in page-at-a-time mode and for bitmap scans */
-	int			rs_cindex;		/* current tuple's index in vistuples */
-	int			rs_mindex;		/* marked tuple's saved index */
-	int			rs_ntuples;		/* number of visible tuples on page */
-	OffsetNumber rs_vistuples[MaxHeapTuplesPerPage];	/* their offsets */
-}	HeapScanDescData;
-
-/*
- * We use the same IndexScanDescData structure for both amgettuple-based
- * and amgetbitmap-based index scans.  Some fields are only relevant in
- * amgettuple-based scans.
- */
-typedef struct IndexScanDescData
-{
-	/* scan parameters */
-	Relation	heapRelation;	/* heap relation descriptor, or NULL */
-	Relation	indexRelation;	/* index relation descriptor */
-	Snapshot	xs_snapshot;	/* snapshot to see */
-	int			numberOfKeys;	/* number of index qualifier conditions */
-	int			numberOfOrderBys;		/* number of ordering operators */
-	ScanKey		keyData;		/* array of index qualifier descriptors */
-	ScanKey		orderByData;	/* array of ordering op descriptors */
-	bool		xs_want_itup;	/* caller requests index tuples */
-
-	/* signaling to index AM about killing index tuples */
-	bool		kill_prior_tuple;		/* last-returned tuple is dead */
-	bool		ignore_killed_tuples;	/* do not return killed entries */
-	bool		xactStartedInRecovery;	/* prevents killing/seeing killed
-										 * tuples */
-
-	/* index access method's private state */
-	void	   *opaque;			/* access-method-specific info */
-
-	/* in an index-only scan, this is valid after a successful amgettuple */
-	IndexTuple	xs_itup;		/* index tuple returned by AM */
-	TupleDesc	xs_itupdesc;	/* rowtype descriptor of xs_itup */
-
-	/* xs_ctup/xs_cbuf/xs_recheck are valid after a successful index_getnext */
-	HeapTupleData xs_ctup;		/* current heap tuple, if any */
-	Buffer		xs_cbuf;		/* current heap buffer in scan, if any */
-	/* NB: if xs_cbuf is not InvalidBuffer, we hold a pin on that buffer */
-	bool		xs_recheck;		/* T means scan keys must be rechecked */
-
-	/* state data for traversing HOT chains in index_getnext */
-	bool		xs_continue_hot;	/* T if must keep walking HOT chain */
-}	IndexScanDescData;
-
-/* Struct for heap-or-index scans of system tables */
-typedef struct SysScanDescData
-{
-	Relation	heap_rel;		/* catalog being scanned */
-	Relation	irel;			/* NULL if doing heap scan */
-	HeapScanDesc scan;			/* only valid in heap-scan case */
-	IndexScanDesc iscan;		/* only valid in index-scan case */
-	Snapshot	snapshot;		/* snapshot to unregister at end of scan */
-}	SysScanDescData;
-
-#endif   /* RELSCAN_H */
+#endif	/* RELSCAN_H */
diff --git a/src/include/access/relscan_details.h b/src/include/access/relscan_details.h
new file mode 100644
index 0000000..34e308f
--- /dev/null
+++ b/src/include/access/relscan_details.h
@@ -0,0 +1,108 @@
+/*-------------------------------------------------------------------------
+ *
+ * relscan_details.h
+ *	  POSTGRES relation scan descriptor definitions.
+ *
+ *
+ * Portions Copyright (c) 1996-2013, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ * src/include/access/relscan_details.h
+ *
+ *-------------------------------------------------------------------------
+ */
+#ifndef RELSCAN_DETAILS_H
+#define RELSCAN_DETAILS_H
+
+#include "access/genam.h"
+#include "access/heapam.h"
+#include "access/htup_details.h"
+#include "access/itup.h"
+#include "access/tupdesc.h"
+
+
+struct HeapScanDescData
+{
+	/* scan parameters */
+	Relation	rs_rd;			/* heap relation descriptor */
+	Snapshot	rs_snapshot;	/* snapshot to see */
+	int			rs_nkeys;		/* number of scan keys */
+	ScanKey		rs_key;			/* array of scan key descriptors */
+	bool		rs_bitmapscan;	/* true if this is really a bitmap scan */
+	bool		rs_pageatatime; /* verify visibility page-at-a-time? */
+	bool		rs_allow_strat; /* allow or disallow use of access strategy */
+	bool		rs_allow_sync;	/* allow or disallow use of syncscan */
+	bool		rs_temp_snap;	/* unregister snapshot at scan end? */
+
+	/* state set up at initscan time */
+	BlockNumber rs_nblocks;		/* number of blocks to scan */
+	BlockNumber rs_startblock;	/* block # to start at */
+	BufferAccessStrategy rs_strategy;	/* access strategy for reads */
+	bool		rs_syncscan;	/* report location to syncscan logic? */
+
+	/* scan current state */
+	bool		rs_inited;		/* false = scan not init'd yet */
+	HeapTupleData rs_ctup;		/* current tuple in scan, if any */
+	BlockNumber rs_cblock;		/* current block # in scan, if any */
+	Buffer		rs_cbuf;		/* current buffer in scan, if any */
+	/* NB: if rs_cbuf is not InvalidBuffer, we hold a pin on that buffer */
+	ItemPointerData rs_mctid;	/* marked scan position, if any */
+
+	/* these fields only used in page-at-a-time mode and for bitmap scans */
+	int			rs_cindex;		/* current tuple's index in vistuples */
+	int			rs_mindex;		/* marked tuple's saved index */
+	int			rs_ntuples;		/* number of visible tuples on page */
+	OffsetNumber rs_vistuples[MaxHeapTuplesPerPage];	/* their offsets */
+};
+
+/*
+ * We use the same IndexScanDescData structure for both amgettuple-based
+ * and amgetbitmap-based index scans.  Some fields are only relevant in
+ * amgettuple-based scans.
+ */
+struct IndexScanDescData
+{
+	/* scan parameters */
+	Relation	heapRelation;	/* heap relation descriptor, or NULL */
+	Relation	indexRelation;	/* index relation descriptor */
+	Snapshot	xs_snapshot;	/* snapshot to see */
+	int			numberOfKeys;	/* number of index qualifier conditions */
+	int			numberOfOrderBys;		/* number of ordering operators */
+	ScanKey		keyData;		/* array of index qualifier descriptors */
+	ScanKey		orderByData;	/* array of ordering op descriptors */
+	bool		xs_want_itup;	/* caller requests index tuples */
+
+	/* signaling to index AM about killing index tuples */
+	bool		kill_prior_tuple;		/* last-returned tuple is dead */
+	bool		ignore_killed_tuples;	/* do not return killed entries */
+	bool		xactStartedInRecovery;	/* prevents killing/seeing killed
+										 * tuples */
+
+	/* index access method's private state */
+	void	   *opaque;			/* access-method-specific info */
+
+	/* in an index-only scan, this is valid after a successful amgettuple */
+	IndexTuple	xs_itup;		/* index tuple returned by AM */
+	TupleDesc	xs_itupdesc;	/* rowtype descriptor of xs_itup */
+
+	/* xs_ctup/xs_cbuf/xs_recheck are valid after a successful index_getnext */
+	HeapTupleData xs_ctup;		/* current heap tuple, if any */
+	Buffer		xs_cbuf;		/* current heap buffer in scan, if any */
+	/* NB: if xs_cbuf is not InvalidBuffer, we hold a pin on that buffer */
+	bool		xs_recheck;		/* T means scan keys must be rechecked */
+
+	/* state data for traversing HOT chains in index_getnext */
+	bool		xs_continue_hot;	/* T if must keep walking HOT chain */
+};
+
+/* Struct for heap-or-index scans of system tables */
+struct SysScanDescData
+{
+	Relation	heap_rel;		/* catalog being scanned */
+	Relation	irel;			/* NULL if doing heap scan */
+	HeapScanDesc scan;			/* only valid in heap-scan case */
+	IndexScanDesc iscan;		/* only valid in index-scan case */
+	Snapshot	snapshot;		/* snapshot to unregister at end of scan */
+};
+
+#endif   /* RELSCAN_DETAILS_H */
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index 3b430e0..9980ca7 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -14,12 +14,17 @@
 #ifndef EXECNODES_H
 #define EXECNODES_H
 
-#include "access/genam.h"
-#include "access/heapam.h"
+#include "access/relscan.h"
+#include "access/skey.h"
 #include "executor/instrument.h"
+#include "fmgr.h"
 #include "nodes/params.h"
 #include "nodes/plannodes.h"
+#include "nodes/tidbitmap.h"
+#include "utils/hsearch.h"
+#include "utils/relcache.h"
 #include "utils/reltrigger.h"
+#include "utils/snapshot.h"
 #include "utils/sortsupport.h"
 #include "utils/tuplestore.h"

Noah Misch

noah@leadboat.com

over 12 years ago

In reply to: Alvaro Herrera (#1)

Re: relscan_details.h

On Mon, Sep 16, 2013 at 11:02:28PM -0300, Alvaro Herrera wrote:

relscan.h is a bit of an unfortunate header because it requires a lot of
other headers to compile, and is also required by execnodes.h. This

Not in my copy of the source tree. execnodes.h uses the corresponding
typedefs that appear in genam.h and heapam.h. Indeed, "find . -name '*.Po' |
xargs grep -l relscan.h | wc -l" only locates 26 files that include relscan.h
directly or indirectly.

quick patch removes the struct definitions from that file, moving them
into a new relscan_details.h file; the reworked relscan.h does not need
any other header, freeing execnodes.h from needlessly including lots of
unnecessary stuff all over the place. Only the files that really need
the struct definition include the new file, and as seen in the patch,
they are quite few.

execnodes.h is included by 147 backend source files; relscan_details.h
only 27. So the exposure area is reduced considerably. This patch also
modifies a few other backend source files to add one or both of genam.h
and heapam.h, which were previously included through execnodes.h.

This is probably missing tweaks for contrib/. The following modules
would need to include relscan_details.h: sepgsql dblink pgstattuple
pgrowlocks. I expect the effect on third-party modules to be much less
than by the htup_details.h change.

-1 on header restructuring in the absence of noteworthy compile-time benchmark
improvements. Besides the obvious benchmark of full-build time, one could
exercise the benefit of fewer header dependencies by modelling the series of
compile times from running "git pull; make" at each commit for the past
several months. The latter benchmark is perhaps more favorable.

--
Noah Misch
EnterpriseDB http://www.enterprisedb.com

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Alvaro Herrera

alvherre@2ndquadrant.com

over 12 years ago

In reply to: Noah Misch (#2)

Re: relscan_details.h

Noah Misch wrote:

On Mon, Sep 16, 2013 at 11:02:28PM -0300, Alvaro Herrera wrote:

relscan.h is a bit of an unfortunate header because it requires a lot of
other headers to compile, and is also required by execnodes.h. This

Not in my copy of the source tree. execnodes.h uses the corresponding
typedefs that appear in genam.h and heapam.h. Indeed, "find . -name '*.Po' |
xargs grep -l relscan.h | wc -l" only locates 26 files that include relscan.h
directly or indirectly.

Ah, I see now that I misspoke. This changeset has been dormant on my
repo for a while, so I confused the detail of what it is about. The
real benefit of this change is to eliminate indirect inclusion of
genam.h and heapam.h. The number of indirect inclusions of each of them
is:
HEAD with patch
heapam.h 219 118
genam.h 226 121

-1 on header restructuring in the absence of noteworthy compile-time benchmark
improvements. Besides the obvious benchmark of full-build time, one could
exercise the benefit of fewer header dependencies by modelling the series of
compile times from running "git pull; make" at each commit for the past
several months. The latter benchmark is perhaps more favorable.

I will have a go at measuring things this way.

--
ï¿½lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Robert Haas

robertmhaas@gmail.com

over 12 years ago

In reply to: Alvaro Herrera (#3)

Re: relscan_details.h

On Tue, Sep 17, 2013 at 2:30 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:

-1 on header restructuring in the absence of noteworthy compile-time benchmark
improvements. Besides the obvious benchmark of full-build time, one could
exercise the benefit of fewer header dependencies by modelling the series of
compile times from running "git pull; make" at each commit for the past
several months. The latter benchmark is perhaps more favorable.

I will have a go at measuring things this way.

Personally, I'm not particularly in favor of these kinds of changes.
The changes we made last time broke a lot of extensions - including
some proprietary EDB ones that I had to go fix. I think a lot of
people spent a lot of time fixing broken builds, at EDB and elsewhere,
as well as rebasing patches. And the only benefit we have to balance
that out is that incremental recompiles are faster, and I'm not really
sure how important that actually is. On my system, configure takes 25
seconds and make -j3 takes 65 seconds; so, even a full recompile is
pretty darn fast, and an incremental recompile is usually really fast.
Now granted this is a relatively new system, but still.

I don't want to be too dogmatic in opposing this; I accept that we
should, from time to time, refactor things. If we don't, superflouous
dependencies will probably proliferate over time. But personally, I'd
rather do these changes in bulk every third release or so and keep
them to a minimum in between times, so that we're not forcing people
to constantly decorate their extensions with new #if directives.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Alvaro Herrera

alvherre@2ndquadrant.com

over 12 years ago

In reply to: Robert Haas (#4)

Re: relscan_details.h

Robert Haas escribiï¿½:

Personally, I'm not particularly in favor of these kinds of changes.
The changes we made last time broke a lot of extensions - including
some proprietary EDB ones that I had to go fix. I think a lot of
people spent a lot of time fixing broken builds, at EDB and elsewhere,
as well as rebasing patches. And the only benefit we have to balance
that out is that incremental recompiles are faster, and I'm not really
sure how important that actually is. On my system, configure takes 25
seconds and make -j3 takes 65 seconds; so, even a full recompile is
pretty darn fast, and an incremental recompile is usually really fast.
Now granted this is a relatively new system, but still.

Fortunately the machines I work on now are also reasonably fast. There
was a time when my desktop was so slow that it paid off to tweak certain
file timestamps to avoid spurious recompiles. Now I don't have to
worry. But it still annoys me that I have enough time to context-switch
to, say, the email client or web browser, from where I don't switch back
so quickly; which means I waste five or ten minutes for a task that
should have taken 20 seconds.

Now, htup_details.h was a bit different than the case at hand because
there's evidently lots of code that want to deal with the guts of
tuples, but for scans you mainly want to start one, iterate and finish,
but don't care much about the innards. So the cleanup work required is
going to be orders of magnitude smaller.

OTOH, back then we had the alternative of doing the split in such a way
that third-party code wouldn't have been affected that heavily, but we
failed to take that into consideration. I trust we have all learned
that lesson and will keep it in mind for next time.

I don't want to be too dogmatic in opposing this; I accept that we
should, from time to time, refactor things. If we don't, superflouous
dependencies will probably proliferate over time. But personally, I'd
rather do these changes in bulk every third release or so and keep
them to a minimum in between times, so that we're not forcing people
to constantly decorate their extensions with new #if directives.

We can batch things, sure, but I don't think doing it only once every
three years is the right tradeoff. There's not all that much of this
refactoring, after all.

--
ï¿½lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Robert Haas

robertmhaas@gmail.com

over 12 years ago

In reply to: Alvaro Herrera (#5)

Re: relscan_details.h

On Tue, Sep 17, 2013 at 4:54 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:

Now, htup_details.h was a bit different than the case at hand because
there's evidently lots of code that want to deal with the guts of
tuples, but for scans you mainly want to start one, iterate and finish,
but don't care much about the innards. So the cleanup work required is
going to be orders of magnitude smaller.

I think the point is that we failed to predict the knock-on
consequences of that refactoring accurately. If we make enough such
changes, we will probably face such issues again. Sure, we can hope
that our ability to predict which changes will be disruptive will
improve with practice, but I doubt it's ever going to be perfect.

I certainly don't have the only vote here. I'm just telling you that
from my point of view the last round of changes was a noticeable
headache and I don't really feel that I'm better off because of it, so
I'm in not in favor of continuing to make such changes on a regular
basis.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Noah Misch

noah@leadboat.com

over 12 years ago

In reply to: Alvaro Herrera (#5)

Re: relscan_details.h

On Tue, Sep 17, 2013 at 05:54:04PM -0300, Alvaro Herrera wrote:

Robert Haas escribi?:

Personally, I'm not particularly in favor of these kinds of changes.
The changes we made last time broke a lot of extensions - including
some proprietary EDB ones that I had to go fix. I think a lot of
people spent a lot of time fixing broken builds, at EDB and elsewhere,
as well as rebasing patches. And the only benefit we have to balance
that out is that incremental recompiles are faster, and I'm not really
sure how important that actually is. On my system, configure takes 25
seconds and make -j3 takes 65 seconds; so, even a full recompile is
pretty darn fast, and an incremental recompile is usually really fast.
Now granted this is a relatively new system, but still.

Fortunately the machines I work on now are also reasonably fast. There
was a time when my desktop was so slow that it paid off to tweak certain
file timestamps to avoid spurious recompiles. Now I don't have to
worry. But it still annoys me that I have enough time to context-switch
to, say, the email client or web browser, from where I don't switch back
so quickly; which means I waste five or ten minutes for a task that
should have taken 20 seconds.

Right. If we can speed up a representative sample of incremental recompiles
by 20%, then I'm on board. At 3%, probably not. (Alas, even 20% doesn't move
it out of the causes-context-switch category. For that, I think you need
fundamentally smarter tools.)

Now, htup_details.h was a bit different than the case at hand because
there's evidently lots of code that want to deal with the guts of
tuples, but for scans you mainly want to start one, iterate and finish,
but don't care much about the innards. So the cleanup work required is
going to be orders of magnitude smaller.

There will also be the folks who must add heapam.h and/or genam.h includes
despite formerly getting it/them through execnodes.h. That's not ugly like
"#if PG_VERSION_NUM ...", but it's still work for authors.

--
Noah Misch
EnterpriseDB http://www.enterprisedb.com

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Bruce Momjian

bruce@momjian.us

over 12 years ago

In reply to: Alvaro Herrera (#5)

Re: relscan_details.h

On Tue, Sep 17, 2013 at 05:54:04PM -0300, Alvaro Herrera wrote:

I don't want to be too dogmatic in opposing this; I accept that we
should, from time to time, refactor things. If we don't, superflouous
dependencies will probably proliferate over time. But personally, I'd
rather do these changes in bulk every third release or so and keep
them to a minimum in between times, so that we're not forcing people
to constantly decorate their extensions with new #if directives.

We can batch things, sure, but I don't think doing it only once every
three years is the right tradeoff. There's not all that much of this
refactoring, after all.

Agreed, we should go ahead and make the changes. People who use our
code for plugins are already going to have a boat-load of stuff to
change in each major release, and doing this isn't going to add a lot of
burden, and it will give us cleaner code for the future.

If we had not made massive cleanup changes years ago, our code would not
be as good as it is today. By avoiding cleanup to reduce the burden on
people who use our code, we are positioning our code on a slow decline
in clarity.

What I don't want to do is get into a mode where every code cleanup has
to be verified that it isn't going to excessively burden outside code
users. Cleanup is hard enough, and adding another check to that process
makes cleanup even less likely.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Peter Geoghegan

pg@heroku.com

over 12 years ago

In reply to: Robert Haas (#4)

Re: relscan_details.h

On Tue, Sep 17, 2013 at 12:59 PM, Robert Haas <robertmhaas@gmail.com> wrote:

Personally, I'm not particularly in favor of these kinds of changes.

+1. Experience has shown this kind of effort to be a tarpit. It turns
out that refactoring away compiler dependencies has this kind of
fractal complexity - the more you look at it, the less sense it makes.

I would be in favor of this if there were compelling gains in either
compile time (and like others, I have a pretty high bar for compelling
here), or if the refactoring effort remedied a clear modularity
violation. Though I think it has to happen every once in a while, I'd
suggest about every 5 years as the right interval.

--
Peter Geoghegan

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#10

Noah Misch

noah@leadboat.com

over 12 years ago

In reply to: Bruce Momjian (#8)

Re: relscan_details.h

On Tue, Oct 01, 2013 at 10:12:05PM -0400, Bruce Momjian wrote:

If we had not made massive cleanup changes years ago, our code would not
be as good as it is today. By avoiding cleanup to reduce the burden on
people who use our code, we are positioning our code on a slow decline
in clarity.

What I don't want to do is get into a mode where every code cleanup has
to be verified that it isn't going to excessively burden outside code
users. Cleanup is hard enough, and adding another check to that process
makes cleanup even less likely.

I don't disagree with that, but I would describe the proposal as a mild
dirty-up for the sake of build performance, not a cleanup.

--
Noah Misch
EnterpriseDB http://www.enterprisedb.com

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#11

Bruce Momjian

bruce@momjian.us

over 12 years ago

In reply to: Noah Misch (#10)

Re: relscan_details.h

On Wed, Oct 2, 2013 at 08:59:42AM -0400, Noah Misch wrote:

On Tue, Oct 01, 2013 at 10:12:05PM -0400, Bruce Momjian wrote:

If we had not made massive cleanup changes years ago, our code would not
be as good as it is today. By avoiding cleanup to reduce the burden on
people who use our code, we are positioning our code on a slow decline
in clarity.

What I don't want to do is get into a mode where every code cleanup has
to be verified that it isn't going to excessively burden outside code
users. Cleanup is hard enough, and adding another check to that process
makes cleanup even less likely.

I don't disagree with that, but I would describe the proposal as a mild
dirty-up for the sake of build performance, not a cleanup.

OK, good enough. Thanks for the feedback.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#12

Alvaro Herrera

alvherre@2ndquadrant.com

over 12 years ago

In reply to: Bruce Momjian (#11)

Re: relscan_details.h

Bruce Momjian escribiï¿½:

On Wed, Oct 2, 2013 at 08:59:42AM -0400, Noah Misch wrote:

On Tue, Oct 01, 2013 at 10:12:05PM -0400, Bruce Momjian wrote:

If we had not made massive cleanup changes years ago, our code would not
be as good as it is today. By avoiding cleanup to reduce the burden on
people who use our code, we are positioning our code on a slow decline
in clarity.

What I don't want to do is get into a mode where every code cleanup has
to be verified that it isn't going to excessively burden outside code
users. Cleanup is hard enough, and adding another check to that process
makes cleanup even less likely.

I don't disagree with that, but I would describe the proposal as a mild
dirty-up for the sake of build performance, not a cleanup.

OK, good enough. Thanks for the feedback.

Agreed. I will keep this patch in my local repo; I might present it
again later as part of more invasive surgery.

--
ï¿½lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers