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++;
}
- Assert(RefCountErrors == 0);
}
+ Assert(RefCountErrors == 0);
#endif
-
- /* localbuf.c needs a chance too */
- AtProcExit_LocalBuffers();
}
/*
diff --git a/src/backend/storage/buffer/localbuf.c b/src/backend/storage/buffer/localbuf.c
index 3135c5c..6c81be4 100644
--- a/src/backend/storage/buffer/localbuf.c
+++ b/src/backend/storage/buffer/localbuf.c
@@ -491,15 +491,15 @@ GetLocalBufferStorage(void)
}
/*
- * AtEOXact_LocalBuffers - clean up at end of transaction.
+ * CheckForLocalBufferLeaks - ensure this backend holds no local buffer pins
*
- * This is just like AtEOXact_Buffers, but for local buffers.
+ * This is just like CheckBufferLeaks(), but for local buffers.
*/
-void
-AtEOXact_LocalBuffers(bool isCommit)
+static void
+CheckForLocalBufferLeaks(void)
{
#ifdef USE_ASSERT_CHECKING
- if (assert_enabled && LocalRefCount)
+ if (LocalRefCount)
{
int RefCountErrors = 0;
int i;
@@ -520,33 +520,28 @@ AtEOXact_LocalBuffers(bool isCommit)
}
/*
+ * AtEOXact_LocalBuffers - clean up at end of transaction.
+ *
+ * This is just like AtEOXact_Buffers, but for local buffers.
+ */
+void
+AtEOXact_LocalBuffers(bool isCommit)
+{
+ CheckForLocalBufferLeaks();
+}
+
+/*
* AtProcExit_LocalBuffers - ensure we have dropped pins during backend exit.
*
- * This is just like AtProcExit_Buffers, but for local buffers. We shouldn't
- * be holding any remaining pins; if we are, and assertions aren't enabled,
- * we'll fail later in DropRelFileNodeBuffers while trying to drop the temp
- * rels.
+ * This is just like AtProcExit_Buffers, but for local buffers.
*/
void
AtProcExit_LocalBuffers(void)
{
-#ifdef USE_ASSERT_CHECKING
- if (assert_enabled && LocalRefCount)
- {
- int RefCountErrors = 0;
- int i;
-
- for (i = 0; i < NLocBuffer; i++)
- {
- if (LocalRefCount[i] != 0)
- {
- Buffer b = -i - 1;
-
- PrintBufferLeakWarning(b);
- RefCountErrors++;
- }
- }
- Assert(RefCountErrors == 0);
- }
-#endif
+ /*
+ * We shouldn't be holding any remaining pins; if we are, and assertions
+ * aren't enabled, we'll fail later in DropRelFileNodeBuffers while trying
+ * to drop the temp rels.
+ */
+ CheckForLocalBufferLeaks();
}
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index 266b0da..dfaf10e 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -376,7 +376,6 @@ InitProcess(void)
MyProc->waitLock = NULL;
MyProc->waitProcLock = NULL;
#ifdef USE_ASSERT_CHECKING
- if (assert_enabled)
{
int i;
@@ -539,7 +538,6 @@ InitAuxiliaryProcess(void)
MyProc->waitLock = NULL;
MyProc->waitProcLock = NULL;
#ifdef USE_ASSERT_CHECKING
- if (assert_enabled)
{
int i;
@@ -782,7 +780,6 @@ ProcKill(int code, Datum arg)
SyncRepCleanupAtProcExit();
#ifdef USE_ASSERT_CHECKING
- if (assert_enabled)
{
int i;
diff --git a/src/backend/utils/cache/catcache.c b/src/backend/utils/cache/catcache.c
index 954b435..4dd6753 100644
--- a/src/backend/utils/cache/catcache.c
+++ b/src/backend/utils/cache/catcache.c
@@ -553,41 +553,38 @@ void
AtEOXact_CatCache(bool isCommit)
{
#ifdef USE_ASSERT_CHECKING
- if (assert_enabled)
+ slist_iter cache_iter;
+
+ slist_foreach(cache_iter, &CacheHdr->ch_caches)
{
- slist_iter cache_iter;
+ CatCache *ccp = slist_container(CatCache, cc_next, cache_iter.cur);
+ dlist_iter iter;
+ int i;
- slist_foreach(cache_iter, &CacheHdr->ch_caches)
+ /* Check CatCLists */
+ dlist_foreach(iter, &ccp->cc_lists)
{
- CatCache *ccp = slist_container(CatCache, cc_next, cache_iter.cur);
- dlist_iter iter;
- int i;
+ CatCList *cl;
- /* Check CatCLists */
- dlist_foreach(iter, &ccp->cc_lists)
- {
- CatCList *cl;
+ cl = dlist_container(CatCList, cache_elem, iter.cur);
+ Assert(cl->cl_magic == CL_MAGIC);
+ Assert(cl->refcount == 0);
+ Assert(!cl->dead);
+ }
- cl = dlist_container(CatCList, cache_elem, iter.cur);
- Assert(cl->cl_magic == CL_MAGIC);
- Assert(cl->refcount == 0);
- Assert(!cl->dead);
- }
+ /* Check individual tuples */
+ for (i = 0; i < ccp->cc_nbuckets; i++)
+ {
+ dlist_head *bucket = &ccp->cc_bucket[i];
- /* Check individual tuples */
- for (i = 0; i < ccp->cc_nbuckets; i++)
+ dlist_foreach(iter, bucket)
{
- dlist_head *bucket = &ccp->cc_bucket[i];
-
- dlist_foreach(iter, bucket)
- {
- CatCTup *ct;
+ CatCTup *ct;
- ct = dlist_container(CatCTup, cache_elem, iter.cur);
- Assert(ct->ct_magic == CT_MAGIC);
- Assert(ct->refcount == 0);
- Assert(!ct->dead);
- }
+ ct = dlist_container(CatCTup, cache_elem, iter.cur);
+ Assert(ct->ct_magic == CT_MAGIC);
+ Assert(ct->refcount == 0);
+ Assert(!ct->dead);
}
}
}
diff --git a/src/backend/utils/cache/relfilenodemap.c b/src/backend/utils/cache/relfilenodemap.c
index 557ff61..1e8429c 100644
--- a/src/backend/utils/cache/relfilenodemap.c
+++ b/src/backend/utils/cache/relfilenodemap.c
@@ -220,7 +220,6 @@ RelidByRelfilenode(Oid reltablespace, Oid relfilenode)
found = true;
#ifdef USE_ASSERT_CHECKING
- if (assert_enabled)
{
bool isnull;
Oid check;
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 8ca065b..6902c23 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -174,7 +174,6 @@ static void assign_syslog_ident(const char *newval, void *extra);
static void assign_session_replication_role(int newval, void *extra);
static bool check_temp_buffers(int *newval, void **extra, GucSource source);
static bool check_phony_autocommit(bool *newval, void **extra, GucSource source);
-static bool check_debug_assertions(bool *newval, void **extra, GucSource source);
static bool check_bonjour(bool *newval, void **extra, GucSource source);
static bool check_ssl(bool *newval, void **extra, GucSource source);
static bool check_stage_log_stats(bool *newval, void **extra, GucSource source);
@@ -413,11 +412,6 @@ extern const struct config_enum_entry dynamic_shared_memory_options[];
/*
* GUC option variables that are exported from this module
*/
-#ifdef USE_ASSERT_CHECKING
-bool assert_enabled = true;
-#else
-bool assert_enabled = false;
-#endif
bool log_duration = false;
bool Debug_print_plan = false;
bool Debug_print_parse = false;
@@ -500,6 +494,7 @@ static bool data_checksums;
static int wal_segment_size;
static bool integer_datetimes;
static int effective_io_concurrency;
+static bool assert_enabled;
/* should be static, but commands/variable.c needs to get at this */
char *role_string;
@@ -931,10 +926,10 @@ static struct config_bool ConfigureNamesBool[] =
NULL, NULL, NULL
},
{
- {"debug_assertions", PGC_USERSET, DEVELOPER_OPTIONS,
- gettext_noop("Turns on various assertion checks."),
- gettext_noop("This is a debugging aid."),
- GUC_NOT_IN_SAMPLE
+ {"debug_assertions", PGC_INTERNAL, PRESET_OPTIONS,
+ gettext_noop("Shows whether the running server has assertion checks enabled."),
+ NULL,
+ GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE
},
&assert_enabled,
#ifdef USE_ASSERT_CHECKING
@@ -942,7 +937,7 @@ static struct config_bool ConfigureNamesBool[] =
#else
false,
#endif
- check_debug_assertions, NULL, NULL
+ NULL, NULL, NULL
},
{
@@ -9118,19 +9113,6 @@ check_phony_autocommit(bool *newval, void **extra, GucSource source)
}
static bool
-check_debug_assertions(bool *newval, void **extra, GucSource source)
-{
-#ifndef USE_ASSERT_CHECKING
- if (*newval)
- {
- GUC_check_errmsg("assertion checking is not supported by this build");
- return false;
- }
-#endif
- return true;
-}
-
-static bool
check_bonjour(bool *newval, void **extra, GucSource source)
{
#ifndef USE_BONJOUR
diff --git a/src/include/c.h b/src/include/c.h
index df22d50..a48a57a 100644
--- a/src/include/c.h
+++ b/src/include/c.h
@@ -597,7 +597,7 @@ typedef NameData *Name;
*/
#define Trap(condition, errorType) \
do { \
- if ((assert_enabled) && (condition)) \
+ if (condition) \
ExceptionalCondition(CppAsString(condition), (errorType), \
__FILE__, __LINE__); \
} while (0)
@@ -610,7 +610,7 @@ typedef NameData *Name;
* Isn't CPP fun?
*/
#define TrapMacro(condition, errorType) \
- ((bool) ((! assert_enabled) || ! (condition) || \
+ ((bool) (! (condition) || \
(ExceptionalCondition(CppAsString(condition), (errorType), \
__FILE__, __LINE__), 0)))
diff --git a/src/include/postgres.h b/src/include/postgres.h
index 00fbaaf9..b123813 100644
--- a/src/include/postgres.h
+++ b/src/include/postgres.h
@@ -680,13 +680,10 @@ extern Datum Float8GetDatum(float8 X);
*/
/*
- * These declarations supports the assertion-related macros in c.h.
- * assert_enabled is here because that file doesn't have PGDLLIMPORT in the
- * right place, and ExceptionalCondition must be present, for the backend only,
- * even when assertions are not enabled.
+ * Backend only infrastructure for the the assertion-related macros in c.h.
+ *
+ * ExceptionalCondition must be present even when assertions are not enabled.
*/
-extern PGDLLIMPORT bool assert_enabled;
-
extern void ExceptionalCondition(const char *conditionName,
const char *errorType,
const char *fileName, int lineNumber) __attribute__((noreturn));
--
2.0.0.rc2.4.g1dc51c6.dirty
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
