On Thu, Mar 14, 2019 at 12:56 PM Tom Lane <t...@sss.pgh.pa.us> wrote: > Agreed, but the comments in this area are crap. Why doesn't > CreatePartitionDirectory say something like > > * The object lives inside the given memory context and will be > * freed when that context is destroyed. Nonetheless, the caller > * must *also* ensure that (unless the transaction is aborted) > * DestroyPartitionDirectory is called before that happens, else > * we may leak some relcache reference counts. > > It's completely not acceptable that every reader of this code should > have to reverse-engineer these design assumptions, especially given > how shaky they are.
Well, one reason is that everything you just said is basically self-evident. If you spend 5 seconds looking at the header file, you'll see that there is a CreatePartitionDirectory() function and a DestroyPartitionDirectory() function, and so you'll probably figure out that the latter is intended to be called rather than just ignored. You will probably also guess that it doesn't need to be called if there's an ERROR, just like basically everything else in PostgreSQL. And if you want to know why, you can look at the code and you shouldn't have any trouble determining that it releases relcache ref counts, which may tip you off that if you don't call it, some relcache refcounts will not be released. But, look, I wrote the code. What's clear to me may not be clear to everybody. I have to admit I'm kinda surprised that this is the thing that is confusing to anybody, but if it is, then sure, let's add the comment! > There's an independent question as to whether the planner's use of > the feature is specifying a safe memory context. Has this code been > exercised under GEQO? Probably not. That's a good idea. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company