On 2019-Nov-13, Alvaro Herrera wrote:
> On 2019-Nov-13, Tom Lane wrote:
>
> > (BTW, a different question one could ask is exactly why
> > RelationBuildPartitionDesc is so profligate of leaked memory.)
>
> The original partitioning code (f0e44751d717) decided that it didn't
> want to bother with adding a "free" routine for PartitionBoundInfo
> structs, maybe because it had too many pointers, so there's no way for
> RelationBuildPartitionDesc to free everything it allocates anyway. We
> could add a couple of pfrees and list_frees here and there, but for the
> main thing being leaked we'd need to improve that API.
Ah, we also leak an array of PartitionBoundSpec, which is a Node. Do we
have any way to free those? I don't think we do.
In short, it looks to me as if this function was explicitly designed
with the idea that it'd be called in a temp mem context.
I looked at d3f48dfae42f again per your earlier suggestion. Doing that
memory context dance for partitioned relations does seem to fix the
problem too; we just need to move the context creation to just after
ScanPgRelation, at which point we have the relkind. (Note: I think the
problematic case is the partitioned table, not the partitions
themselves. At least, with the attached patch the problem goes away. I
guess it would be sensible to research whether we need to do this for
relispartition=true as well, but I haven't done that.)
There is indeed some leakage for relations that have triggers too (or
rules), but in order for those to become significant you would have to
have thousands of triggers or rules ... and in reasonable designs, you
just don't because it doesn't make sense. But it is not totally
unreasonable to have lots of partitions, and as we improve the system,
more and more people will want to.
Aside: while messing with this I noticed that how significant pg_strtok
is as a resource hog when building partition descs (from the
stringToNode that's applied to each partition's partbound.)
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index e8d11a1d0e..de6d6b3555 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -1029,28 +1029,8 @@ RelationBuildDesc(Oid targetRelId, bool insertIt)
Oid relid;
HeapTuple pg_class_tuple;
Form_pg_class relp;
-
- /*
- * This function and its subroutines can allocate a good deal of transient
- * data in CurrentMemoryContext. Traditionally we've just leaked that
- * data, reasoning that the caller's context is at worst of transaction
- * scope, and relcache loads shouldn't happen so often that it's essential
- * to recover transient data before end of statement/transaction. However
- * that's definitely not true in clobber-cache test builds, and perhaps
- * it's not true in other cases. If RECOVER_RELATION_BUILD_MEMORY is not
- * zero, arrange to allocate the junk in a temporary context that we'll
- * free before returning. Make it a child of caller's context so that it
- * will get cleaned up appropriately if we error out partway through.
- */
-#if RECOVER_RELATION_BUILD_MEMORY
- MemoryContext tmpcxt;
- MemoryContext oldcxt;
-
- tmpcxt = AllocSetContextCreate(CurrentMemoryContext,
- "RelationBuildDesc workspace",
- ALLOCSET_DEFAULT_SIZES);
- oldcxt = MemoryContextSwitchTo(tmpcxt);
-#endif
+ MemoryContext tmpcxt = NULL;
+ MemoryContext oldcxt = NULL;
/*
* find the tuple in pg_class corresponding to the given relation id
@@ -1061,14 +1041,7 @@ RelationBuildDesc(Oid targetRelId, bool insertIt)
* if no such tuple exists, return NULL
*/
if (!HeapTupleIsValid(pg_class_tuple))
- {
-#if RECOVER_RELATION_BUILD_MEMORY
- /* Return to caller's context, and blow away the temporary context */
- MemoryContextSwitchTo(oldcxt);
- MemoryContextDelete(tmpcxt);
-#endif
return NULL;
- }
/*
* get information from the pg_class_tuple
@@ -1077,6 +1050,33 @@ RelationBuildDesc(Oid targetRelId, bool insertIt)
relid = relp->oid;
Assert(relid == targetRelId);
+ /*
+ * This function and its subroutines can allocate a good deal of transient
+ * data in CurrentMemoryContext. Traditionally we've just leaked that
+ * data, reasoning that the caller's context is at worst of transaction
+ * scope, and relcache loads shouldn't happen so often that it's essential
+ * to recover transient data before end of statement/transaction. However
+ * that's definitely not true in clobber-cache test builds, and perhaps
+ * it's not true in other cases. If RECOVER_RELATION_BUILD_MEMORY is not
+ * zero, arrange to allocate the junk in a temporary context that we'll
+ * free before returning. Make it a child of caller's context so that it
+ * will get cleaned up appropriately if we error out partway through.
+ *
+ * Partitioned tables are notoriously leaky to build, so we do this for
+ * them always.
+ */
+ if ((relp->relkind == RELKIND_PARTITIONED_TABLE)
+#if RECOVER_RELATION_BUILD_MEMORY
+ || true
+#endif
+ )
+ {
+ tmpcxt = AllocSetContextCreate(CurrentMemoryContext,
+ "RelationBuildDesc workspace",
+ ALLOCSET_DEFAULT_SIZES);
+ oldcxt = MemoryContextSwitchTo(tmpcxt);
+ }
+
/*
* allocate storage for the relation descriptor, and copy pg_class_tuple
* to relation->rd_rel.
@@ -1252,11 +1252,16 @@ RelationBuildDesc(Oid targetRelId, bool insertIt)
/* It's fully valid */
relation->rd_isvalid = true;
+ if ((relation->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
#if RECOVER_RELATION_BUILD_MEMORY
- /* Return to caller's context, and blow away the temporary context */
- MemoryContextSwitchTo(oldcxt);
- MemoryContextDelete(tmpcxt);
+ || true
#endif
+ )
+ {
+ /* Return to caller's context, and blow away the temporary context */
+ MemoryContextSwitchTo(oldcxt);
+ MemoryContextDelete(tmpcxt);
+ }
return relation;
}
