On Thu, Mar 14, 2019 at 12:56 PM Tom Lane <tgl@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