On Thu, Mar 14, 2019 at 1:40 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> So here's my problem with that argument: you're effectively saying that
> you needn't write any API spec for the PartitionDirectory functions
> because you intend that every person calling them will read their code,
> carefully and fully, before using them. This is not my idea of sound
> software engineering. If you need me to spell out why not, I will do
> so, but I'd like to think that I needn't explain abstraction to a senior
> committer.
I think you're attacking a straw man. I expect you don't seriously
believe that I lack an understanding of abstraction. However,
abstraction doesn't mean that the comment for CreatePartitionDirectory
must describe what DestroyPartitionDirectory is going to do
internally, as you seem to be proposing. Had I thought about this
issue more sooner, I think I would have guessed you would be opposed
to such a comment, since the chances of someone neglecting to update
it when changing DestroyPartitionDirectory seem to be non-negligible.
At the same time, and as I already said, I am also fine to improve the
comments for these functions. The fact that both you and Amit found
them inadequate - albeit in somewhat different ways - shows that they
need improvement. However, that doesn't mean that what I did was
flagrantly unreasonable or that I'm full of crap. Those things may be
true, but this isn't believable evidence of it.
If we're going to start talking about comments that make inadequate
mention of important memory management details, I think a much better
place to start than anything that I did in this commit would be the
get_relation_info() function we were just discussing in a different
part of this email thread. As I said over there, people keep failing
to understand that any data you want to use during query planning has
got to be copied out of the relcache in that function -- and this is a
pretty subtle hazard, actually, because it works fine unless you get a
cache flush at the wrong time or build with CLOBBER_CACHE_ALWAYS.
Failing to call DestroyPartitionDirectory() breaks in a much more
obvious way.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
