Hi, On 2019-01-17 10:50:56 +0100, Chris Travers wrote: > diff --git a/src/backend/utils/init/globals.c > b/src/backend/utils/init/globals.c > index c6939779b9..5ed715589e 100644 > --- a/src/backend/utils/init/globals.c > +++ b/src/backend/utils/init/globals.c > @@ -27,12 +27,35 @@ > > ProtocolVersion FrontendProtocol; > > +/* > + * Signal Handling variables here are set in signal handlers and can be > + * checked by code to determine the implications of calling > + * CHECK_FOR_INTERRUPTS(). While all are supported, in practice > + * InterruptPending, QueryCancelPending, and ProcDiePending appear to be > + * sufficient for almost all use cases. > + */
Especially the latter part of comment seems like a bad idea. > +/* Whether CHECK_FOR_INTERRUPTS will do anything */ > volatile sig_atomic_t InterruptPending = false; That's not actually a correct description. CFI is dependent on other things than just InterruptPending, namely HOLD_INTERRUPTS() (even though it's inside ProcessInterrupts()). It also always does more on windows. I think the variable name is at least as good a description as the comment you added. > +/* Whether we are planning on cancelling the current query */ > volatile sig_atomic_t QueryCancelPending = false; > +/* Whether we are planning to exit the current process when safe to do so.*/ > volatile sig_atomic_t ProcDiePending = false; > + > +/* Whether we have detected a lost client connection */ > volatile sig_atomic_t ClientConnectionLost = false; > + > +/* > + * Whether the process is dying because idle transaction timeout has been > + * reached. > + */ > volatile sig_atomic_t IdleInTransactionSessionTimeoutPending = false; > + > +/* Whether we will reload the configuration at next opportunity */ > volatile sig_atomic_t ConfigReloadPending = false; > + These comments all seem to add no information above the variable name. I'm not quite understanding what this patch is intended to solve, sorry. Greetings, Andres Freund
