Re: psql not responding to SIGINT upon db reconnection
Robert Haas <robertmhaas@gmail.com>
From: Robert Haas <robertmhaas@gmail.com>
To: Jelte Fennema-Nio <postgres@jeltef.nl>
Cc: Tristan Partin <tristan@neon.tech>, Heikki Linnakangas <hlinnaka@iki.fi>,
pgsql-hackers@postgresql.org, Shlok Kyal <shlok.kyal.oss@gmail.com>
Date: 2024-04-04T20:32:35Z
Lists: pgsql-hackers
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
Commits
Same data as JSON:
GET /api/v1/messages/:b64id/commits
the thread's linked commits as JSON, with link sources.
API reference →
-
Fix old, misleading comment for PGRES_POLLING_ACTIVE.
- e57fe3824ee7 17.0 landed
-
Remove reachable call to pg_unreachable().
- 12b964d7815b 17.0 landed
-
Allow SIGINT to cancel psql database reconnections.
- cafe1056558f 17.0 landed
-
Expose PQsocketPoll via libpq
- f5e4dedfa81f 17.0 landed
-
Fix query cancellation handling in psql
- 5d43c3c54d77 13.0 cited