Re: [HACKERS] Re: [PATCH 1/5] Centralize Assert* macros into c.h so its common between backend/frontend
Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: On 2013-01-18 10:33:16 -0500, Tom Lane wrote: Really I'd prefer not to move the backend definitions out of postgres.h at all, just because doing so will lose fifteen years of git history about those particular lines (or at least make it a lot harder to locate with git blame). The alternative seems to be sprinkling a few more #ifdef FRONTEND's in postgres.h, if thats preferred I can prepare a patch for that although I prefer my proposal. Yeah, surrounding the existing definitions with #ifndef FRONTEND was what I was imagining. But on reflection that seems pretty darn ugly, especially if the corresponding FRONTEND definitions are far away. Maybe we should just live with the git history disconnect. Right at the moment the c.h location seems like the best bet. Since there seems to be consensus that this is the best way to go, I have committed it. -- Álvaro Herrerahttp://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
Re: [HACKERS] Re: [PATCH 1/5] Centralize Assert* macros into c.h so its common between backend/frontend
On 2013-01-08 15:55:24 -0500, Tom Lane wrote: Alvaro Herrera alvhe...@2ndquadrant.com writes: Andres Freund wrote: Sorry, misremembered the problem somewhat. The problem is that code that includes postgres.h atm ends up with ExceptionalCondition() et al. declared even if FRONTEND is defined. So if anything uses an assert you need to provide wrappers for those which seems nasty. If they are provided centrally and check for FRONTEND that problem doesn't exist. I think the right fix here is to fix things so that postgres.h is not necessary. How hard is that? Maybe it just requires some more reshuffling of xlog headers. That would definitely be the ideal fix. In general, #include'ing postgres.h into code that's not backend code opens all kinds of hazards that are likely to bite us sooner or later; the incompatibility of the Assert definitions is just the tip of that iceberg. (In the past we've had issues around stdio.h providing different definitions in the two environments, for example.) I agree that going in that direction is a good thing, but imo that doesn't really has any bearing on whether this patch is a good/bad idea... But having said that, I'm also now remembering that we have files in src/port/ and probably elsewhere that try to work in both environments by just #include'ing c.h directly (relying on the Makefile to supply -DFRONTEND or not). If we wanted to support Assert use in such files, we would have to move at least the Assert() macro definitions into c.h as Andres is proposing. Now, I've always thought that #include'ing c.h directly was kind of an ugly hack, but I don't have a better design than that to offer ATM. Imo having some common dumping ground for things that concern both environments is a good idea. I don't think c.h would be the best place for it when redesigning from ground up, but were not doing that so imo its acceptable as long as we take care not to let it grow too much. Here's a trivially updated patch which also defines AssertArg() for FRONTEND-ish environments since Alvaro added one in xlogreader.c. Do we agree this is the way forward, at least for now? Greetings, Andres Freund -- Andres Freund 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
Re: [HACKERS] Re: [PATCH 1/5] Centralize Assert* macros into c.h so its common between backend/frontend
On 2013-01-18 15:48:01 +0100, Andres Freund wrote:
Here's a trivially updated patch which also defines AssertArg() for
FRONTEND-ish environments since Alvaro added one in xlogreader.c.
This time for real. Please.
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training Services
From 0190dd4d9a6d8e638727eaee71cb1390e6bbe88e Mon Sep 17 00:00:00 2001
From: Andres Freund and...@anarazel.de
Date: Tue, 8 Jan 2013 17:59:10 +0100
Subject: [PATCH] Centralize Assert* macros into c.h so its common between
backend/frontend
c.h already had parts of the assert support (StaticAssert*) and its the shared
file between postgres.h and postgres_fe.h. This makes it easier to build
frontend programs which have to do the hack.
---
src/include/c.h | 67 +++
src/include/postgres.h| 54 ++
src/include/postgres_fe.h | 12 -
3 files changed, 69 insertions(+), 64 deletions(-)
diff --git a/src/include/c.h b/src/include/c.h
index 59af5b5..e2aefa9 100644
--- a/src/include/c.h
+++ b/src/include/c.h
@@ -694,6 +694,73 @@ typedef NameData *Name;
/*
+ * USE_ASSERT_CHECKING, if defined, turns on all the assertions.
+ * - plai 9/5/90
+ *
+ * It should _NOT_ be defined in releases or in benchmark copies
+ */
+
+/*
+ * Assert() can be used in both frontend and backend code. In frontend code it
+ * just calls the standard assert, if it's available. If use of assertions is
+ * not configured, it does nothing.
+ */
+#ifndef USE_ASSERT_CHECKING
+
+#define Assert(condition)
+#define AssertMacro(condition) ((void)true)
+#define AssertArg(condition)
+#define AssertState(condition)
+
+#elif defined FRONTEND
+
+#include assert.h
+#define Assert(p) assert(p)
+#define AssertMacro(p) ((void) assert(p))
+#define AssertArg(condition) assert(condition)
+#define AssertState(condition) assert(condition)
+
+#else /* USE_ASSERT_CHECKING FRONTEND */
+
+/*
+ * Trap
+ * Generates an exception if the given condition is true.
+ */
+#define Trap(condition, errorType) \
+ do { \
+ if ((assert_enabled) (condition)) \
+ ExceptionalCondition(CppAsString(condition), (errorType), \
+ __FILE__, __LINE__); \
+ } while (0)
+
+/*
+ * TrapMacro is the same as Trap but it's intended for use in macros:
+ *
+ * #define foo(x) (AssertMacro(x != 0), bar(x))
+ *
+ * Isn't CPP fun?
+ */
+#define TrapMacro(condition, errorType) \
+ ((bool) ((! assert_enabled) || ! (condition) || \
+ (ExceptionalCondition(CppAsString(condition), (errorType), \
+ __FILE__, __LINE__), 0)))
+
+#define Assert(condition) \
+ Trap(!(condition), FailedAssertion)
+
+#define AssertMacro(condition) \
+ ((void) TrapMacro(!(condition), FailedAssertion))
+
+#define AssertArg(condition) \
+ Trap(!(condition), BadArgument)
+
+#define AssertState(condition) \
+ Trap(!(condition), BadState)
+
+#endif /* USE_ASSERT_CHECKING !FRONTEND */
+
+
+/*
* Macros to support compile-time assertion checks.
*
* If the condition (a compile-time-constant expression) evaluates to false,
diff --git a/src/include/postgres.h b/src/include/postgres.h
index b6e922f..bbe125a 100644
--- a/src/include/postgres.h
+++ b/src/include/postgres.h
@@ -25,7 +25,7 @@
* ---
* 1) variable-length datatypes (TOAST support)
* 2) datum type + support macros
- * 3) exception handling definitions
+ * 3) exception handling
*
* NOTES
*
@@ -627,62 +627,12 @@ extern Datum Float8GetDatum(float8 X);
/*
- *Section 3: exception handling definitions
- * Assert, Trap, etc macros
+ *Section 3: exception handling backend support
*
*/
extern PGDLLIMPORT bool assert_enabled;
-/*
- * USE_ASSERT_CHECKING, if defined, turns on all the assertions.
- * - plai 9/5/90
- *
- * It should _NOT_ be defined in releases or in benchmark copies
- */
-
-/*
- * Trap
- * Generates an exception if the given condition is true.
- */
-#define Trap(condition, errorType) \
- do { \
- if ((assert_enabled) (condition)) \
- ExceptionalCondition(CppAsString(condition), (errorType), \
- __FILE__, __LINE__); \
- } while (0)
-
-/*
- * TrapMacro is the same as Trap but it's intended for use in macros:
- *
- * #define foo(x) (AssertMacro(x != 0), bar(x))
- *
- * Isn't CPP fun?
- */
-#define TrapMacro(condition, errorType) \
- ((bool) ((! assert_enabled) || ! (condition) || \
- (ExceptionalCondition(CppAsString(condition), (errorType), \
- __FILE__, __LINE__), 0)))
-
-#ifndef USE_ASSERT_CHECKING
-#define Assert(condition)
-#define AssertMacro(condition) ((void)true)
-#define AssertArg(condition)
-#define AssertState(condition)
-#else
-#define Assert(condition) \
- Trap(!(condition), FailedAssertion)
-
-#define AssertMacro(condition) \
-
Re: [HACKERS] Re: [PATCH 1/5] Centralize Assert* macros into c.h so its common between backend/frontend
Andres Freund wrote: On 2013-01-18 15:48:01 +0100, Andres Freund wrote: Here's a trivially updated patch which also defines AssertArg() for FRONTEND-ish environments since Alvaro added one in xlogreader.c. This time for real. Please. Here's a different idea: move all the Assert() and StaticAssert() etc definitions from c.h and postgres.h into a new header, say pgassert.h. That header is included directly by postgres.h (just like palloc.h and elog.h already are) so we don't have to touch the backend code at all. Frontend programs that want that functionality can just #include pgassert.h by themselves. The definitions are (obviously) protected by #ifdef FRONTEND so that it all works in both environments cleanly. -- Álvaro Herrerahttp://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
Re: [HACKERS] Re: [PATCH 1/5] Centralize Assert* macros into c.h so its common between backend/frontend
Alvaro Herrera alvhe...@2ndquadrant.com writes: Here's a different idea: move all the Assert() and StaticAssert() etc definitions from c.h and postgres.h into a new header, say pgassert.h. That header is included directly by postgres.h (just like palloc.h and elog.h already are) so we don't have to touch the backend code at all. Frontend programs that want that functionality can just #include pgassert.h by themselves. The definitions are (obviously) protected by #ifdef FRONTEND so that it all works in both environments cleanly. That might work. Files using c.h would have to include this too, but as described it should work in either environment for them. The other corner case is pg_controldata.c and other frontend programs that include postgres.h --- but it looks like they #define FRONTEND first, so they'd get the correct set of Assert definitions. Whether it's really any better than just sticking them in c.h isn't clear. Really I'd prefer not to move the backend definitions out of postgres.h at all, just because doing so will lose fifteen years of git history about those particular lines (or at least make it a lot harder to locate with git blame). regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [PATCH 1/5] Centralize Assert* macros into c.h so its common between backend/frontend
Alvaro Herrera wrote: Andres Freund wrote: On 2013-01-18 15:48:01 +0100, Andres Freund wrote: Here's a trivially updated patch which also defines AssertArg() for FRONTEND-ish environments since Alvaro added one in xlogreader.c. This time for real. Please. Here's a different idea: move all the Assert() and StaticAssert() etc definitions from c.h and postgres.h into a new header, say pgassert.h. That header is included directly by postgres.h (just like palloc.h and elog.h already are) so we don't have to touch the backend code at all. Frontend programs that want that functionality can just #include pgassert.h by themselves. The definitions are (obviously) protected by #ifdef FRONTEND so that it all works in both environments cleanly. One slight problem with this is the common port/*.c idiom would require an extra include: #ifndef FRONTEND #include postgres.h #else #include postgres_fe.h #include pgassert.h /* --- new line required here */ #endif /* FRONTEND */ If this is seen as a serious issue (I don't think so) then we would have to include pgassert.h into postgres_fe.h which would make the whole thing pointless (i.e. the same as just having the definitions in c.h). -- Álvaro Herrerahttp://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
Re: [HACKERS] Re: [PATCH 1/5] Centralize Assert* macros into c.h so its common between backend/frontend
Alvaro Herrera alvhe...@2ndquadrant.com writes: One slight problem with this is the common port/*.c idiom would require an extra include: #ifndef FRONTEND #include postgres.h #else #include postgres_fe.h #include pgassert.h /* --- new line required here */ #endif /* FRONTEND */ If this is seen as a serious issue (I don't think so) then we would have to include pgassert.h into postgres_fe.h which would make the whole thing pointless (i.e. the same as just having the definitions in c.h). We'd only need to add that when/if the file started to use asserts, so I don't think it's a problem. But still not sure it's better than just putting the stuff in c.h. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [PATCH 1/5] Centralize Assert* macros into c.h so its common between backend/frontend
On 2013-01-18 10:33:16 -0500, Tom Lane wrote: Alvaro Herrera alvhe...@2ndquadrant.com writes: Here's a different idea: move all the Assert() and StaticAssert() etc definitions from c.h and postgres.h into a new header, say pgassert.h. That header is included directly by postgres.h (just like palloc.h and elog.h already are) so we don't have to touch the backend code at all. Frontend programs that want that functionality can just #include pgassert.h by themselves. The definitions are (obviously) protected by #ifdef FRONTEND so that it all works in both environments cleanly. That might work. Files using c.h would have to include this too, but as described it should work in either environment for them. The other corner case is pg_controldata.c and other frontend programs that include postgres.h --- but it looks like they #define FRONTEND first, so they'd get the correct set of Assert definitions. Whether it's really any better than just sticking them in c.h isn't clear. I don't really see an advantage. To me providing sensible asserts sounds like a good fit for c.h... And we already have StaticAssert*, AssertVariableIsOfType* in c.h... Really I'd prefer not to move the backend definitions out of postgres.h at all, just because doing so will lose fifteen years of git history about those particular lines (or at least make it a lot harder to locate with git blame). I can imagine some ugly macro solutions to that problem (pgassert.h includes postgres.h with special defines), but that doesn't seem to be warranted to me. The alternative seems to be sprinkling a few more #ifdef FRONTEND's in postgres.h, if thats preferred I can prepare a patch for that although I prefer my proposal. Greetings, Andres Freund -- Andres Freund 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
Re: [HACKERS] Re: [PATCH 1/5] Centralize Assert* macros into c.h so its common between backend/frontend
Andres Freund and...@2ndquadrant.com writes: On 2013-01-18 10:33:16 -0500, Tom Lane wrote: Really I'd prefer not to move the backend definitions out of postgres.h at all, just because doing so will lose fifteen years of git history about those particular lines (or at least make it a lot harder to locate with git blame). The alternative seems to be sprinkling a few more #ifdef FRONTEND's in postgres.h, if thats preferred I can prepare a patch for that although I prefer my proposal. Yeah, surrounding the existing definitions with #ifndef FRONTEND was what I was imagining. But on reflection that seems pretty darn ugly, especially if the corresponding FRONTEND definitions are far away. Maybe we should just live with the git history disconnect. Right at the moment the c.h location seems like the best bet. I don't see much advantage to Alvaro's proposal of a new header file. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [PATCH 1/5] Centralize Assert* macros into c.h so its common between backend/frontend
Andres Freund wrote: On 2013-01-08 14:35:12 -0500, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: On 2013-01-08 14:25:06 -0500, Tom Lane wrote: This patch seems unnecessary given that we already put a version of Assert() into postgres_fe.h. The problem is that some (including existing) pieces of code need to include postgres.h itself, those can't easily include postgres_fe.h as well without getting into problems with redefinitions. There is no place, anywhere, that should be including both. So I don't see the problem. Sorry, misremembered the problem somewhat. The problem is that code that includes postgres.h atm ends up with ExceptionalCondition() et al. declared even if FRONTEND is defined. So if anything uses an assert you need to provide wrappers for those which seems nasty. If they are provided centrally and check for FRONTEND that problem doesn't exist. I think the right fix here is to fix things so that postgres.h is not necessary. How hard is that? Maybe it just requires some more reshuffling of xlog headers. -- Álvaro Herrerahttp://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
Re: [HACKERS] Re: [PATCH 1/5] Centralize Assert* macros into c.h so its common between backend/frontend
On 2013-01-08 17:36:19 -0300, Alvaro Herrera wrote: Andres Freund wrote: On 2013-01-08 14:35:12 -0500, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: On 2013-01-08 14:25:06 -0500, Tom Lane wrote: This patch seems unnecessary given that we already put a version of Assert() into postgres_fe.h. The problem is that some (including existing) pieces of code need to include postgres.h itself, those can't easily include postgres_fe.h as well without getting into problems with redefinitions. There is no place, anywhere, that should be including both. So I don't see the problem. Sorry, misremembered the problem somewhat. The problem is that code that includes postgres.h atm ends up with ExceptionalCondition() et al. declared even if FRONTEND is defined. So if anything uses an assert you need to provide wrappers for those which seems nasty. If they are provided centrally and check for FRONTEND that problem doesn't exist. I think the right fix here is to fix things so that postgres.h is not necessary. How hard is that? Maybe it just requires some more reshuffling of xlog headers. I don't really think thats realistic. Think the rmgrdesc/* files and xlogreader.c itself... Greetings, Andres Freund -- Andres Freund 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
Re: [HACKERS] Re: [PATCH 1/5] Centralize Assert* macros into c.h so its common between backend/frontend
Alvaro Herrera alvhe...@2ndquadrant.com writes: Andres Freund wrote: Sorry, misremembered the problem somewhat. The problem is that code that includes postgres.h atm ends up with ExceptionalCondition() et al. declared even if FRONTEND is defined. So if anything uses an assert you need to provide wrappers for those which seems nasty. If they are provided centrally and check for FRONTEND that problem doesn't exist. I think the right fix here is to fix things so that postgres.h is not necessary. How hard is that? Maybe it just requires some more reshuffling of xlog headers. That would definitely be the ideal fix. In general, #include'ing postgres.h into code that's not backend code opens all kinds of hazards that are likely to bite us sooner or later; the incompatibility of the Assert definitions is just the tip of that iceberg. (In the past we've had issues around stdio.h providing different definitions in the two environments, for example.) But having said that, I'm also now remembering that we have files in src/port/ and probably elsewhere that try to work in both environments by just #include'ing c.h directly (relying on the Makefile to supply -DFRONTEND or not). If we wanted to support Assert use in such files, we would have to move at least the Assert() macro definitions into c.h as Andres is proposing. Now, I've always thought that #include'ing c.h directly was kind of an ugly hack, but I don't have a better design than that to offer ATM. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
