Robert Haas wrote:
> On Thu, Dec 21, 2017 at 12:43 PM, Alvaro Herrera
> <alvherre@alvh.no-ip.org> wrote:
> > Get rid of copy_partition_key
> >
> > That function currently exists to avoid leaking memory in
> > CacheMemoryContext in case of trouble while the partition key is being
> > built, but there's a better way: allocate everything in a memcxt that
> > goes away if the current (sub)transaction fails, and once the partition
> > key is built and no further errors can occur, make the memcxt permanent
> > by making it a child of CacheMemoryContext.
>
> I'm not saying this commit is wrong, but it's a weaker form of
> protection than we had before. For instance, suppose that while
> building the partition key we do a bunch of random palloc's that are
> only used transiently when constructing the key. In the old coding,
> those go into the temp context and get blown up immediately; only what
> we copy to the new context ends up there. In the new way, they stick
> around for as long as the relcache entry does.
True. We're careful to avoid leaks, so I'm not terribly worried TBH.
Example: in the original coding we were pfree'ing exprString, even
though that was mostly pointless since (I think) it would just be freed
with some short-lived context deletion shortly afterwards.
Compare RelationBuildRuleLock. It doesn't have the problem you describe
because it uses MemoryContextAlloc instead of palloc for the things it
wants to persist. Maybe we should do that in RelationBuildPartitionKey
too. Best of both worlds I think.
(At the same time, we should copy in the other direction the idea
of reparenting a context initially made temporary.)
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services