Protect against hypothetical memory leaks in RelationGetPartitionKey

Also, fix a comment that commit 8a0596cb656e made obsolete.

Reported-by: Robert Haas
Discussion: http://postgr.es/m/CA+TgmoYbpuUUUp2GhYNwWm0qkah39spiU7uOiNXLz20ASfKYoA@mail.gmail.com

diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index e2760daac4c1d9ed88f4d693c260c5acfdd09923..1d0cc6cb7981eba29b0d8af35f206a785858f8a1 100644 (file)
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -807,17 +807,16 @@ RelationBuildRuleLock(Relation relation)
  * RelationBuildPartitionKey
  *             Build and attach to relcache partition key data of relation
  *
- * Partitioning key data is stored in CacheMemoryContext to ensure it survives
- * as long as the relcache.  To avoid leaking memory in that context in case
- * of an error partway through this function, we build the structure in the
- * working context (which must be short-lived) and copy the completed
- * structure into the cache memory.
- *
- * Also, since the structure being created here is sufficiently complex, we
- * make a private child context of CacheMemoryContext for each relation that
- * has associated partition key information.  That means no complicated logic
- * to free individual elements whenever the relcache entry is flushed - just
- * delete the context.
+ * Partitioning key data is a complex structure; to avoid complicated logic to
+ * free individual elements whenever the relcache entry is flushed, we give it
+ * its own memory context, child of CacheMemoryContext, which can easily be
+ * deleted on its own.  To avoid leaking memory in that context in case of an
+ * error partway through this function, the context is initially created as a
+ * child of CurTransactionContext and only re-parented to CacheMemoryContext
+ * at the end, when no further errors are possible.  Also, we don't make this
+ * context the current context except in very brief code sections, out of fear
+ * that some of our callees allocate memory on their own which would be leaked
+ * permanently.
  */
 static void
 RelationBuildPartitionKey(Relation relation)
@@ -850,9 +849,9 @@ RelationBuildPartitionKey(Relation relation)
                                                                                           RelationGetRelationName(relation),
                                                                                           MEMCONTEXT_COPY_NAME,
                                                                                           ALLOCSET_SMALL_SIZES);
-       oldcxt = MemoryContextSwitchTo(partkeycxt);
 
-       key = (PartitionKey) palloc0(sizeof(PartitionKeyData));
+       key = (PartitionKey) MemoryContextAllocZero(partkeycxt,
+                                                                                               sizeof(PartitionKeyData));
 
        /* Fixed-length attributes */
        form = (Form_pg_partitioned_table) GETSTRUCT(tuple);
@@ -894,17 +893,20 @@ RelationBuildPartitionKey(Relation relation)
                /*
                 * Run the expressions through const-simplification since the planner
                 * will be comparing them to similarly-processed qual clause operands,
-                * and may fail to detect valid matches without this step.  We don't
-                * need to bother with canonicalize_qual() though, because partition
-                * expressions are not full-fledged qualification clauses.
+                * and may fail to detect valid matches without this step; fix
+                * opfuncids while at it.  We don't need to bother with
+                * canonicalize_qual() though, because partition expressions are not
+                * full-fledged qualification clauses.
                 */
-               expr = eval_const_expressions(NULL, (Node *) expr);
+               expr = eval_const_expressions(NULL, expr);
+               fix_opfuncids(expr);
 
-               /* May as well fix opfuncids too */
-               fix_opfuncids((Node *) expr);
-               key->partexprs = (List *) expr;
+               oldcxt = MemoryContextSwitchTo(partkeycxt);
+               key->partexprs = (List *) copyObject(expr);
+               MemoryContextSwitchTo(oldcxt);
        }
 
+       oldcxt = MemoryContextSwitchTo(partkeycxt);
        key->partattrs = (AttrNumber *) palloc0(key->partnatts * sizeof(AttrNumber));
        key->partopfamily = (Oid *) palloc0(key->partnatts * sizeof(Oid));
        key->partopcintype = (Oid *) palloc0(key->partnatts * sizeof(Oid));
@@ -919,8 +921,9 @@ RelationBuildPartitionKey(Relation relation)
        key->parttypbyval = (bool *) palloc0(key->partnatts * sizeof(bool));
        key->parttypalign = (char *) palloc0(key->partnatts * sizeof(char));
        key->parttypcoll = (Oid *) palloc0(key->partnatts * sizeof(Oid));
+       MemoryContextSwitchTo(oldcxt);
 
-       /* For the hash partitioning, an extended hash function will be used. */
+       /* determine support function number to search for */
        procnum = (key->strategy == PARTITION_STRATEGY_HASH) ?
                HASHEXTENDED_PROC : BTORDER_PROC;
 
@@ -952,7 +955,7 @@ RelationBuildPartitionKey(Relation relation)
                if (!OidIsValid(funcid))
                        ereport(ERROR,
                                        (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
-                                        errmsg("operator class \"%s\" of access method %s is missing support function %d for data type \"%s\"",
+                                        errmsg("operator class \"%s\" of access method %s is missing support function %d for type %s",
                                                        NameStr(opclassform->opcname),
                                                        (key->strategy == PARTITION_STRATEGY_HASH) ?
                                                        "hash" : "btree",
@@ -989,11 +992,13 @@ RelationBuildPartitionKey(Relation relation)
 
        ReleaseSysCache(tuple);
 
-       /* Success --- make the relcache point to the newly constructed key */
+       /*
+        * Success --- reparent our context and make the relcache point to the
+        * newly constructed key
+        */
        MemoryContextSetParent(partkeycxt, CacheMemoryContext);
        relation->rd_partkeycxt = partkeycxt;
        relation->rd_partkey = key;
-       MemoryContextSwitchTo(oldcxt);
 }
 
 /*

diff --git a/src/include/access/hash.h b/src/include/access/hash.h
index ccdf9dff4bb0070db8b1920b76cae5e8a74a5d98..179ac97f8765f222d1c9ea659f4dfe67bf749054 100644 (file)
--- a/src/include/access/hash.h
+++ b/src/include/access/hash.h
@@ -338,7 +338,7 @@ typedef HashMetaPageData *HashMetaPage;
 
 /*
  * When a new operator class is declared, we require that the user supply
- * us with an amproc procudure for hashing a key of the new type, returning
+ * us with an amproc procedure for hashing a key of the new type, returning
  * a 32-bit hash value.  We call this the "standard" hash procedure.  We
  * also allow an optional "extended" hash procedure which accepts a salt and
  * returns a 64-bit hash value.  This is highly recommended but, for reasons