Re: [HACKERS] -DDISABLE_ENABLE_ASSERT
Andres Freund writes: > Hm, that missed a couple things. Updated patch attached. Besides adding > the missed documentation adjustments this also removes the -A > commandline/startup packet parameter since it doesn't serve a purpose > anymore. > Does anyone think we should rather keep -A and have it not to anything? > It seems fairly unlikely, but not impossible, that someone had the > great idea to add -A off in their connection options. I think we could delete it. If anyone is using that, I think they'd be happier getting an error than having it silently not do what they want (and more slowly than before ...) 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] -DDISABLE_ENABLE_ASSERT
On 2014-06-19 19:31:28 +0200, Andres Freund wrote: > On 2014-05-23 10:01:37 -0400, Tom Lane wrote: > > Andres Freund writes: > > > The next question is whether to wait till after the branching with this? > > > > +1 for waiting (it's only a couple weeks anyway). This isn't a > > user-facing feature in any way, so I feel no urgency to ship it in 9.4. > > Here's my patch for this that I plan to apply later. Hm, that missed a couple things. Updated patch attached. Besides adding the missed documentation adjustments this also removes the -A commandline/startup packet parameter since it doesn't serve a purpose anymore. Does anyone think we should rather keep -A and have it not to anything? It seems fairly unlikely, but not impossible, that someone had the great idea to add -A off in their connection options. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services >From dd658b4a7e4aa7fdf43d659286dc1e21822ef5c6 Mon Sep 17 00:00:00 2001 From: Andres Freund Date: Thu, 19 Jun 2014 19:16:49 +0200 Subject: [PATCH] Don't allow to disable backend assertions via the debug_assertions GUC. The existance of the assert_enabled variable (backing the debug_assertions GUC) reduced the amount of knowledge some static code checkers (like coverity and various compilers) could infer from the existance of the assertion. That could have been solved by optionally removing the assertion_enabled variable from the Assert() et al macros at compile time when some special macro is defined, but the resulting complication doesn't seem to be worth the gain from having debug_assertions. Recompiling is fast enough. The debug_assertions GUC is still available, but readonly, as it's useful when diagnosing problems. The commandline/client startup option -A, which previously also allowed to enable/disable assertions, has been removed as it doesn't serve a purpose anymore. While at it, reduce code duplication in bufmgr.c and localbuf.c assertions checking for spurious buffer pins. That code had to be reindented anyway to cope with the assert_enabled removal. --- doc/src/sgml/config.sgml | 46 +++- src/backend/access/gin/ginpostinglist.c | 1 - src/backend/commands/event_trigger.c | 1 - src/backend/postmaster/postmaster.c | 6 +--- src/backend/storage/buffer/bufmgr.c | 62 ++-- src/backend/storage/buffer/localbuf.c| 51 -- src/backend/storage/lmgr/proc.c | 3 -- src/backend/tcop/postgres.c | 6 +--- src/backend/utils/cache/catcache.c | 51 +- src/backend/utils/cache/relfilenodemap.c | 1 - src/backend/utils/misc/guc.c | 30 src/include/c.h | 4 +-- src/include/postgres.h | 9 ++--- 13 files changed, 106 insertions(+), 165 deletions(-) diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 697cf99..8f0ca4c 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -6722,6 +6722,26 @@ dynamic_library_path = 'C:\tools\postgresql;H:\my_project\lib;$libdir' + + debug_assertions (boolean) + + debug_assertions configuration parameter + + + + +Reports whether PostgreSQL has been built +with assertions enabled. That is the case if the +macro USE_ASSERT_CHECKING is defined +when PostgreSQL is built (accomplished +e.g. by the configure option +--enable-cassert). By +default PostgreSQL is built without +assertions. + + + + integer_datetimes (boolean) @@ -6973,28 +6993,6 @@ dynamic_library_path = 'C:\tools\postgresql;H:\my_project\lib;$libdir' - - debug_assertions (boolean) - - debug_assertions configuration parameter - - - - -Turns on various assertion checks. This is a debugging aid. If -you are experiencing strange problems or crashes you might want -to turn this on, as it might expose programming mistakes. To use -this parameter, the macro USE_ASSERT_CHECKING -must be defined when PostgreSQL is -built (accomplished by the configure option ---enable-cassert). Note that -debug_assertions defaults to on -if PostgreSQL has been built with -assertions enabled. - - - - ignore_system_indexes (boolean) @@ -7355,10 +7353,6 @@ LOG: CleanUpLock: deleting: lock(0xb7acd844) id(24688,24696,0,0,0,1) --A x -debug_assertions = x - - -B x shared_buffers = x diff --git a/src/backend/access/gin/ginpostinglist.c b/src/backend/access/gin/ginpostinglist.c index 606a824..ea59dea 100644 --- a/
Re: [HACKERS] -DDISABLE_ENABLE_ASSERT
On 2014-05-23 10:01:37 -0400, Tom Lane wrote:
> Andres Freund writes:
> > The next question is whether to wait till after the branching with this?
>
> +1 for waiting (it's only a couple weeks anyway). This isn't a
> user-facing feature in any way, so I feel no urgency to ship it in 9.4.
Here's my patch for this that I plan to apply later.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
>From 2a1e793479bbc038aef0580a339fef38518ad17f Mon Sep 17 00:00:00 2001
From: Andres Freund
Date: Thu, 19 Jun 2014 19:16:49 +0200
Subject: [PATCH] Don't allow to disable backend assertions via assert_enabled
GUC anymore.
The existance of the assert_enabled variable reduced the amount of
knowledge some static code checkers (like coverity and various
compilers) could infer from the existance of the assertion. That could
have been solved by optionally removing the assertion_enabled at
compile time, but the resulting complication doesn't seem to be worth
it. Recompiling is fast enough.
While at it, reduce code duplication in bufmgr.c and localbuf.c in
assertions checking for spurious buffer pins.
---
src/backend/access/gin/ginpostinglist.c | 1 -
src/backend/commands/event_trigger.c | 1 -
src/backend/storage/buffer/bufmgr.c | 62 ++--
src/backend/storage/buffer/localbuf.c| 51 --
src/backend/storage/lmgr/proc.c | 3 --
src/backend/utils/cache/catcache.c | 51 +-
src/backend/utils/cache/relfilenodemap.c | 1 -
src/backend/utils/misc/guc.c | 30
src/include/c.h | 4 +--
src/include/postgres.h | 9 ++---
10 files changed, 84 insertions(+), 129 deletions(-)
diff --git a/src/backend/access/gin/ginpostinglist.c b/src/backend/access/gin/ginpostinglist.c
index 606a824..ea59dea 100644
--- a/src/backend/access/gin/ginpostinglist.c
+++ b/src/backend/access/gin/ginpostinglist.c
@@ -250,7 +250,6 @@ ginCompressPostingList(const ItemPointer ipd, int nipd, int maxsize,
* Check that the encoded segment decodes back to the original items.
*/
#if defined (CHECK_ENCODING_ROUNDTRIP)
- if (assert_enabled)
{
int ndecoded;
ItemPointer tmp = ginPostingListDecode(result, &ndecoded);
diff --git a/src/backend/commands/event_trigger.c b/src/backend/commands/event_trigger.c
index 6d4e091..110fe00 100644
--- a/src/backend/commands/event_trigger.c
+++ b/src/backend/commands/event_trigger.c
@@ -635,7 +635,6 @@ EventTriggerCommonSetup(Node *parsetree,
* relevant command tag.
*/
#ifdef USE_ASSERT_CHECKING
- if (assert_enabled)
{
const char *dbgtag;
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index c070278..07ea665 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -109,6 +109,7 @@ static volatile BufferDesc *BufferAlloc(SMgrRelation smgr,
bool *foundPtr);
static void FlushBuffer(volatile BufferDesc *buf, SMgrRelation reln);
static void AtProcExit_Buffers(int code, Datum arg);
+static void CheckForBufferLeaks(void);
static int rnode_comparator(const void *p1, const void *p2);
@@ -1699,34 +1700,13 @@ SyncOneBuffer(int buf_id, bool skip_recently_used)
return result | BUF_WRITTEN;
}
-
/*
* AtEOXact_Buffers - clean up at end of transaction.
- *
- * As of PostgreSQL 8.0, buffer pins should get released by the
- * ResourceOwner mechanism. This routine is just a debugging
- * cross-check that no pins remain.
*/
void
AtEOXact_Buffers(bool isCommit)
{
-#ifdef USE_ASSERT_CHECKING
- if (assert_enabled)
- {
- int RefCountErrors = 0;
- Buffer b;
-
- for (b = 1; b <= NBuffers; b++)
- {
- if (PrivateRefCount[b - 1] != 0)
- {
-PrintBufferLeakWarning(b);
-RefCountErrors++;
- }
- }
- Assert(RefCountErrors == 0);
- }
-#endif
+ CheckForBufferLeaks();
AtEOXact_LocalBuffers(isCommit);
}
@@ -1756,26 +1736,36 @@ AtProcExit_Buffers(int code, Datum arg)
AbortBufferIO();
UnlockBuffers();
+ CheckForBufferLeaks();
+
+ /* localbuf.c needs a chance too */
+ AtProcExit_LocalBuffers();
+}
+
+/*
+ * CheckForBufferLeaks - ensure this backend holds no buffer pins
+ *
+ * As of PostgreSQL 8.0, buffer pins should get released by the
+ * ResourceOwner mechanism. This routine is just a debugging
+ * cross-check that no pins remain.
+ */
+static void
+CheckForBufferLeaks(void)
+{
#ifdef USE_ASSERT_CHECKING
- if (assert_enabled)
- {
- int RefCountErrors = 0;
- Buffer b;
+ int RefCountErrors = 0;
+ Buffer b;
- for (b = 1; b <= NBuffers; b++)
+ for (b = 1; b <= NBuffers; b++)
+ {
+ if (PrivateRefCount[b - 1] != 0)
{
- if (PrivateRefCount[b - 1] != 0)
- {
-PrintBufferLeakWarning(b);
-RefCountErrors++;
- }
+ PrintBufferLeakWarning(b);
+ RefCountErrors++;
}
- Assert(RefCountErrors == 0);
}
+ Ass
Re: [HACKERS] -DDISABLE_ENABLE_ASSERT
Andres Freund writes: > The next question is whether to wait till after the branching with this? +1 for waiting (it's only a couple weeks anyway). This isn't a user-facing feature in any way, so I feel no urgency to ship it in 9.4. 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] -DDISABLE_ENABLE_ASSERT
On 2014-05-23 09:56:03 -0400, Tom Lane wrote: > Robert Haas writes: > > On Fri, May 23, 2014 at 8:15 AM, Andres Freund > > wrote: > >> That means you're for a (differently named) disable macro? Or is it not > >> recent enough that you don't care? > > > I'm leaning toward thinking we should just rip it out. The fact that > > 3 out of the 4 people commenting on this thread have used it at some > > point provides some evidence that it has more than no value - but on > > the other hand, there's a cost to keeping it around. > > Yeah. For the record, I've used it too (don't recall what for exactly). > But I don't think it's worth adding yet another layer of complication for. Cool. Seems like we have an agreement then. The next question is whether to wait till after the branching with this? 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] -DDISABLE_ENABLE_ASSERT
Robert Haas writes: > On Fri, May 23, 2014 at 8:15 AM, Andres Freund wrote: >> That means you're for a (differently named) disable macro? Or is it not >> recent enough that you don't care? > I'm leaning toward thinking we should just rip it out. The fact that > 3 out of the 4 people commenting on this thread have used it at some > point provides some evidence that it has more than no value - but on > the other hand, there's a cost to keeping it around. Yeah. For the record, I've used it too (don't recall what for exactly). But I don't think it's worth adding yet another layer of complication for. The main argument for it given in this thread is recompile cost ... but TBH, I have one word for anybody who's worried about that, and that word is "ccache". If you don't have that tool installed, you're missing out on a huge timesaver. 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] -DDISABLE_ENABLE_ASSERT
On Fri, May 23, 2014 at 8:15 AM, Andres Freund wrote: >> >> I've used it once or twice to avoid having to recompile postgres when I >> >> wanted things not to be *that* slow (AtEOXactBuffers() I am looking at >> >> you). But I wouldn't be very sad if it'd go. >> >> >> >> Anybody against that? >> > >> > I have used it too (for a different reason IIRC), but like you I >> > wouldn't have a problem if it weren't there. >> >> I've used it, too, although not recently. > > That means you're for a (differently named) disable macro? Or is it not > recent enough that you don't care? I'm leaning toward thinking we should just rip it out. The fact that 3 out of the 4 people commenting on this thread have used it at some point provides some evidence that it has more than no value - but on the other hand, there's a cost to keeping it around. The need to guard some sections of code by both #ifdef USE_ASSERT_CHECKING and if (assert_enabled) has occasionally been a source of confusion over the years, and once I had a customer who I was afraid had gotten an assert-enabled build into production and the only way to distinguish between an assert-enabled build with assertions shut off via the GUC and a build that didn't support them in the first place was to try turning the GUC on and see whether it worked, which led to some confusion. Now it looks like we need to add another macro to make this dance even more complicated ... and I'm starting to think it's just not worth it. -- 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
Re: [HACKERS] -DDISABLE_ENABLE_ASSERT
On 2014-05-23 07:20:12 -0400, Robert Haas wrote: > On Thu, May 22, 2014 at 5:00 PM, Alvaro Herrera > wrote: > > Andres Freund wrote: > >> On 2014-05-22 16:37:35 -0400, Tom Lane wrote: > >> > We could do that ... but I wonder if we shouldn't remove assert_enabled > >> > altogether. What's the use case for turning it off? Not matching the > >> > speed of a non-cassert build, because for instance > >> > MEMORY_CONTEXT_CHECKING > >> > doesn't get turned off. > >> > >> I've used it once or twice to avoid having to recompile postgres when I > >> wanted things not to be *that* slow (AtEOXactBuffers() I am looking at > >> you). But I wouldn't be very sad if it'd go. > >> > >> Anybody against that? > > > > I have used it too (for a different reason IIRC), but like you I > > wouldn't have a problem if it weren't there. > > I've used it, too, although not recently. That means you're for a (differently named) disable macro? Or is it not recent enough that you don't care? 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] -DDISABLE_ENABLE_ASSERT
On Thu, May 22, 2014 at 5:00 PM, Alvaro Herrera wrote: > Andres Freund wrote: >> On 2014-05-22 16:37:35 -0400, Tom Lane wrote: >> > We could do that ... but I wonder if we shouldn't remove assert_enabled >> > altogether. What's the use case for turning it off? Not matching the >> > speed of a non-cassert build, because for instance MEMORY_CONTEXT_CHECKING >> > doesn't get turned off. >> >> I've used it once or twice to avoid having to recompile postgres when I >> wanted things not to be *that* slow (AtEOXactBuffers() I am looking at >> you). But I wouldn't be very sad if it'd go. >> >> Anybody against that? > > I have used it too (for a different reason IIRC), but like you I > wouldn't have a problem if it weren't there. I've used it, too, although not recently. -- 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
Re: [HACKERS] -DDISABLE_ENABLE_ASSERT
Andres Freund wrote: > On 2014-05-22 16:37:35 -0400, Tom Lane wrote: > > We could do that ... but I wonder if we shouldn't remove assert_enabled > > altogether. What's the use case for turning it off? Not matching the > > speed of a non-cassert build, because for instance MEMORY_CONTEXT_CHECKING > > doesn't get turned off. > > I've used it once or twice to avoid having to recompile postgres when I > wanted things not to be *that* slow (AtEOXactBuffers() I am looking at > you). But I wouldn't be very sad if it'd go. > > Anybody against that? I have used it too (for a different reason IIRC), but like you I wouldn't have a problem if it weren't there. -- Á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] -DDISABLE_ENABLE_ASSERT
On 2014-05-22 16:37:35 -0400, Tom Lane wrote: > Andres Freund writes: > > Since there seem to be multiple static checkers (coverity, clang > > checker) having problems with assert_enabled can we just optionally > > disable it? > > I am thinking of replacing it by a AssertionsEnabled() macro which then > > is unconditionally defined when DISABLE_ENABLE_ASSERT is defined. > > We could do that ... but I wonder if we shouldn't remove assert_enabled > altogether. What's the use case for turning it off? Not matching the > speed of a non-cassert build, because for instance MEMORY_CONTEXT_CHECKING > doesn't get turned off. I've used it once or twice to avoid having to recompile postgres when I wanted things not to be *that* slow (AtEOXactBuffers() I am looking at you). But I wouldn't be very sad if it'd go. Anybody against that? > If we went this direction I'd suggest keeping the GUC but turning it into > a read-only report of whether the backend was compiled with assertions. Yes, that makes sense. 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] -DDISABLE_ENABLE_ASSERT
On Thu, May 22, 2014 at 4:31 PM, Andres Freund wrote: > Since there seem to be multiple static checkers (coverity, clang > checker) having problems with assert_enabled can we just optionally > disable it? > I am thinking of replacing it by a AssertionsEnabled() macro which then > is unconditionally defined when DISABLE_ENABLE_ASSERT is defined. Uh, probably. But I think you need a better name; specifically, one that doesn't contain two words which are antonyms. -- 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
Re: [HACKERS] -DDISABLE_ENABLE_ASSERT
Andres Freund writes: > Since there seem to be multiple static checkers (coverity, clang > checker) having problems with assert_enabled can we just optionally > disable it? > I am thinking of replacing it by a AssertionsEnabled() macro which then > is unconditionally defined when DISABLE_ENABLE_ASSERT is defined. We could do that ... but I wonder if we shouldn't remove assert_enabled altogether. What's the use case for turning it off? Not matching the speed of a non-cassert build, because for instance MEMORY_CONTEXT_CHECKING doesn't get turned off. If we went this direction I'd suggest keeping the GUC but turning it into a read-only report of whether the backend was compiled with assertions. 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
