Re: hyrax vs. RelationBuildPartitionDesc

On Thu, Mar 14, 2019 at 6:08 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> I found that even with 2455ab488, there still seemed to be an
> unreasonable amount of MessageContext bloat during some of the queries,
> on the order of 50MB in some of them. Investigating more closely,
> I found that 2455ab488 has a couple of simple oversights: it's still
> calling partition_bounds_create in the caller's context, allowing
> whatever that builds or leaks to be a MessageContext leak.

Oops.

> And it's
> still calling get_rel_relkind() in the rd_pdcxt context, potentially
> leaking a *lot* of stuff into that normally-long-lived context, since
> that will result in fresh catcache (and thence potentially relcache)
> loads in some cases.

That's really unfortunate. I know CLOBBER_CACHE_ALWAYS testing has a
lot of value, but get_rel_relkind() is the sort of function that
developers need to be able to call without worrying with creating a
massive memory leak. Maybe the only time it is really going to get
called enough to matter is from within the cache invalidation
machinery itself, in which case your idea will fix it. But I'm not
sure about that.

> What I'm thinking, therefore, is that 2455ab488 had the right idea but
> didn't take it far enough. We should remove the temp-context logic it
> added to RelationBuildPartitionDesc and instead put that one level up,
> in RelationBuildDesc, where the same temp context can serve all of these
> leak-prone sub-facilities.

Yeah, that sounds good.

> Possibly it'd make sense to conditionally compile this so that we only
> do it in a CLOBBER_CACHE_ALWAYS build. I'm not very sure about that,
> but arguably in a normal build the overhead of making and destroying
> a context would outweigh the cost of the leaked memory. The main
> argument I can think of for doing it all the time is that having memory
> allocation work noticeably differently in CCA builds than normal ones
> seems like a recipe for masking normal-mode bugs from the CCA animals.
> But that's only a guess not proven fact; it's also possible that having
> two different behaviors would expose bugs we'd otherwise have a hard
> time detecting, such as continuing to rely on the "temporary" data
> structures longer than we should.

I lean toward thinking it makes more sense to be consistent, but I'm
not sure that's right.

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