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.

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 697cf99de54e1eeecdab0d368d374ed8ced2a365..8f0ca4c1eb6f76c10d85da2737e512973d860f36 100644 (file)
--- 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">
+      <term><varname>debug_assertions</varname> (<type>boolean</type>)
+      <indexterm>
+       <primary><varname>debug_assertions</> configuration parameter</primary>
+      </indexterm>
+      </term>
+      <listitem>
+       <para>
+        Reports whether <productname>PostgreSQL</productname> has been built
+        with assertions enabled. That is the case if the
+        macro <symbol>USE_ASSERT_CHECKING</symbol> is defined
+        when <productname>PostgreSQL</productname> is built (accomplished
+        e.g. by the <command>configure</command> option
+        <option>--enable-cassert</option>). By
+        default <productname>PostgreSQL</productname> is built without
+        assertions.
+       </para>
+      </listitem>
+     </varlistentry>
+
      <varlistentry id="guc-integer-datetimes" xreflabel="integer_datetimes">
       <term><varname>integer_datetimes</varname> (<type>boolean</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">
-      <term><varname>debug_assertions</varname> (<type>boolean</type>)
-      <indexterm>
-       <primary><varname>debug_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 <symbol>USE_ASSERT_CHECKING</symbol>
-        must be defined when <productname>PostgreSQL</productname> is
-        built (accomplished by the <command>configure</command> option
-        <option>--enable-cassert</option>). Note that
-        <varname>debug_assertions</varname> defaults to <literal>on</>
-        if <productname>PostgreSQL</productname> has been built with
-        assertions enabled.
-       </para>
-      </listitem>
-     </varlistentry>
-
      <varlistentry id="guc-ignore-system-indexes" xreflabel="ignore_system_indexes">
       <term><varname>ignore_system_indexes</varname> (<type>boolean</type>)
       <indexterm>
@@ -7354,10 +7352,6 @@ LOG:  CleanUpLock: deleting: lock(0xb7acd844) id(24688,24696,0,0,0,1)
       </thead>
 
       <tbody>
-       <row>
-        <entry><option>-A <replaceable>x</replaceable></option></entry>
-        <entry><literal>debug_assertions = <replaceable>x</replaceable></></entry>
-       </row>
        <row>
         <entry><option>-B <replaceable>x</replaceable></option></entry>
         <entry><literal>shared_buffers = <replaceable>x</replaceable></></entry>

diff --git a/doc/src/sgml/ref/postgres-ref.sgml b/doc/src/sgml/ref/postgres-ref.sgml
index 8e225e4c5d0b9e5c22685ef31664664013eef96a..cdfdaa0b397ade839cf9274403f7002e5659ff0a 100644 (file)
--- a/doc/src/sgml/ref/postgres-ref.sgml
+++ b/doc/src/sgml/ref/postgres-ref.sgml
@@ -101,18 +101,6 @@ PostgreSQL documentation
     <title>General Purpose</title>
 
     <variablelist>
-     <varlistentry>
-      <term><option>-A 0|1</option></term>
-      <listitem>
-       <para>
-        Enables run-time assertion checks, which is a debugging aid to
-        detect programming mistakes.  This option is only available if
-        assertions were enabled when <productname>PostgreSQL</> was
-        compiled. If so, the default is on.
-       </para>
-      </listitem>
-     </varlistentry>
-
      <varlistentry>
       <term><option>-B <replaceable class="parameter">nbuffers</replaceable></option></term>
       <listitem>

diff --git a/src/backend/access/gin/ginpostinglist.c b/src/backend/access/gin/ginpostinglist.c
index 606a824f1254095c81f5929752013e07a4e41b67..ea59dea13d11d7096da87f4adca19445eca7a6eb 100644 (file)
--- 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 6d4e09116cfb78a2f3309832533c1ae8654d41a1..110fe004a464fcbbb5001d31a26b1c85e586e602 100644 (file)
--- 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/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index a5d5c2dbcb62b1b5eb858d49fa4700d13e752fc3..5a406e12c24a9e3d20f490d718c8c8f3a5987047 100644 (file)
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -603,14 +603,10 @@ PostmasterMain(int argc, char *argv[])
         * tcop/postgres.c (the option sets should not conflict) and with the
         * common help() function in main/main.c.
         */
-       while ((opt = getopt(argc, argv, "A:B:bc:C:D:d:EeFf:h:ijk:lN:nOo:Pp:r:S:sTt:W:-:")) != -1)
+       while ((opt = getopt(argc, argv, "B:bc:C:D:d:EeFf:h:ijk:lN:nOo:Pp:r:S:sTt:W:-:")) != -1)
        {
                switch (opt)
                {
-                       case 'A':
-                               SetConfigOption("debug_assertions", optarg, PGC_POSTMASTER, PGC_S_ARGV);
-                               break;
-
                        case 'B':
                                SetConfigOption("shared_buffers", optarg, PGC_POSTMASTER, PGC_S_ARGV);
                                break;

diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index c0702789446df02400fbeeee1f2bae7edd53d2f8..07ea6652190dd7d390d0c4398856fd887f0bfe5d 100644 (file)
--- 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 3135c5cf156502ff9e7a511ac079ff7cd668e036..6c81be4237709c1603ea0c6f05eab741264700ae 100644 (file)
--- 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;
@@ -519,34 +519,29 @@ AtEOXact_LocalBuffers(bool isCommit)
 #endif
 }
 
+/*
+ * 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 266b0daa94f48f5c3937d77f5537282067ce7664..dfaf10e4b5c58c0dde687a8a1ce3106dab381f6a 100644 (file)
--- 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/tcop/postgres.c b/src/backend/tcop/postgres.c
index 9abc11bea51ffc7295a093f274cd2e0aa9cc0154..6b4221a2439549d3e5d8a3b689080c001a401540 100644 (file)
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -3305,14 +3305,10 @@ process_postgres_switches(int argc, char *argv[], GucContext ctx,
         * postmaster/postmaster.c (the option sets should not conflict) and with
         * the common help() function in main/main.c.
         */
-       while ((flag = getopt(argc, argv, "A:B:bc:C:D:d:EeFf:h:ijk:lN:nOo:Pp:r:S:sTt:v:W:-:")) != -1)
+       while ((flag = getopt(argc, argv, "B:bc:C:D:d:EeFf:h:ijk:lN:nOo:Pp:r:S:sTt:v:W:-:")) != -1)
        {
                switch (flag)
                {
-                       case 'A':
-                               SetConfigOption("debug_assertions", optarg, ctx, gucsource);
-                               break;
-
                        case 'B':
                                SetConfigOption("shared_buffers", optarg, ctx, gucsource);
                                break;

diff --git a/src/backend/utils/cache/catcache.c b/src/backend/utils/cache/catcache.c
index 954b435bffa7fa6d0bbfe13b076f1319c71444b1..4dd6753c82f2ca3bc5c9a4887bc107c7e1b5b0f4 100644 (file)
--- 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 557ff6148d0fce334fa4feca41dbea8c7d3fbbe5..1e8429c64c335c9607c164d91e829439f1f1006a 100644 (file)
--- 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 8ca065b7feeb51a28b8bb49c7f78903c2f22d8e0..6902c2322a9481350a3fb43fa6848e2f399f9092 100644 (file)
--- 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
        },
 
        {
@@ -9117,19 +9112,6 @@ check_phony_autocommit(bool *newval, void **extra, GucSource source)
        return true;
 }
 
-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)
 {

diff --git a/src/include/c.h b/src/include/c.h
index df22d50d4e40826520b3b50292199b71ca0aa670..a48a57a42b30a2e0121a7d62bfe486a87d87fc86 100644 (file)
--- 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 00fbaaf91bcd784b058f59f4b11dcadd282e3fd9..b123813bc477d1e2fb0b2c6e9b5a04b3670b546d 100644 (file)
--- 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));