Re: [HACKERS] -DDISABLE_ENABLE_ASSERT

2014-06-19 Thread Andres Freund
On 2014-05-23 10:01:37 -0400, Tom Lane wrote:
 Andres Freund and...@2ndquadrant.com 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 and...@anarazel.de
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++;
 		}
-		

Re: [HACKERS] -DDISABLE_ENABLE_ASSERT

2014-06-19 Thread Andres Freund
On 2014-06-19 19:31:28 +0200, Andres Freund wrote:
 On 2014-05-23 10:01:37 -0400, Tom Lane wrote:
  Andres Freund and...@2ndquadrant.com 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 and...@anarazel.de
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'
   /listitem
  /varlistentry
 
+ varlistentry id=guc-debug-assertions xreflabel=debug_assertions
+  termvarnamedebug_assertions/varname (typeboolean/type)
+  indexterm
+   primaryvarnamedebug_assertions/ configuration parameter/primary
+  /indexterm
+  /term
+  listitem
+   para
+Reports whether productnamePostgreSQL/productname has been built
+with assertions enabled. That is the case if the
+macro symbolUSE_ASSERT_CHECKING/symbol is defined
+when productnamePostgreSQL/productname is built (accomplished
+e.g. by the commandconfigure/command option
+option--enable-cassert/option). By
+default productnamePostgreSQL/productname is built without
+assertions.
+   /para
+  /listitem
+ /varlistentry
+
  varlistentry id=guc-integer-datetimes xreflabel=integer_datetimes
   termvarnameinteger_datetimes/varname (typeboolean/type)
   indexterm
@@ -6973,28 +6993,6 @@ dynamic_library_path = 'C:\tools\postgresql;H:\my_project\lib;$libdir'
   /listitem
  /varlistentry
 
- varlistentry id=guc-debug-assertions xreflabel=debug_assertions
-  termvarnamedebug_assertions/varname (typeboolean/type)
-  indexterm
-   primaryvarnamedebug_assertions/ configuration parameter/primary
-  /indexterm
-  /term
-  listitem
-   para
-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 symbolUSE_ASSERT_CHECKING/symbol
-must be defined when productnamePostgreSQL/productname 

Re: [HACKERS] -DDISABLE_ENABLE_ASSERT

2014-06-19 Thread Tom Lane
Andres Freund and...@2ndquadrant.com 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

2014-05-23 Thread Robert Haas
On Thu, May 22, 2014 at 5:00 PM, Alvaro Herrera
alvhe...@2ndquadrant.com 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

2014-05-23 Thread Andres Freund
On 2014-05-23 07:20:12 -0400, Robert Haas wrote:
 On Thu, May 22, 2014 at 5:00 PM, Alvaro Herrera
 alvhe...@2ndquadrant.com 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

2014-05-23 Thread Robert Haas
On Fri, May 23, 2014 at 8:15 AM, Andres Freund and...@2ndquadrant.com 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

2014-05-23 Thread Tom Lane
Robert Haas robertmh...@gmail.com writes:
 On Fri, May 23, 2014 at 8:15 AM, Andres Freund and...@2ndquadrant.com 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

2014-05-23 Thread Andres Freund
On 2014-05-23 09:56:03 -0400, Tom Lane wrote:
 Robert Haas robertmh...@gmail.com writes:
  On Fri, May 23, 2014 at 8:15 AM, Andres Freund and...@2ndquadrant.com 
  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

2014-05-23 Thread Tom Lane
Andres Freund and...@2ndquadrant.com 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

2014-05-22 Thread Robert Haas
On Thu, May 22, 2014 at 4:31 PM, Andres Freund and...@2ndquadrant.com 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

2014-05-22 Thread Tom Lane
Andres Freund and...@2ndquadrant.com 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


Re: [HACKERS] -DDISABLE_ENABLE_ASSERT

2014-05-22 Thread Andres Freund
On 2014-05-22 16:37:35 -0400, Tom Lane wrote:
 Andres Freund and...@2ndquadrant.com 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

2014-05-22 Thread Alvaro Herrera
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