Re: Getting better results from valgrind leak tracking - Mailing list pgsql-hackers

I wrote:
> Andres Freund <andres@anarazel.de> writes:
>> There's plenty other hits, but I think I should get back to working on
>> making the shared memory stats patch committable. I really wouldn't want
>> it to slip yet another year.

> +1, so far there's little indication that we're finding any serious leaks
> here.  Might be best to set it all aside till there's more time.

Well, I failed to resist the temptation to keep poking at this issue,
mainly because I felt that it'd be a good idea to make sure we'd gotten
our arms around all of the detectable issues, even if we choose not to
fix them right away.  The attached patch, combined with your preceding
memory context patch, eliminates all but a very small number of the leak
complaints in the core regression tests.

The remaing leak warnings that I see are:

1. WaitForReplicationWorkerAttach leaks the BackgroundWorkerHandle it's
passed.  I'm not sure which function should clean that up, but in any
case it's only 16 bytes one time in one process, so it's hardly exciting.

2. There's lots of leakage in text search dictionary initialization
functions.  This is hard to get around completely, because the API for
those functions has them being called in the dictionary's long-lived
context.  In any case, the leaks are not large (modulo comments below),
and they would get cleaned up if the dictionary cache were thrown away.

3. partition_prune.sql repeatably produces this warning:

==00:00:44:39.764 3813570== 32,768 bytes in 1 blocks are possibly lost in loss record 2,084 of 2,096
==00:00:44:39.764 3813570==    at 0x4C30F0B: malloc (vg_replace_malloc.c:307)
==00:00:44:39.764 3813570==    by 0x94315A: AllocSetAlloc (aset.c:941)
==00:00:44:39.764 3813570==    by 0x94B52F: MemoryContextAlloc (mcxt.c:804)
==00:00:44:39.764 3813570==    by 0x8023DA: LockAcquireExtended (lock.c:845)
==00:00:44:39.764 3813570==    by 0x7FFC7E: LockRelationOid (lmgr.c:116)
==00:00:44:39.764 3813570==    by 0x5CB89F: findDependentObjects (dependency.c:962)
==00:00:44:39.764 3813570==    by 0x5CBA66: findDependentObjects (dependency.c:1060)
==00:00:44:39.764 3813570==    by 0x5CBA66: findDependentObjects (dependency.c:1060)
==00:00:44:39.764 3813570==    by 0x5CCB72: performMultipleDeletions (dependency.c:409)
==00:00:44:39.764 3813570==    by 0x66F574: RemoveRelations (tablecmds.c:1395)
==00:00:44:39.764 3813570==    by 0x81C986: ProcessUtilitySlow.isra.8 (utility.c:1698)
==00:00:44:39.764 3813570==    by 0x81BCF2: standard_ProcessUtility (utility.c:1034)

which evidently is warning that some LockMethodLocalHash entry is losing
track of its lockOwners array.  But I sure don't see how that could
happen, nor why it'd only happen in this test.  Could this be a false
report?

As you can see from the patch's additions to src/tools/valgrind.supp,
I punted on the issues around pl/pgsql's function-compile-time leaks.
We know that's an issue, but again the leaks are fairly small and
they are confined within function cache entries, so I'm not sure
how hard we should work on that.

(An idea for silencing both this and the dictionary leak warnings is
to install an on-proc-exit callback to drop the cached objects'
contexts.)

Anyway, looking through the patch, I found several non-cosmetic issues:

* You were right to suspect that there was residual leakage in guc.c's
handling of error cases.  ProcessGUCArray and call_string_check_hook
are both guilty of leaking malloc'd strings if an error in a proposed
GUC setting is detected.

* I still haven't tried to wrap my brain around the question of what
RestoreGUCState really needs to be doing.  I have, however, found that
check-world passes just fine with the InitializeOneGUCOption calls
diked out entirely, so that's what's in this patch.

* libpqwalreceiver.c leaks a malloc'd error string when
libpqrcv_check_conninfo detects an erroneous conninfo string.

