Re: Proposal for Signal Detection Refactoring

On Thu, Jan 17, 2019 at 6:38 PM Andres Freund <andres(at)anarazel(dot)de> wrote:

> 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.
>

I would like to see the fact that checking these global fields is the
correct way of understanding what CHECK_FOR_INTERRUPTS will do before you
do it as per the two or three places in the code where this is already done.

I don't like reasoning about acceptable coding practices based on "someone
has already committed something like this before." I found that difficult
when I was trying to fix the race condition and thought this would help
others in similar cases.

Keep in mind that C extensions might use internal functions etc. perhaps in
ways which are different from what the main source code does. Therefore
what is safe to do, and what we are not willing to guarantee as safe should
probably be documented.

>
> Greetings,
>
> Andres Freund
>

--
Best Regards,
Chris Travers
Head of Database

Tel: +49 162 9037 210 | Skype: einhverfr | www.adjust.com
Saarbrücker Straße 37a, 10405 Berlin