On Tue, Jan 28, 2020 at 2:29 PM Tom Lane <tgl@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
