Обсуждение: Re: pgsql: Get rid of copy_partition_key
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. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
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
I believe this patch (which also fixes a comment I neglected to fix in the previous one) should satisfy your concerns. It's still running a few relevant tests (create_function_1 create_type point polygon circle create_table copy create_misc create_index alter_table partition_join partition_prune hash_part) under CLOBBER_FREED_MEMORY + CLOBBER_CACHE_ALWAYS, but so far it's looking good. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Вложения
Seems I misremembered the whole opfuncid getting reset thing (it applies to reading a node from string, not copying) and it was undone by 9f1255ac8593 anyway. I don't think it makes much of a difference, but I mention this in case you're wondering why I changed the fix_opfuncids() call. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Pushed, thanks. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services