* spell.c leaks a compiled regular expression if an ispell dictionary
cache entry is dropped.  I fixed this by adding a memory context reset
callback to release the regex.  This is potentially a rather large
leak, if the regex is complex (though typically it wouldn't be).

* BuildEventTriggerCache leaks working storage into
EventTriggerCacheContext.

* Likewise, load_domaintype_info leaks working storage into
a long-lived cache context.

* RelationDestroyRelation leaks rd_statlist; the patch that added
that failed to emulate the rd_indexlist prototype very well.

* As previously noted, RelationInitTableAccessMethod causes leaks.

* I made some effort to remove easily-removable leakage in
lookup_ts_dictionary_cache itself, although given the leaks in
its callees, this isn't terribly exciting.

I am suspicious that there's something still not quite right in the
memory context changes, because I noticed that the sanity_check.sql
test (and no other ones) complained about row_description_context being
leaked.  I realized that the warning was because my compiler optimizes
away the row_description_context static variable altogether, leaving no
pointer to the context behind.  I hacked around that in this patch by
marking that variable volatile.  However, that explanation just begs
the question of why every run didn't show the same warning.  I suppose
that valgrind was considering the context to be referenced by some
child or sibling context pointer, but if that's the explanation then
we should never have seen the warning.  I'm forced to the conclusion
that valgrind is counting some but not all child/sibling context
pointers, which sure seems like a bug.  Maybe we need the two-level-
mempool mechanism after all to get that to work better.

Anyway, I think we need to commit and even back-patch the portion
of the attached changes that are in
* libpqwalreceiver.c
* spell.h / spell.c
* relcache.c
* guc.c (except the quick hack in RestoreGUCState)
Those are all genuine leaks that are in places where they could be
repeated and thus potentially add up to something significant.

There's a weaker case for applying the changes in evtcache.c,
ts_cache.c, and typcache.c.  Those are all basically leaving some cruft
behind in a long-lived cache entry.  But the cruft isn't huge and it
would be recovered at cache flush, so I'm not convinced it amounts to a
real-world problem.

The rest of this is either working around valgrind shortcomings or
jumping through a hoop to convince it that some data structure that's
still around at exit is still referenced.  Maybe we should commit some
of it under "#ifdef USE_VALGRIND" tests.  I really want to find some
other answer than moving the dlist_node fields, though.

Comments?

            regards, tom lane

diff --git a/src/backend/libpq/pqcomm.c b/src/backend/libpq/pqcomm.c
index 4c7b1e7bfd..cd984929a6 100644
--- a/src/backend/libpq/pqcomm.c
+++ b/src/backend/libpq/pqcomm.c
@@ -888,7 +888,6 @@ RemoveSocketFiles(void)
         (void) unlink(sock_path);
     }
     /* Since we're about to exit, no need to reclaim storage */
-    sock_paths = NIL;
 }


diff --git a/src/backend/libpq/pqmq.c b/src/backend/libpq/pqmq.c
index d1a1f47a78..3cadfecfb0 100644
--- a/src/backend/libpq/pqmq.c
+++ b/src/backend/libpq/pqmq.c
@@ -22,6 +22,7 @@
 #include "utils/builtins.h"

 static shm_mq_handle *pq_mq_handle;
+static shm_mq_handle *volatile dummy_mq_handle = NULL;
 static bool pq_mq_busy = false;
 static pid_t pq_mq_parallel_leader_pid = 0;
 static pid_t pq_mq_parallel_leader_backend_id = InvalidBackendId;
@@ -64,6 +65,9 @@ pq_redirect_to_shm_mq(dsm_segment *seg, shm_mq_handle *mqh)
 static void
 pq_cleanup_redirect_to_shm_mq(dsm_segment *seg, Datum arg)
 {
+    /* fake out valgrind */
+    if (pq_mq_handle)
+        dummy_mq_handle = pq_mq_handle;
     pq_mq_handle = NULL;
     whereToSendOutput = DestNone;
 }
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index 23ef23c13e..c4e2e1d228 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -850,6 +850,15 @@ AutoVacLauncherShutdown(void)
             (errmsg_internal("autovacuum launcher shutting down")));
     AutoVacuumShmem->av_launcherpid = 0;

+    /*
+     * If valgrind'ing, free all the cruft in AutovacMemCxt, to avoid leak
+     * complaints.
+     */
+#ifdef USE_VALGRIND
+    MemoryContextSwitchTo(TopMemoryContext);
+    MemoryContextDelete(AutovacMemCxt);
+#endif
+
     proc_exit(0);                /* done */
 }

@@ -2045,11 +2054,12 @@ do_autovacuum(void)
     /* create hash table for toast <-> main relid mapping */
     ctl.keysize = sizeof(Oid);
     ctl.entrysize = sizeof(av_relation);
