Hi,
On 2015-08-06 12:43:06 -0300, Alvaro Herrera wrote:
> Andres Freund wrote:
> > On 2015-08-06 12:29:15 -0300, Alvaro Herrera wrote:
>
> > > Ah, but that's because you cheated and didn't remove the include from
> > > namespace.h ...
> >
> > Well, it's not included from frontend code, so I didn't see the need?
> > Going through all the backend code and replacing lock.h by lockdefs.h
> > and some other includes doesn't seem particularly beneficial to me.
> >
> > FWIW, removing it from namespace.h is relatively easy. It starts to get
> > a lot more noisy when you want to touch heapam.h.
>
> Ah, heapam.h is the granddaddy of all header messes, isn't it. (Actually
> it's execnodes.h or something like that.)
> > > > diff --git a/src/include/storage/lockdefs.h
> > > > b/src/include/storage/lockdefs.h
> > > > new file mode 100644
> > > > index 0000000..bfbcdba
> > > > --- /dev/null
> > > > +++ b/src/include/storage/lockdefs.h
> > > > @@ -0,0 +1,56 @@
> > > > +/*-------------------------------------------------------------------------
> > > > + *
> > > > + * lockdefs.h
> > > > + * Frontend exposed parts of postgres' low level lock mechanism
> > > > + *
> > > > + * The split between lockdefs.h and lock.h is not very principled.
> > >
> > > No kidding!
> >
> > Do you have a good suggestion about the split? I wanted to expose the
> > minimal amount necessary, and those were the ones.
>
> Nope, nothing useful, sorry.
To pick up on the general discussion and on the points you made here:
I actually think the split came out to work far better than I'd
anticipated. Having a slimmed-down version of lock.h for code that just
needs to declare/pass lockmode parameters seems to improve our layering
considerably. I got round to Alvaro's and Roberts position that
removing lock.h from namespace.h and heapam.h would be a really nice
improvemen on that front.
Attached is a WIP patch to that end. After the further changes only few
headers still have to include lock.h and they're pretty firmly in the
backend-only territory. It also allows to remove the uglyness of passing
around LOCKMODE as an int in parserOpenTable().
Imo lockdefs.h should be updated to describe that it contains the part
of the lock infrastructure needed by indirect users of lock.h
infrastructure, and that that currently unfortunately may include some
frontend programs.
I *also* think that removing lwlock.h from lock.h would be a good
idea. In my opinion it's a bad idea to pointlessly expose so much
low-level things to the wider world, even if it's only needed by
relatively low level things. Given that dependent macros are in
lwlock.h, it seems pretty sane to move LockHash* there too.
We could additionally move all but LOCKMETHODID, LockTagType,
LockAcquire*(), LockRelease() and probably one or two more to
lock_internals.h (although I'd maybe rather name it lock_details?). I
think it'd be an improvement, although possibly not a tremendous one
given how many places it's likely going to be included from.
Greetings,
Andres Freund
>From f9a74a50c5c02b1a3276385468ae41359741f9fa Mon Sep 17 00:00:00 2001
From: Andres Freund <and...@anarazel.de>
Date: Fri, 14 Aug 2015 19:36:04 +0200
Subject: [PATCH] WIP: Decrease usage of lock.h further.
---
contrib/pg_stat_statements/pg_stat_statements.c | 2 ++
src/backend/access/common/reloptions.c | 1 +
src/backend/access/heap/syncscan.c | 2 ++
src/backend/access/nbtree/nbtutils.c | 2 ++
src/backend/access/transam/commit_ts.c | 1 +
src/backend/catalog/toasting.c | 1 -
src/backend/commands/discard.c | 1 +
src/backend/commands/policy.c | 1 -
src/backend/commands/tablecmds.c | 1 -
src/backend/parser/parse_relation.c | 6 +-----
src/backend/tsearch/ts_typanalyze.c | 1 +
src/backend/utils/adt/array_typanalyze.c | 1 +
src/backend/utils/cache/ts_cache.c | 1 +
src/include/access/heapam.h | 2 +-
src/include/access/reloptions.h | 2 +-
src/include/catalog/namespace.h | 2 +-
src/include/catalog/pg_inherits_fn.h | 2 +-
src/include/catalog/toasting.h | 2 +-
src/include/commands/cluster.h | 2 +-
src/include/commands/tablecmds.h | 2 +-
src/include/commands/vacuum.h | 2 +-
src/include/nodes/execnodes.h | 1 +
src/include/parser/parse_oper.h | 1 +
src/include/parser/parse_relation.h | 3 ++-
24 files changed, 25 insertions(+), 17 deletions(-)
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 59b8a2e..a3b40e8 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -72,6 +72,8 @@
#include "pgstat.h"
#include "storage/fd.h"
#include "storage/ipc.h"
+#include "storage/lwlock.h"
+#include "storage/shmem.h"
#include "storage/spin.h"
#include "tcop/utility.h"
#include "utils/builtins.h"
diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index 7479d40..8c75ff5 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -26,6 +26,7 @@
#include "commands/tablespace.h"
#include "commands/view.h"
#include "nodes/makefuncs.h"
+#include "storage/lock.h"
#include "utils/array.h"
#include "utils/attoptcache.h"
#include "utils/builtins.h"
diff --git a/src/backend/access/heap/syncscan.c b/src/backend/access/heap/syncscan.c
index 266c330..58241d0 100644
--- a/src/backend/access/heap/syncscan.c
+++ b/src/backend/access/heap/syncscan.c
@@ -49,6 +49,8 @@
#include "access/heapam.h"
#include "miscadmin.h"
#include "utils/rel.h"
+#include "storage/lwlock.h"
+#include "storage/shmem.h"
/* GUC variables */
diff --git a/src/backend/access/nbtree/nbtutils.c b/src/backend/access/nbtree/nbtutils.c
index 91331ba..f36086d 100644
--- a/src/backend/access/nbtree/nbtutils.c
+++ b/src/backend/access/nbtree/nbtutils.c
@@ -25,6 +25,8 @@
#include "utils/lsyscache.h"
#include "utils/memutils.h"
#include "utils/rel.h"
+#include "storage/lwlock.h"
+#include "storage/shmem.h"
typedef struct BTSortArrayContext
diff --git a/src/backend/access/transam/commit_ts.c b/src/backend/access/transam/commit_ts.c
index 5ad35c0..a8d2cc2 100644
--- a/src/backend/access/transam/commit_ts.c
+++ b/src/backend/access/transam/commit_ts.c
@@ -32,6 +32,7 @@
#include "funcapi.h"
#include "miscadmin.h"
#include "pg_trace.h"
+#include "storage/shmem.h"
#include "utils/builtins.h"
#include "utils/snapmgr.h"
#include "utils/timestamp.h"
diff --git a/src/backend/catalog/toasting.c b/src/backend/catalog/toasting.c
index 3652d7b..8e8f32e 100644
--- a/src/backend/catalog/toasting.c
+++ b/src/backend/catalog/toasting.c
@@ -27,7 +27,6 @@
#include "catalog/toasting.h"
#include "miscadmin.h"
#include "nodes/makefuncs.h"
-#include "storage/lock.h"
#include "utils/builtins.h"
#include "utils/rel.h"
#include "utils/syscache.h"
diff --git a/src/backend/commands/discard.c b/src/backend/commands/discard.c
index b40ef35..6ba4efb 100644
--- a/src/backend/commands/discard.c
+++ b/src/backend/commands/discard.c
@@ -19,6 +19,7 @@
#include "commands/discard.h"
#include "commands/prepare.h"
#include "commands/sequence.h"
+#include "storage/lock.h"
#include "utils/guc.h"
#include "utils/portal.h"
diff --git a/src/backend/commands/policy.c b/src/backend/commands/policy.c
index bcf4a8f..977c315 100644
--- a/src/backend/commands/policy.c
+++ b/src/backend/commands/policy.c
@@ -35,7 +35,6 @@
#include "parser/parse_relation.h"
#include "rewrite/rewriteManip.h"
#include "rewrite/rowsecurity.h"
-#include "storage/lock.h"
#include "utils/acl.h"
#include "utils/array.h"
#include "utils/builtins.h"
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 126b119..b625bac 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -77,7 +77,6 @@
#include "rewrite/rewriteManip.h"
#include "storage/bufmgr.h"
#include "storage/lmgr.h"
-#include "storage/lock.h"
#include "storage/predicate.h"
#include "storage/smgr.h"
#include "utils/acl.h"
diff --git a/src/backend/parser/parse_relation.c b/src/backend/parser/parse_relation.c
index 0b2dacf..e8c6313 100644
--- a/src/backend/parser/parse_relation.c
+++ b/src/backend/parser/parse_relation.c
@@ -1110,13 +1110,9 @@ chooseScalarFunctionAlias(Node *funcexpr, char *funcname,
* This is essentially just the same as heap_openrv(), except that it caters
* to some parser-specific error reporting needs, notably that it arranges
* to include the RangeVar's parse location in any resulting error.
- *
- * Note: properly, lockmode should be declared LOCKMODE not int, but that
- * would require importing storage/lock.h into parse_relation.h. Since
- * LOCKMODE is typedef'd as int anyway, that seems like overkill.
*/
Relation
-parserOpenTable(ParseState *pstate, const RangeVar *relation, int lockmode)
+parserOpenTable(ParseState *pstate, const RangeVar *relation, LOCKMODE lockmode)
{
Relation rel;
ParseCallbackState pcbstate;
diff --git a/src/backend/tsearch/ts_typanalyze.c b/src/backend/tsearch/ts_typanalyze.c
index 432b582..c799a5c 100644
--- a/src/backend/tsearch/ts_typanalyze.c
+++ b/src/backend/tsearch/ts_typanalyze.c
@@ -18,6 +18,7 @@
#include "commands/vacuum.h"
#include "tsearch/ts_type.h"
#include "utils/builtins.h"
+#include "utils/hsearch.h"
/* A hash key for lexemes */
diff --git a/src/backend/utils/adt/array_typanalyze.c b/src/backend/utils/adt/array_typanalyze.c
index ffe8035..201614c 100644
--- a/src/backend/utils/adt/array_typanalyze.c
+++ b/src/backend/utils/adt/array_typanalyze.c
@@ -19,6 +19,7 @@
#include "commands/vacuum.h"
#include "utils/array.h"
#include "utils/datum.h"
+#include "utils/hsearch.h"
#include "utils/lsyscache.h"
#include "utils/typcache.h"
diff --git a/src/backend/utils/cache/ts_cache.c b/src/backend/utils/cache/ts_cache.c
index 5b1c358..a10d649 100644
--- a/src/backend/utils/cache/ts_cache.c
+++ b/src/backend/utils/cache/ts_cache.c
@@ -42,6 +42,7 @@
#include "utils/builtins.h"
#include "utils/catcache.h"
#include "utils/fmgroids.h"
+#include "utils/hsearch.h"
#include "utils/inval.h"
#include "utils/lsyscache.h"
#include "utils/memutils.h"
diff --git a/src/include/access/heapam.h b/src/include/access/heapam.h
index 75e6b72..e7f99af 100644
--- a/src/include/access/heapam.h
+++ b/src/include/access/heapam.h
@@ -19,7 +19,7 @@
#include "nodes/lockoptions.h"
#include "nodes/primnodes.h"
#include "storage/bufpage.h"
-#include "storage/lock.h"
+#include "storage/lockdefs.h"
#include "utils/relcache.h"
#include "utils/snapshot.h"
diff --git a/src/include/access/reloptions.h b/src/include/access/reloptions.h
index 2a3cbcd..3e45a50 100644
--- a/src/include/access/reloptions.h
+++ b/src/include/access/reloptions.h
@@ -22,7 +22,7 @@
#include "access/htup.h"
#include "access/tupdesc.h"
#include "nodes/pg_list.h"
-#include "storage/lock.h"
+#include "storage/lockdefs.h"
/* types supported by reloptions */
typedef enum relopt_type
diff --git a/src/include/catalog/namespace.h b/src/include/catalog/namespace.h
index f3b005f..b4c04dd 100644
--- a/src/include/catalog/namespace.h
+++ b/src/include/catalog/namespace.h
@@ -15,7 +15,7 @@
#define NAMESPACE_H
#include "nodes/primnodes.h"
-#include "storage/lock.h"
+#include "storage/lockdefs.h"
/*
diff --git a/src/include/catalog/pg_inherits_fn.h b/src/include/catalog/pg_inherits_fn.h
index 3ff1947..d64ec94 100644
--- a/src/include/catalog/pg_inherits_fn.h
+++ b/src/include/catalog/pg_inherits_fn.h
@@ -15,7 +15,7 @@
#define PG_INHERITS_FN_H
#include "nodes/pg_list.h"
-#include "storage/lock.h"
+#include "storage/lockdefs.h"
extern List *find_inheritance_children(Oid parentrelId, LOCKMODE lockmode);
extern List *find_all_inheritors(Oid parentrelId, LOCKMODE lockmode,
diff --git a/src/include/catalog/toasting.h b/src/include/catalog/toasting.h
index fb2f035..a06772a 100644
--- a/src/include/catalog/toasting.h
+++ b/src/include/catalog/toasting.h
@@ -14,7 +14,7 @@
#ifndef TOASTING_H
#define TOASTING_H
-#include "storage/lock.h"
+#include "storage/lockdefs.h"
/*
* toasting.c prototypes
diff --git a/src/include/commands/cluster.h b/src/include/commands/cluster.h
index 098d09b..f32b3fe 100644
--- a/src/include/commands/cluster.h
+++ b/src/include/commands/cluster.h
@@ -14,7 +14,7 @@
#define CLUSTER_H
#include "nodes/parsenodes.h"
-#include "storage/lock.h"
+#include "storage/lockdefs.h"
#include "utils/relcache.h"
diff --git a/src/include/commands/tablecmds.h b/src/include/commands/tablecmds.h
index f269c63..00366f5 100644
--- a/src/include/commands/tablecmds.h
+++ b/src/include/commands/tablecmds.h
@@ -18,7 +18,7 @@
#include "catalog/dependency.h"
#include "catalog/objectaddress.h"
#include "nodes/parsenodes.h"
-#include "storage/lock.h"
+#include "storage/lockdefs.h"
#include "utils/relcache.h"
diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h
index e3a31af..e688adf 100644
--- a/src/include/commands/vacuum.h
+++ b/src/include/commands/vacuum.h
@@ -19,7 +19,7 @@
#include "catalog/pg_type.h"
#include "nodes/parsenodes.h"
#include "storage/buf.h"
-#include "storage/lock.h"
+#include "storage/lockdefs.h"
#include "utils/relcache.h"
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index 5796de8..c65de97 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -20,6 +20,7 @@
#include "lib/pairingheap.h"
#include "nodes/params.h"
#include "nodes/plannodes.h"
+#include "utils/hsearch.h"
#include "utils/reltrigger.h"
#include "utils/sortsupport.h"
#include "utils/tuplestore.h"
diff --git a/src/include/parser/parse_oper.h b/src/include/parser/parse_oper.h
index ed5332d..8346566 100644
--- a/src/include/parser/parse_oper.h
+++ b/src/include/parser/parse_oper.h
@@ -15,6 +15,7 @@
#define PARSE_OPER_H
#include "access/htup.h"
+#include "utils/hsearch.h"
#include "parser/parse_node.h"
diff --git a/src/include/parser/parse_relation.h b/src/include/parser/parse_relation.h
index e2875a0..cd95b88 100644
--- a/src/include/parser/parse_relation.h
+++ b/src/include/parser/parse_relation.h
@@ -15,6 +15,7 @@
#define PARSE_RELATION_H
#include "parser/parse_node.h"
+#include "storage/lockdefs.h"
/*
@@ -60,7 +61,7 @@ extern Node *colNameToVar(ParseState *pstate, char *colname, bool localonly,
extern void markVarForSelectPriv(ParseState *pstate, Var *var,
RangeTblEntry *rte);
extern Relation parserOpenTable(ParseState *pstate, const RangeVar *relation,
- int lockmode);
+ LOCKMODE lockmode);
extern RangeTblEntry *addRangeTableEntry(ParseState *pstate,
RangeVar *relation,
Alias *alias,
--
2.3.0.149.gf3f4077.dirty
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
