Re: Proposal for Signal Detection Refactoring

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