On Thu, Apr 4, 2024 at 6:43 AM Jelte Fennema-Nio <postgres@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