On 2015-08-06 12:05:24 -0300, Alvaro Herrera wrote:
> Andres Freund wrote:
> > On 2015-08-06 10:27:52 -0400, Tom Lane wrote:
> > > Andres Freund <and...@anarazel.de> writes:
> > > >> One approach is to avoid including lwlock.h/slot.h in frontend
> > > >> code. That'll require some minor surgery and adding a couple includes,
> > > >> but it doesn't look that bad.
> > >
> > > > Patch doing that attached.
> > >
> > > This seems kinda messy. Looking at the contents of lock.h, it seems like
> > > getting rid of its dependency on lwlock.h is not really very appropriate,
> > > because there is boatloads of other backend-only stuff in there. Why is
> > > any frontend code including lock.h at all? If there is a valid reason,
> > > should we refactor lock.h into two separate headers, one that is safe to
> > > expose to frontends and one with the rest of the stuff?
> >
> > I think the primary reason for lock.h being included pretty widely is to
> > have the declaration of LOCKMODE. That's pretty widely used in headers
> > included by clients for various reasons. There's also a bit of fun
> > around xl_standby_locks.
>
> I think it is a good idea to split up LOCKMODE so that most headers do
> not need to include lock.h at all; you will need to add an explicit
> #include "storage/lock.h" to a lot of C files, but to me that's a good
> thing.
I had to split of three things: LOCKMASK, the individual lock levels and
xl_standby_lock to be able to prohibit lock.h to be included by frontend
code. lockdefs.h works for me, counter proposals?
There weren't any places that needed additional lock.h includes. But
hashfn.c somewhat hilariously missed utils/hsearch.h ;)
Regards,
Andres
>From bc9dc1910058a3e4638576261f24cb12471e7b15 Mon Sep 17 00:00:00 2001
From: Andres Freund <and...@anarazel.de>
Date: Thu, 6 Aug 2015 12:53:56 +0200
Subject: [PATCH] Don't include low level locking code from frontend code.
To avoid that happening again put some checks agains this happening into
atomics.h, lock.h, lwlock.h and s_lock.h.
---
src/backend/utils/hash/hashfn.c | 1 +
src/include/access/genam.h | 2 +-
src/include/access/hash.h | 2 +-
src/include/access/tuptoaster.h | 2 +-
src/include/catalog/objectaddress.h | 2 +-
src/include/port/atomics.h | 4 +++
src/include/storage/lock.h | 43 ++++------------------------
src/include/storage/lockdefs.h | 56 +++++++++++++++++++++++++++++++++++++
src/include/storage/lwlock.h | 4 +++
src/include/storage/procarray.h | 1 +
src/include/storage/s_lock.h | 4 +++
src/include/storage/standby.h | 2 +-
12 files changed, 80 insertions(+), 43 deletions(-)
create mode 100644 src/include/storage/lockdefs.h
diff --git a/src/backend/utils/hash/hashfn.c b/src/backend/utils/hash/hashfn.c
index 260d880..bdd438d 100644
--- a/src/backend/utils/hash/hashfn.c
+++ b/src/backend/utils/hash/hashfn.c
@@ -22,6 +22,7 @@
#include "postgres.h"
#include "access/hash.h"
+#include "utils/hsearch.h"
/*
diff --git a/src/include/access/genam.h b/src/include/access/genam.h
index d86590a..d9d05a0 100644
--- a/src/include/access/genam.h
+++ b/src/include/access/genam.h
@@ -17,7 +17,7 @@
#include "access/sdir.h"
#include "access/skey.h"
#include "nodes/tidbitmap.h"
-#include "storage/lock.h"
+#include "storage/lockdefs.h"
#include "utils/relcache.h"
#include "utils/snapshot.h"
diff --git a/src/include/access/hash.h b/src/include/access/hash.h
index 93cc8af..97cb859 100644
--- a/src/include/access/hash.h
+++ b/src/include/access/hash.h
@@ -24,7 +24,7 @@
#include "fmgr.h"
#include "lib/stringinfo.h"
#include "storage/bufmgr.h"
-#include "storage/lock.h"
+#include "storage/lockdefs.h"
#include "utils/relcache.h"
/*
diff --git a/src/include/access/tuptoaster.h b/src/include/access/tuptoaster.h
index 7d18535..77f637e 100644
--- a/src/include/access/tuptoaster.h
+++ b/src/include/access/tuptoaster.h
@@ -14,8 +14,8 @@
#define TUPTOASTER_H
#include "access/htup_details.h"
+#include "storage/lockdefs.h"
#include "utils/relcache.h"
-#include "storage/lock.h"
/*
* This enables de-toasting of index entries. Needed until VACUUM is
diff --git a/src/include/catalog/objectaddress.h b/src/include/catalog/objectaddress.h
index 37808c0..0fc16ed 100644
--- a/src/include/catalog/objectaddress.h
+++ b/src/include/catalog/objectaddress.h
@@ -14,7 +14,7 @@
#define OBJECTADDRESS_H
#include "nodes/pg_list.h"
-#include "storage/lock.h"
+#include "storage/lockdefs.h"
#include "utils/acl.h"
#include "utils/relcache.h"
diff --git a/src/include/port/atomics.h b/src/include/port/atomics.h
index bb87945..65cfa3f 100644
--- a/src/include/port/atomics.h
+++ b/src/include/port/atomics.h
@@ -37,6 +37,10 @@
#ifndef ATOMICS_H
#define ATOMICS_H
+#ifdef FRONTEND
+#error "atomics.h may not be included from frontend code"
+#endif
+
#define INSIDE_ATOMICS_H
#include <limits.h>
diff --git a/src/include/storage/lock.h b/src/include/storage/lock.h
index 96fe3a6..a9cd08c 100644
--- a/src/include/storage/lock.h
+++ b/src/include/storage/lock.h
@@ -14,6 +14,11 @@
#ifndef LOCK_H_
#define LOCK_H_
+#ifdef FRONTEND
+#error "lock.h may not be included from frontend code"
+#endif
+
+#include "storage/lockdefs.h"
#include "storage/backendid.h"
#include "storage/lwlock.h"
#include "storage/shmem.h"
@@ -77,15 +82,6 @@ typedef struct
((vxid).backendId = (proc).backendId, \
(vxid).localTransactionId = (proc).lxid)
-
-/*
- * LOCKMODE is an integer (1..N) indicating a lock type. LOCKMASK is a bit
- * mask indicating a set of held or requested lock types (the bit 1<<mode
- * corresponds to a particular lock mode).
- */
-typedef int LOCKMASK;
-typedef int LOCKMODE;
-
/* MAX_LOCKMODES cannot be larger than the # of bits in LOCKMASK */
#define MAX_LOCKMODES 10
@@ -134,28 +130,6 @@ typedef uint16 LOCKMETHODID;
#define USER_LOCKMETHOD 2
/*
- * These are the valid values of type LOCKMODE for all the standard lock
- * methods (both DEFAULT and USER).
- */
-
-/* NoLock is not a lock mode, but a flag value meaning "don't get a lock" */
-#define NoLock 0
-
-#define AccessShareLock 1 /* SELECT */
-#define RowShareLock 2 /* SELECT FOR UPDATE/FOR SHARE */
-#define RowExclusiveLock 3 /* INSERT, UPDATE, DELETE */
-#define ShareUpdateExclusiveLock 4 /* VACUUM (non-FULL),ANALYZE, CREATE
- * INDEX CONCURRENTLY */
-#define ShareLock 5 /* CREATE INDEX (WITHOUT CONCURRENTLY) */
-#define ShareRowExclusiveLock 6 /* like EXCLUSIVE MODE, but allows ROW
- * SHARE */
-#define ExclusiveLock 7 /* blocks ROW SHARE/SELECT...FOR
- * UPDATE */
-#define AccessExclusiveLock 8 /* ALTER TABLE, DROP TABLE, VACUUM
- * FULL, and unqualified LOCK TABLE */
-
-
-/*
* LOCKTAG is the key information needed to look up a LOCK item in the
* lock hashtable. A LOCKTAG value uniquely identifies a lockable object.
*
@@ -536,13 +510,6 @@ extern void RemoveFromWaitQueue(PGPROC *proc, uint32 hashcode);
extern Size LockShmemSize(void);
extern LockData *GetLockStatusData(void);
-typedef struct xl_standby_lock
-{
- TransactionId xid; /* xid of holder of AccessExclusiveLock */
- Oid dbOid;
- Oid relOid;
-} xl_standby_lock;
-
extern xl_standby_lock *GetRunningTransactionLocks(int *nlocks);
extern const char *GetLockmodeName(LOCKMETHODID lockmethodid, LOCKMODE mode);
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. This file
+ * contains definition that have to (indirectly) be available when included by
+ * FRONTEND code.
+ *
+ * Portions Copyright (c) 1996-2015, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ * src/include/storage/lockdefs.h
+ *
+ *-------------------------------------------------------------------------
+ */
+#ifndef LOCKDEFS_H_
+#define LOCKDEFS_H_
+
+/*
+ * LOCKMODE is an integer (1..N) indicating a lock type. LOCKMASK is a bit
+ * mask indicating a set of held or requested lock types (the bit 1<<mode
+ * corresponds to a particular lock mode).
+ */
+typedef int LOCKMASK;
+typedef int LOCKMODE;
+
+/*
+ * These are the valid values of type LOCKMODE for all the standard lock
+ * methods (both DEFAULT and USER).
+ */
+
+/* NoLock is not a lock mode, but a flag value meaning "don't get a lock" */
+#define NoLock 0
+
+#define AccessShareLock 1 /* SELECT */
+#define RowShareLock 2 /* SELECT FOR UPDATE/FOR SHARE */
+#define RowExclusiveLock 3 /* INSERT, UPDATE, DELETE */
+#define ShareUpdateExclusiveLock 4 /* VACUUM (non-FULL),ANALYZE, CREATE
+ * INDEX CONCURRENTLY */
+#define ShareLock 5 /* CREATE INDEX (WITHOUT CONCURRENTLY) */
+#define ShareRowExclusiveLock 6 /* like EXCLUSIVE MODE, but allows ROW
+ * SHARE */
+#define ExclusiveLock 7 /* blocks ROW SHARE/SELECT...FOR
+ * UPDATE */
+#define AccessExclusiveLock 8 /* ALTER TABLE, DROP TABLE, VACUUM
+ * FULL, and unqualified LOCK TABLE */
+
+typedef struct xl_standby_lock
+{
+ TransactionId xid; /* xid of holder of AccessExclusiveLock */
+ Oid dbOid;
+ Oid relOid;
+} xl_standby_lock;
+
+#endif /* LOCKDEF_H_ */
diff --git a/src/include/storage/lwlock.h b/src/include/storage/lwlock.h
index cbd6318..f2ff6a0 100644
--- a/src/include/storage/lwlock.h
+++ b/src/include/storage/lwlock.h
@@ -14,6 +14,10 @@
#ifndef LWLOCK_H
#define LWLOCK_H
+#ifdef FRONTEND
+#error "lwlock.h may not be included from frontend code"
+#endif
+
#include "lib/ilist.h"
#include "storage/s_lock.h"
#include "port/atomics.h"
diff --git a/src/include/storage/procarray.h b/src/include/storage/procarray.h
index a9b40ed..834f99b 100644
--- a/src/include/storage/procarray.h
+++ b/src/include/storage/procarray.h
@@ -14,6 +14,7 @@
#ifndef PROCARRAY_H
#define PROCARRAY_H
+#include "storage/lock.h"
#include "storage/standby.h"
#include "utils/relcache.h"
#include "utils/snapshot.h"
diff --git a/src/include/storage/s_lock.h b/src/include/storage/s_lock.h
index 30f28b0..c461fda 100644
--- a/src/include/storage/s_lock.h
+++ b/src/include/storage/s_lock.h
@@ -96,6 +96,10 @@
#ifndef S_LOCK_H
#define S_LOCK_H
+#ifdef FRONTEND
+#error "s_lock.h may not be included from frontend code"
+#endif
+
#ifdef HAVE_SPINLOCKS /* skip spinlocks if requested */
#if defined(__GNUC__) || defined(__INTEL_COMPILER)
diff --git a/src/include/storage/standby.h b/src/include/storage/standby.h
index 7626c4c..40b329b 100644
--- a/src/include/storage/standby.h
+++ b/src/include/storage/standby.h
@@ -16,7 +16,7 @@
#include "access/xlogreader.h"
#include "lib/stringinfo.h"
-#include "storage/lock.h"
+#include "storage/lockdefs.h"
#include "storage/procsignal.h"
#include "storage/relfilenode.h"
--
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
