On Wed, Mar 13, 2019 at 2:21 PM Alvaro Herrera <alvhe...@2ndquadrant.com> wrote: > On 2019-Mar-13, Robert Haas wrote: > > On Wed, Mar 13, 2019 at 1:38 PM Alvaro Herrera <alvhe...@2ndquadrant.com> > > wrote: > > > A bit, yes, but not overly so, and it's less fragile that not having > > > such a protection. Anything that allocates in CacheMemoryContext needs > > > to be very careful anyway. > > > > True, but I think it's more fragile than either of the options I proposed. > > You do? Unless I misunderstood, your options are: > > 1. (the patch you attached) create a temporary memory context that is > used for everything, then at the end copy the good stuff to CacheMemCxt > (or a sub-context thereof). This still needs to copy. > > 2. create a temp memory context, do everything there, do retail freeing > of everything we don't want, reparenting the context to CacheMemCxt. > Hope we didn't forget to pfree anything. > > How is any of those superior to what I propose?
Well, *I* thought of it, so obviously it must be superior. :-) More seriously, your idea does seem better in some ways. My #1 doesn't avoid the copy, but does kill the leaks. My #2 avoids the copy, but risks a different flavor of leakage. Your idea also avoids the copy and leaks in fewer cases than my #2. In that sense, it is the technically superior option. However, it also requires more memory context switches than either of my options, and I guess that seems fragile to me in the sense that I think future code changes are more likely to go wrong due to the complexity involved. I might be mistaken about that, though. One other note is that, although the extra copy looks irksome, it's probably not very significant from a performance point of view. In a non-CLOBBER_CACHE_ALWAYS build it doesn't happen frequently enough to matter, and in a CLOBBER_CACHE_ALWAYS build everything is already so slow that it still doesn't matter. That's not the only consideration, though. Code which copies data structures might be buggy, or might develop bugs in the future, and avoiding that copy would avoid exposure to such bugs. On the other hand, it's not really possible to remove the copying without increasing the risk of leaking into the long-lived context. In some ways, I think this is all a natural outgrowth of the fact that we rely on palloc() in so many places instead of forcing code to be explicit about which memory context it intends to target. Global variables are considered harmful, and it's not that difficult to see the connection between that general principle and present difficulties. However, I will not have time to write and debug a patch reversing that choice between now and feature freeze. :-) I'm kinda inclined to go for the brute-force approach of slamming the temporary context in there as in the patch I proposed upthread, which should solve the immediate problem, and we can implement one of these other ideas later if we want. What do you think about that? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