+    ctl.hcxt = AutovacMemCxt;

     table_toast_map = hash_create("TOAST to main relid map",
                                   100,
                                   &ctl,
-                                  HASH_ELEM | HASH_BLOBS);
+                                  HASH_ELEM | HASH_BLOBS | HASH_CONTEXT);

     /*
      * Scan pg_class to determine which tables to vacuum.
@@ -2596,11 +2606,6 @@ deleted:
     }
     LWLockRelease(AutovacuumLock);

-    /*
-     * We leak table_toast_map here (among other things), but since we're
-     * going away soon, it's not a problem.
-     */
-
     /*
      * Update pg_database.datfrozenxid, and truncate pg_xact if possible. We
      * only need to do this once, not after each table.
@@ -2626,6 +2631,14 @@ deleted:

     /* Finally close out the last transaction. */
     CommitTransactionCommand();
+
+    /*
+     * If valgrind'ing, free all the cruft in AutovacMemCxt, to avoid leak
+     * complaints.
+     */
+#ifdef USE_VALGRIND
+    MemoryContextDelete(AutovacMemCxt);
+#endif
 }

 /*
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index e8af05c04e..2814ee7609 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -176,13 +176,13 @@
  */
 typedef struct bkend
 {
+    dlist_node    elem;            /* list link in BackendList */
     pid_t        pid;            /* process id of backend */
     int32        cancel_key;        /* cancel key for cancels for this backend */
     int            child_slot;        /* PMChildSlot for this backend, if any */
     int            bkend_type;        /* child process flavor, see above */
     bool        dead_end;        /* is it going to send an error and quit? */
     bool        bgworker_notify;    /* gets bgworker start/stop notifications */
-    dlist_node    elem;            /* list link in BackendList */
 } Backend;

 static dlist_head BackendList = DLIST_STATIC_INIT(BackendList);
@@ -2011,6 +2011,7 @@ ProcessStartupPacket(Port *port, bool ssl_done, bool gss_done)
     if (proto == CANCEL_REQUEST_CODE)
     {
         processCancelRequest(port, buf);
+        pfree(buf);
         /* Not really an error, but we don't want to proceed further */
         return STATUS_ERROR;
     }
diff --git a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
index f74378110a..021c1b36f3 100644
--- a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
+++ b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
@@ -246,9 +246,15 @@ libpqrcv_check_conninfo(const char *conninfo)

     opts = PQconninfoParse(conninfo, &err);
     if (opts == NULL)
+    {
+        /* The error string is malloc'd, so we must free it explicitly */
+        char       *errcopy = err ? pstrdup(err) : "out of memory";
+
+        PQfreemem(err);
         ereport(ERROR,
                 (errcode(ERRCODE_SYNTAX_ERROR),
-                 errmsg("invalid connection string syntax: %s", err)));
+                 errmsg("invalid connection string syntax: %s", errcopy)));
+    }

     PQconninfoFree(opts);
 }
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 2b1b68109f..0ceeb8f911 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -176,7 +176,7 @@ static bool RecoveryConflictRetryable = true;
 static ProcSignalReason RecoveryConflictReason;

 /* reused buffer to pass to SendRowDescriptionMessage() */
-static MemoryContext row_description_context = NULL;
+static volatile MemoryContext row_description_context = NULL;
 static StringInfoData row_description_buf;

 /* ----------------------------------------------------------------
diff --git a/src/backend/tsearch/spell.c b/src/backend/tsearch/spell.c
index 9b9a9afaa8..0b9d8353a9 100644
--- a/src/backend/tsearch/spell.c
+++ b/src/backend/tsearch/spell.c
@@ -654,6 +654,17 @@ FindWord(IspellDict *Conf, const char *word, const char *affixflag, int flag)
     return 0;
 }

+/*
+ * Context reset/delete callback for a regular-expression AFFIX
+ */
+static void
+regex_affix_deletion_callback(void *arg)
+{
+    aff_regex_struct *affregex = (aff_regex_struct *) arg;
+
+    pg_regfree(&(affregex->regex));
+}
+
 /*
  * Adds a new affix rule to the Affix field.
  *
@@ -716,6 +727,7 @@ NIAddAffix(IspellDict *Conf, const char *flag, char flagflags, const char *mask,
         int            err;
         pg_wchar   *wmask;
         char       *tmask;
+        aff_regex_struct *pregex;

         Affix->issimple = 0;
         Affix->isregis = 0;
@@ -729,18 +741,32 @@ NIAddAffix(IspellDict *Conf, const char *flag, char flagflags, const char *mask,
         wmask = (pg_wchar *) tmpalloc((masklen + 1) * sizeof(pg_wchar));
         wmasklen = pg_mb2wchar_with_len(tmask, wmask, masklen);

-        err = pg_regcomp(&(Affix->reg.regex), wmask, wmasklen,
+        /*
+         * The regex engine stores its stuff using malloc not palloc, so we
+         * must arrange to explicitly clean up the regex when the dictionary's
+         * context is cleared.  That means the regex_t has to stay in a fixed
+         * location within the context; we can't keep it directly in the AFFIX
+         * struct, since we may sort and resize the array of AFFIXes.
+         */
+        Affix->reg.pregex = pregex = palloc(sizeof(aff_regex_struct));
+
+        err = pg_regcomp(&(pregex->regex), wmask, wmasklen,
                          REG_ADVANCED | REG_NOSUB,
                          DEFAULT_COLLATION_OID);
         if (err)
         {
             char        errstr[100];

-            pg_regerror(err, &(Affix->reg.regex), errstr, sizeof(errstr));
+            pg_regerror(err, &(pregex->regex), errstr, sizeof(errstr));
             ereport(ERROR,
                     (errcode(ERRCODE_INVALID_REGULAR_EXPRESSION),
                      errmsg("invalid regular expression: %s", errstr)));
         }
