hyrax vs. RelationBuildPartitionDesc

Hi,

Amit Kapila pointed out to be that there are some buidfarm failures on
hyrax which seem to have started happening around the time I committed
898e5e3290a72d288923260143930fb32036c00c. It failed like this once:

2019-03-07 19:57:40.231 EST [28073:11] DETAIL: Failed process was
running: /* same as above */
explain (costs off) select * from rlp where a = 1::numeric;

And then the next three runs failed like this:

2019-03-09 04:56:04.939 EST [1727:4] LOG: server process (PID 2898)
was terminated by signal 9: Killed
2019-03-09 04:56:04.939 EST [1727:5] DETAIL: Failed process was
running: UPDATE range_parted set c = (case when c = 96 then 110 else c
+ 1 end ) WHERE a = 'b' and b > 10 and c >= 96;

I couldn't think of an explanation for this other than that the
process involved had used a lot of memory and gotten killed by the OOM
killer, and it turns out that RelationBuildPartitionDesc leaks memory
into the surrounding context every time it's called. It's not a lot
of memory, but in a CLOBBER_CACHE_ALWAYS build it adds up, because
this function gets called a lot. All of this is true even before the
commit in question, but for some reason that I don't understand that
commit makes it worse.

I tried having that function use a temporary context for its scratch
space and that causes a massive drop in memory usage when running
update.sql frmo the regression tests under CLOBBER_CACHE_ALWAYS. I
ran MacOS X's vmmap tool to see the impact, measuring the size of the
"DefaultMallocZone". Without this patch, that peaks at >450MB; with
this patch it peaks ~270MB. There is a significant reduct in typical
memory usage, too. It's noticeably better with this patch than it was
before 898e5e3290a72d288923260143930fb32036c00c.

I'm not sure whether this is the right way to address the problem.
RelationBuildPartitionDesc() creates basically all of the data
structures it needs and then copies them into rel->rd_pdcxt, which has
always seemed a bit inefficient to me. Another way to redesign this
would be to have the function create a temporary context, do all of
its work there, and then reparent the context under CacheMemoryContext
at the end. That means any leaks would go into a relatively
long-lifespan context, but on the other hand you wouldn't leak into
the same context a zillion times over, and you'd save the expense of
copying everything. I think that the biggest thing that is being
copied around here is the partition bounds, so maybe the leak wouldn't
amount to much, and we could also do things like list_free(inhoids) to
make it a little tighter.

Opinions? Suggestions?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company