On Thu, Mar 14, 2019 at 1:40 PM Tom Lane <t...@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
