Re: Proposal for Signal Detection Refactoring

On Mon, Sep 24, 2018 at 7:06 PM Andres Freund <andres(at)anarazel(dot)de> wrote:

> On 2018-09-24 11:45:10 +0200, Chris Travers wrote:
> > I did some more reading.
> >
> > On Mon, Sep 24, 2018 at 10:15 AM Chris Travers <chris(dot)travers(at)adjust(dot)com
> >
> > wrote:
> >
> > > First, thanks for taking the time to write this. Its very helpful.
> > > Additional thoughts inline.
> > >
> > > On Mon, Sep 24, 2018 at 2:12 AM Michael Paquier <michael(at)paquier(dot)xyz>
> > > wrote:
> > >
> > >>
> > >> There could be value in refactoring things so as all the *Pending
> flags
> > >> of miscadmin.h get stored into one single volatile sig_atomic_t which
> > >> uses bit-wise markers, as that's at least 4 bytes because that's
> stored
> > >> as an int for most platforms and can be performed as an atomic
> operation
> > >> safely across signals (If my memory is right;) ). And this leaves a
> lot
> > >> of room for future flags.
> > >>
> > >
> > > Yeah I will look into this.
> > >
> >
> >
> > Ok so having looked into this a bit more....
> >
> > It looks like to be strictly conforming, you can't just use a series of
> > flags because neither C 89 or 99 guarantee that sig_atomic_t is
> read/write
> > round-trip safe in signal handlers. In other words, if you read, are
> > pre-empted by another signal, and then write, you may clobber what the
> > other signal handler did to the variable. So you need atomics, which are
> > C11....
> >
> > What I would suggest instead at least for an initial approach is:
> >
> > 1. A struct of volatile bools statically stored
> > 2. macros for accessing/setting/clearing flags
> > 3. Consistent use of these macros throughout the codebase.
> >
> > To make your solution work it looks like we'd need C11 atomics which
> would
> > be nice and maybe at some point we decide to allow newer feature, or we
> > could wrap this itself in checks for C11 features and provide atomic
> flags
> > in the future. It seems that the above solution would strictly comply to
> > C89 and pose no concurrency issues.
>
> It's certainly *NOT* ok to use atomics in signal handlers
> indiscriminately, on some hardware / configure option combinations
> they're backed by spinlocks (or even semaphores) and thus *NOT*
> interrupt save.
>
> This doesn't seem to solve an actual problem, why are we discussing
> changing this? What'd be measurably improved, worth the cost of making
> backpatching more painful?

I am not sure if this is measurable as a problem but I am fairly sure this
is tangible and is likely to become more so over time.

Right now, the current approach is to use a series of globally defined
single variables for handling interrupt conditions. Because this has grown
organically over time, the sort naming of these variables doesn't have a
hard degree of consistency. Consequently, if one has code which needs to
check why it was interrupted, this is both a fragile process, and one which
imposes a dependency directly on internals. We now have two different
areas in the code which do handling of interrupt conditions, and two more
places that non-intrusively check for whether certain kinds of interrupt
conditions are pending.

My sense is that the number of places which have to be aware of these sort
of things is likely to grow in the future. Therefore I think it would be a
good idea sooner rather than later to ensure that the both the structures
and the mechanisms and interfaces are forward-looking, and make stability
guarantees we can hold well into the future without burdening code
maintenance much. So maybe there is some short-term pain in back porting
but the goal is to make sure that long-term backporting is easier, and that
checking pending interrupt status is safer.

I am not sold on using bit flags here. I don't think they are C89 or 99
safe (maybe in C11 with caveats).

My suspicion is that when I get to a second attempt at this problem, it
will look much closer to what we have now, just better encapsulated.
--
Best Regards,
Chris Travers
Head of Database

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