+
+        pregex->mcallback.func = regex_affix_deletion_callback;
+        pregex->mcallback.arg = (void *) pregex;
+        MemoryContextRegisterResetCallback(CurrentMemoryContext,
+                                           &pregex->mcallback);
     }

     Affix->flagflags = flagflags;
@@ -2133,7 +2159,7 @@ CheckAffix(const char *word, size_t len, AFFIX *Affix, int flagflags, char *neww
         data = (pg_wchar *) palloc((newword_len + 1) * sizeof(pg_wchar));
         data_len = pg_mb2wchar_with_len(newword, data, newword_len);

-        if (pg_regexec(&(Affix->reg.regex), data, data_len,
+        if (pg_regexec(&(Affix->reg.pregex->regex), data, data_len,
                        0, NULL, 0, NULL, 0) == REG_OKAY)
         {
             pfree(data);
diff --git a/src/backend/utils/cache/catcache.c b/src/backend/utils/cache/catcache.c
index 55c9445898..2abdd44190 100644
--- a/src/backend/utils/cache/catcache.c
+++ b/src/backend/utils/cache/catcache.c
@@ -814,7 +814,7 @@ InitCatCache(int id,
      * Note: we rely on zeroing to initialize all the dlist headers correctly
      */
     sz = sizeof(CatCache) + PG_CACHE_LINE_SIZE;
-    cp = (CatCache *) CACHELINEALIGN(palloc0(sz));
+    cp = (CatCache *) palloc0(sz);
     cp->cc_bucket = palloc0(nbuckets * sizeof(dlist_head));

     /*
diff --git a/src/backend/utils/cache/evtcache.c b/src/backend/utils/cache/evtcache.c
index 460b720a65..cf2c26a60f 100644
--- a/src/backend/utils/cache/evtcache.c
+++ b/src/backend/utils/cache/evtcache.c
@@ -111,9 +111,6 @@ BuildEventTriggerCache(void)
                                       (Datum) 0);
     }

-    /* Switch to correct memory context. */
-    oldcontext = MemoryContextSwitchTo(EventTriggerCacheContext);
-
     /* Prevent the memory context from being nuked while we're rebuilding. */
     EventTriggerCacheState = ETCS_REBUILD_STARTED;

@@ -170,6 +167,9 @@ BuildEventTriggerCache(void)
         else
             continue;

+        /* Switch to correct memory context. */
+        oldcontext = MemoryContextSwitchTo(EventTriggerCacheContext);
+
         /* Allocate new cache item. */
         item = palloc0(sizeof(EventTriggerCacheItem));
         item->fnoid = form->evtfoid;
@@ -187,6 +187,9 @@ BuildEventTriggerCache(void)
             entry->triggerlist = lappend(entry->triggerlist, item);
         else
             entry->triggerlist = list_make1(item);
+
+        /* Restore previous memory context. */
+        MemoryContextSwitchTo(oldcontext);
     }

     /* Done with pg_event_trigger scan. */
@@ -194,9 +197,6 @@ BuildEventTriggerCache(void)
     index_close(irel, AccessShareLock);
     relation_close(rel, AccessShareLock);

-    /* Restore previous memory context. */
-    MemoryContextSwitchTo(oldcontext);
-
     /* Install new cache. */
     EventTriggerCache = cache;

diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index 7ef510cd01..d90b2812af 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -2392,6 +2392,7 @@ RelationDestroyRelation(Relation relation, bool remember_tupdesc)
     FreeTriggerDesc(relation->trigdesc);
     list_free_deep(relation->rd_fkeylist);
     list_free(relation->rd_indexlist);
+    list_free(relation->rd_statlist);
     bms_free(relation->rd_indexattr);
     bms_free(relation->rd_keyattr);
     bms_free(relation->rd_pkattr);
@@ -3542,6 +3543,13 @@ RelationBuildLocalRelation(const char *relname,

     rel->rd_rel->relam = accessmtd;

+    /*
+     * RelationInitTableAccessMethod will do syscache lookups, so we
+     * mustn't run it in CacheMemoryContext.  Fortunately, the remaining
+     * steps don't require a long-lived current context.
+     */
+    MemoryContextSwitchTo(oldcxt);
+
     if (relkind == RELKIND_RELATION ||
         relkind == RELKIND_SEQUENCE ||
         relkind == RELKIND_TOASTVALUE ||
@@ -3565,11 +3573,6 @@ RelationBuildLocalRelation(const char *relname,
      */
     EOXactListAdd(rel);

-    /*
-     * done building relcache entry.
-     */
-    MemoryContextSwitchTo(oldcxt);
-
     /* It's fully valid */
     rel->rd_isvalid = true;

diff --git a/src/backend/utils/cache/ts_cache.c b/src/backend/utils/cache/ts_cache.c
index 384107b6ba..a983478f69 100644
--- a/src/backend/utils/cache/ts_cache.c
+++ b/src/backend/utils/cache/ts_cache.c
@@ -314,11 +314,18 @@ lookup_ts_dictionary_cache(Oid dictId)

         if (OidIsValid(template->tmplinit))
         {
+            FmgrInfo    initflinfo;
             List       *dictoptions;
             Datum        opt;
             bool        isnull;
             MemoryContext oldcontext;

+            /*
+             * Lookup the init method while using caller's context, else we
+             * might leak cruft in the private memory context.
+             */
+            fmgr_info(template->tmplinit, &initflinfo);
+
             /*
              * Init method runs in dictionary's private memory context, and we
              * make sure the options are stored there too
@@ -333,9 +340,17 @@ lookup_ts_dictionary_cache(Oid dictId)
             else
                 dictoptions = deserialize_deflist(opt);

+            /*
+             * We don't currently have any real use for the dictoptions list
+             * after calling the init method, but the dictionary might
+             * continue to reference parts of it, so we can't free it.  Keep a
+             * pointer to it so valgrind doesn't complain that it's leaked.
+             */
+            entry->dictOptions = dictoptions;
+
             entry->dictData =
-                DatumGetPointer(OidFunctionCall1(template->tmplinit,
-                                                 PointerGetDatum(dictoptions)));
+                DatumGetPointer(FunctionCall1(&initflinfo,
+                                              PointerGetDatum(dictoptions)));

             MemoryContextSwitchTo(oldcontext);
         }
diff --git a/src/backend/utils/cache/typcache.c b/src/backend/utils/cache/typcache.c
index 4915ef5934..570f2986a7 100644
--- a/src/backend/utils/cache/typcache.c
+++ b/src/backend/utils/cache/typcache.c
@@ -1091,9 +1091,6 @@ load_domaintype_info(TypeCacheEntry *typentry)
                 dcc->dccRefCount = 0;
             }

-            /* Create node trees in DomainConstraintCache's context */
-            oldcxt = MemoryContextSwitchTo(dcc->dccContext);
-
             check_expr = (Expr *) stringToNode(constring);

             /*
@@ -1108,10 +1105,13 @@ load_domaintype_info(TypeCacheEntry *typentry)
              */
             check_expr = expression_planner(check_expr);

+            /* Create node trees in DomainConstraintCache's context */
+            oldcxt = MemoryContextSwitchTo(dcc->dccContext);
+
             r = makeNode(DomainConstraintState);
             r->constrainttype = DOM_CONSTRAINT_CHECK;
             r->name = pstrdup(NameStr(c->conname));
-            r->check_expr = check_expr;
+            r->check_expr = copyObject(check_expr);
             r->check_exprstate = NULL;

             MemoryContextSwitchTo(oldcxt);
diff --git a/src/backend/utils/hash/dynahash.c b/src/backend/utils/hash/dynahash.c
index 6546e3c7c7..df39bc77df 100644
--- a/src/backend/utils/hash/dynahash.c
+++ b/src/backend/utils/hash/dynahash.c
@@ -1713,6 +1713,10 @@ element_alloc(HTAB *hashp, int nelem, int freelist_idx)
     if (hashp->isfixed)
         return false;

+    /* Force separate allocations to de-confuse valgrind */
+    if (!hashp->isshared)
+        nelem = 1;
+
     /* Each element has a HASHELEMENT header plus user data. */
     elementSize = MAXALIGN(sizeof(HASHELEMENT)) + MAXALIGN(hctl->entrysize);

diff --git a/src/backend/utils/init/miscinit.c b/src/backend/utils/init/miscinit.c
index 8b73850d0d..be37e8b312 100644
--- a/src/backend/utils/init/miscinit.c
+++ b/src/backend/utils/init/miscinit.c
@@ -944,7 +944,6 @@ UnlinkLockFiles(int status, Datum arg)
         /* Should we complain if the unlink fails? */
     }
     /* Since we're about to exit, no need to reclaim storage */
-    lock_files = NIL;

     /*
      * Lock file removal should always be the last externally visible action
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 855076b1fd..c2e662a7b6 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -10639,10 +10639,12 @@ RestoreGUCState(void *gucstate)
     int            i;
     ErrorContextCallback error_context_callback;

+#if 0
     /* See comment at can_skip_gucvar(). */
     for (i = 0; i < num_guc_variables; i++)
         if (!can_skip_gucvar(guc_variables[i]))
             InitializeOneGUCOption(guc_variables[i]);
+#endif

     /* First item is the length of the subsequent data */
     memcpy(&len, gucstate, sizeof(len));
@@ -10754,6 +10756,8 @@ ProcessGUCArray(ArrayType *array,
         char       *s;
         char       *name;
         char       *value;
+        char       *namecopy;
+        char       *valuecopy;

         d = array_ref(array, 1, &i,
                       -1 /* varlenarray */ ,
@@ -10778,13 +10782,18 @@ ProcessGUCArray(ArrayType *array,
             continue;
         }

-        (void) set_config_option(name, value,
+        /* free strings immediately to avoid permanent leak if error */
+        namecopy = pstrdup(name);
+        free(name);
+        valuecopy = pstrdup(value);
+        free(value);
+
+        (void) set_config_option(namecopy, valuecopy,
                                  context, source,
                                  action, true, 0, false);

-        free(name);
-        if (value)
-            free(value);
+        pfree(namecopy);
+        pfree(valuecopy);
         pfree(s);
     }
 }
@@ -11216,34 +11225,50 @@ static bool
 call_string_check_hook(struct config_string *conf, char **newval, void **extra,
                        GucSource source, int elevel)
 {
+    volatile bool result = true;
+
     /* Quick success if no hook */
     if (!conf->check_hook)
         return true;

-    /* Reset variables that might be set by hook */
-    GUC_check_errcode_value = ERRCODE_INVALID_PARAMETER_VALUE;
-    GUC_check_errmsg_string = NULL;
-    GUC_check_errdetail_string = NULL;
-    GUC_check_errhint_string = NULL;
+    /*
+     * If elevel is ERROR, or if the check_hook itself throws an elog
+     * (undesirable, but not always avoidable), make sure we don't leak the
+     * already-malloc'd newval string.
+     */
+    PG_TRY();
+    {
+        /* Reset variables that might be set by hook */
+        GUC_check_errcode_value = ERRCODE_INVALID_PARAMETER_VALUE;
+        GUC_check_errmsg_string = NULL;
+        GUC_check_errdetail_string = NULL;
+        GUC_check_errhint_string = NULL;

-    if (!conf->check_hook(newval, extra, source))
+        if (!conf->check_hook(newval, extra, source))
+        {
+            ereport(elevel,
+                    (errcode(GUC_check_errcode_value),
+                     GUC_check_errmsg_string ?
+                     errmsg_internal("%s", GUC_check_errmsg_string) :
+                     errmsg("invalid value for parameter \"%s\": \"%s\"",
+                            conf->gen.name, *newval ? *newval : ""),
+                     GUC_check_errdetail_string ?
+                     errdetail_internal("%s", GUC_check_errdetail_string) : 0,
+                     GUC_check_errhint_string ?
+                     errhint("%s", GUC_check_errhint_string) : 0));
+            /* Flush any strings created in ErrorContext */
+            FlushErrorState();
+            result = false;
+        }
+    }
+    PG_CATCH();
     {
-        ereport(elevel,
-                (errcode(GUC_check_errcode_value),
-                 GUC_check_errmsg_string ?
-                 errmsg_internal("%s", GUC_check_errmsg_string) :
-                 errmsg("invalid value for parameter \"%s\": \"%s\"",
-                        conf->gen.name, *newval ? *newval : ""),
-                 GUC_check_errdetail_string ?
-                 errdetail_internal("%s", GUC_check_errdetail_string) : 0,
-                 GUC_check_errhint_string ?
-                 errhint("%s", GUC_check_errhint_string) : 0));
-        /* Flush any strings created in ErrorContext */
-        FlushErrorState();
-        return false;
+        free(*newval);
+        PG_RE_THROW();
     }
+    PG_END_TRY();

-    return true;
+    return result;
 }

 static bool
diff --git a/src/backend/utils/misc/ps_status.c b/src/backend/utils/misc/ps_status.c
index 5819faaf2d..2d0ef37f7f 100644
--- a/src/backend/utils/misc/ps_status.c
+++ b/src/backend/utils/misc/ps_status.c
@@ -113,6 +113,11 @@ static size_t ps_buffer_fixed_size; /* size of the constant prefix */
 static int    save_argc;
 static char **save_argv;

+/* We use this to convince Valgrind that replacement environ is referenced */
+#ifdef PS_USE_CLOBBER_ARGV
+static char ** volatile fake_environ;
+#endif
+

 /*
  * Call this early in startup to save the original argc/argv values.
@@ -192,6 +197,8 @@ save_ps_display_args(int argc, char **argv)
         }
         new_environ[i] = NULL;
         environ = new_environ;
+        /* Valgrind tends to think this memory is leaked, so fool it */
+        fake_environ = new_environ;
     }
 #endif                            /* PS_USE_CLOBBER_ARGV */

diff --git a/src/include/postmaster/bgworker_internals.h b/src/include/postmaster/bgworker_internals.h
index fc7706314b..f32a13f786 100644
--- a/src/include/postmaster/bgworker_internals.h
+++ b/src/include/postmaster/bgworker_internals.h
@@ -32,6 +32,7 @@
  */
 typedef struct RegisteredBgWorker
 {
+    slist_node    rw_lnode;        /* list link (first to placate valgrind) */
     BackgroundWorker rw_worker; /* its registry entry */
     struct bkend *rw_backend;    /* its BackendList entry, or NULL */
     pid_t        rw_pid;            /* 0 if not running */
@@ -39,7 +40,6 @@ typedef struct RegisteredBgWorker
     TimestampTz rw_crashed_at;    /* if not 0, time it last crashed */
     int            rw_shmem_slot;
     bool        rw_terminate;
-    slist_node    rw_lnode;        /* list link */
 } RegisteredBgWorker;

 extern slist_head BackgroundWorkerList;
diff --git a/src/include/tsearch/dicts/spell.h b/src/include/tsearch/dicts/spell.h
index 9847e30208..e03ed42372 100644
--- a/src/include/tsearch/dicts/spell.h
+++ b/src/include/tsearch/dicts/spell.h
@@ -81,6 +81,17 @@ typedef struct spell_struct

 #define SPELLHDRSZ    (offsetof(SPELL, word))

+/*
+ * If an affix uses a regex, we have to store that separately in a struct
+ * that won't move around when arrays of affixes are enlarged or sorted.
+ * This is so that it can be found to be cleaned up at context destruction.
+ */
+typedef struct aff_regex_struct
+{
+    regex_t        regex;
+    MemoryContextCallback mcallback;
+} aff_regex_struct;
+
 /*
  * Represents an entry in an affix list.
  */
@@ -97,7 +108,7 @@ typedef struct aff_struct
     char       *repl;
     union
     {
-        regex_t        regex;
+        aff_regex_struct *pregex;
         Regis        regis;
     }            reg;
 } AFFIX;
diff --git a/src/include/tsearch/ts_cache.h b/src/include/tsearch/ts_cache.h
index 888f7028b1..c5617bab5b 100644
--- a/src/include/tsearch/ts_cache.h
+++ b/src/include/tsearch/ts_cache.h
@@ -59,6 +59,7 @@ typedef struct TSDictionaryCacheEntry
     FmgrInfo    lexize;

     MemoryContext dictCtx;        /* memory context to store private data */
+    List       *dictOptions;
     void       *dictData;
 } TSDictionaryCacheEntry;

diff --git a/src/include/utils/catcache.h b/src/include/utils/catcache.h
index ddc2762eb3..a304981467 100644
--- a/src/include/utils/catcache.h
+++ b/src/include/utils/catcache.h
@@ -85,6 +85,13 @@ typedef struct catcache

 typedef struct catctup
 {
+    /*
+     * Each tuple in a cache is a member of a dlist that stores the elements
+     * of its hash bucket.  We keep each dlist in LRU order to speed repeated
+     * lookups.
+     */
+    dlist_node    cache_elem;        /* list member of per-bucket list */
+
     int            ct_magic;        /* for identifying CatCTup entries */
 #define CT_MAGIC   0x57261502

@@ -96,13 +103,6 @@ typedef struct catctup
      */
     Datum        keys[CATCACHE_MAXKEYS];

-    /*
-     * Each tuple in a cache is a member of a dlist that stores the elements
-     * of its hash bucket.  We keep each dlist in LRU order to speed repeated
-     * lookups.
-     */
-    dlist_node    cache_elem;        /* list member of per-bucket list */
-
     /*
      * A tuple marked "dead" must not be returned by subsequent searches.
      * However, it won't be physically deleted from the cache until its
@@ -156,13 +156,13 @@ typedef struct catctup
  */
 typedef struct catclist
 {
+    dlist_node    cache_elem;        /* list member of per-catcache list */
+
     int            cl_magic;        /* for identifying CatCList entries */
 #define CL_MAGIC   0x52765103

     uint32        hash_value;        /* hash value for lookup keys */

-    dlist_node    cache_elem;        /* list member of per-catcache list */
-
     /*
      * Lookup keys for the entry, with the first nkeys elements being valid.
      * All by-reference are separately allocated.
diff --git a/src/include/utils/plancache.h b/src/include/utils/plancache.h
index ff09c63a02..afacd94924 100644
--- a/src/include/utils/plancache.h
+++ b/src/include/utils/plancache.h
@@ -95,6 +95,9 @@ extern int    plan_cache_mode;
  */
 typedef struct CachedPlanSource
 {
+    /* If CachedPlanSource has been saved, it is a member of a global list */
+    dlist_node    node;            /* list link, if is_saved */
+
     int            magic;            /* should equal CACHEDPLANSOURCE_MAGIC */
     struct RawStmt *raw_parse_tree; /* output of raw_parser(), or NULL */
     const char *query_string;    /* source text of query */
@@ -125,8 +128,6 @@ typedef struct CachedPlanSource
     bool        is_saved;        /* has CachedPlanSource been "saved"? */
     bool        is_valid;        /* is the query_list currently valid? */
     int            generation;        /* increments each time we create a plan */
-    /* If CachedPlanSource has been saved, it is a member of a global list */
-    dlist_node    node;            /* list link, if is_saved */
     /* State kept to help decide whether to use custom or generic plans: */
     double        generic_cost;    /* cost of generic plan, or -1 if not known */
     double        total_custom_cost;    /* total cost of custom plans so far */
@@ -174,6 +175,8 @@ typedef struct CachedPlan
  */
 typedef struct CachedExpression
 {
+    dlist_node    node;            /* link in global list of CachedExpressions */
+
     int            magic;            /* should equal CACHEDEXPR_MAGIC */
     Node       *expr;            /* planned form of expression */
     bool        is_valid;        /* is the expression still valid? */
@@ -181,7 +184,6 @@ typedef struct CachedExpression
     List       *relationOids;    /* OIDs of relations the expr depends on */
     List       *invalItems;        /* other dependencies, as PlanInvalItems */
     MemoryContext context;        /* context containing this CachedExpression */
-    dlist_node    node;            /* link in global list of CachedExpressions */
 } CachedExpression;


diff --git a/src/tools/valgrind.supp b/src/tools/valgrind.supp
index e3a179d210..c399b787fa 100644
--- a/src/tools/valgrind.supp
+++ b/src/tools/valgrind.supp
@@ -198,3 +198,22 @@
    Memcheck:Cond
    fun:PyObject_Realloc
 }
+
+# Temporary hack to ignore leakage in pl/pgsql compiler
+{
+   ignore-plpgsql-leaks
+   Memcheck:Leak
+   ...
+   fun:do_compile
+}
+
+# Snowball likes to work with a pointer that doesn't point to the start
+# of its palloc chunk; see create_s/lose_s.
+{
+   ignore-snowball-weirdness
+   Memcheck:Leak
+   match-leak-kinds: possible
+   fun:palloc
+   fun:create_s
+   fun:SN_create_env
+}