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: michael(at)paquier(dot)xyz, pgsql-hackers(at)lists(dot)postgresql(dot)org
Subject: Re: Proposal for Signal Detection Refactoring
Date: 2018-09-24 17:06:40
Message-ID: 20180924170640.5zpraws5unufwifk@alap3.anarazel.de
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

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?

Greetings,

Andres Freund

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2018-09-24 17:18:47 Re: Allowing printf("%m") only where it actually works
Previous Message Elvis Pranskevichus 2018-09-24 16:28:49 Re: [HACKERS] [PATCH v2] Add and report the new "session_read_only" GUC pseudo-variable.