On Thu, Apr 4, 2024 at 6:43 AM Jelte Fennema-Nio <postg...@jeltef.nl> wrote: > But I don't agree with this. Having pg_unreachable in places where it > brings no perf benefit has two main downsides: > > 1. It's extra lines of code > 2. If a programmer/reviewer is not careful about maintaining this > unreachable invariant (now or in the future), undefined behavior can > be introduced quite easily. > > Also, I'd expect any optimizing compiler to know that the code after > the loop is already unreachable if there are no break/goto statements > in its body.
I'm not super-well-informed about the effects of pg_unreachable(), but I feel like it's a little strange to complain that it's an extra line of code. Presumably, it translates into no executable instructions, and might even reduce the size of the generated code. Now, it could still be a cognitive burden on human readers, but hopefully it should do the exact opposite: it should make the code easier to understand by documenting that the author thought that a certain point in the code could not be reached. In that sense, it's like a code comment. Likewise, it certainly is true that one must be careful to put pg_unreachable() only in places that aren't reachable, and to add or remove it as appropriate if the set of unreachable places changes. But it's also true that one must be careful to put any other thing that looks like a function call only in places where that thing is appropriate. > > I agree with Tristan's analysis of 0002. > > Updated 0002, to only change the comment. But honestly I don't think > using "default" really introduces many chances for future bugs in this > case, since it seems very unlikely we'll ever add new variants to the > PostgresPollingStatusType enum. That might be true, but we've avoided using default: for switches over lots of other enum types in lots of other places in PostgreSQL, and overall it's saved us from lots of bugs. I'm not a stickler about this; if there's some reason not to do it in some particular case, that's fine with me. But where it's possible to do it without any real disadvantage, I think it's a healthy practice. I have committed these versions of the patches. -- Robert Haas EDB: http://www.enterprisedb.com
