On Tue, Jan 28, 2020 at 2:29 PM Tom Lane <t...@sss.pgh.pa.us> wrote: > Well, yeah, that's exactly my point. But in my book, "refuse to do > anything" should be "elog(ERROR)", not "invoke undefined behavior". > An actual abort() call might be all right here, in that at least > we'd know what would happen and we could debug it once we got hold > of a stack trace. But pg_unreachable() is not that. Basically, if > there's *any* circumstance, broken code or not, where control could > reach a pg_unreachable() call, you did it wrong.
I don't really agree. I think such defensive coding is more than justified when the input is coming from a file on disk or some other external source where it might have been corrupted. For instance, I think the fact that the code which deforms heap tuples will cheerfully sail off the end of the buffer or seg fault if the tuple descriptor doesn't match the tuple is a seriously bad thing. It results in actual production crashes that could be avoided with more defensive coding. Admittedly, there would likely be a performance cost, which might not be a reason to do it, but if that cost is small I would probably vote for paying it, because this is something that actually happens to users on a pretty regular basis. In the case at hand, though, there are no constants of type CommandDest that come from any place other than a constant in the program text, and it seems unlikely that this will ever be different in the future. So, how could we ever end up with a value that's not in the enum? I guess the program text itself could be corrupted, but we cannot defend against that. Mind you, I'm not going to put up a huge stink if you're bound and determined to go change this. I prefer it the way that it is, and I think that preference is well-justified by facts on the ground, but I don't think it's worth fighting about. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
