Robert Haas wrote: > On Thu, Dec 21, 2017 at 12:43 PM, Alvaro Herrera > <alvhe...@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
