Re: Proposal for Signal Detection Refactoring

From: Andres Freund <andres(at)anarazel(dot)de>
To: Chris Travers <chris(dot)travers(at)adjust(dot)com>
Cc: pgsql-hackers(at)lists(dot)postgresql(dot)org
Subject: Re: Proposal for Signal Detection Refactoring
Date: 2019-01-17 17:38:09
Message-ID: 20190117173809.pqs36vsbffjzkahy@alap3.anarazel.de
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message John Naylor 2019-01-17 17:43:36 Re: WIP: Avoid creation of the free space map for small tables
Previous Message Andres Freund 2019-01-17 17:29:04 Re: ArchiveEntry optional arguments refactoring