Re: Proposal for Signal Detection Refactoring

On 2019-Mar-06, Chris Travers wrote: > Here's a new patch. No rush on it. I am moving it to next commitfest > anyway because as code documentation I think this is a low priority late in > the release cycle. Chris, are you submitting an updated patch? There's some unaddressed feedback from Tom,

Re: Proposal for Signal Detection Refactoring

On Wed, 6 Mar 2019 at 17:38, Chris Travers wrote: > > Here's a new patch. No rush on it. I am moving it to next commitfest > anyway because as code documentation I think this is a low priority late in > the release cycle. > While you're looking at signal detection changes I suggest making sure

Re: Proposal for Signal Detection Refactoring

On Tue, Jul 30, 2019 at 6:27 AM Tom Lane wrote: > Chris Travers writes: > > Here's a new patch. No rush on it. I am moving it to next commitfest > > anyway because as code documentation I think this is a low priority late in > > the release cycle. > > The changes mostly address Andres's feedba

Re: Proposal for Signal Detection Refactoring

Chris Travers writes: > Here's a new patch. No rush on it. I am moving it to next commitfest > anyway because as code documentation I think this is a low priority late in > the release cycle. > The changes mostly address Andres's feedback above. I took a look through this version. (BTW, pleas

Re: Proposal for Signal Detection Refactoring

Here's a new patch. No rush on it. I am moving it to next commitfest anyway because as code documentation I think this is a low priority late in the release cycle. The changes mostly address Andres's feedback above. -- Best Regards, Chris Travers Head of Database Tel: +49 162 9037 210 | Skyp

Re: Proposal for Signal Detection Refactoring

Hi, On 2019-01-23 11:55:09 +0100, Chris Travers wrote: > +Implementation Notes on Globals and Signal/Event Handling > += > + > +The approch to signal handling in PostgreSQL is designed to strictly conform > +with the C89 standard and designed

Re: Proposal for Signal Detection Refactoring

On Wed, Jan 23, 2019 at 11:55:09AM +0100, Chris Travers wrote: > attached is a new signal handing patch. Path is corrected and moved. The > documentation is sightly streamlined in some places and expanded in others. This has not been reviewed, so moved to next CF. -- Michael signature.asc Desc

Re: Proposal for Signal Detection Refactoring

attached is a new signal handing patch. Path is corrected and moved. The documentation is sightly streamlined in some places and expanded in others. Feedback requested. -- Best Regards, Chris Travers Head of Database Tel: +49 162 9037 210 | Skype: einhverfr | www.adjust.com Saarbrücker Straße

Re: Proposal for Signal Detection Refactoring

On Thu, Jan 17, 2019 at 7:08 PM Andres Freund wrote: > Hi, > > On 2018-10-09 16:04:35 +0200, Peter Eisentraut wrote: > > More generally, I'd like this material to be code comments. It's the > > kind of stuff that gets outdated before long if it's kept separate. > > I'm not sure I buy this here -

Re: Proposal for Signal Detection Refactoring

Hi, On 2018-10-09 16:04:35 +0200, Peter Eisentraut wrote: > More generally, I'd like this material to be code comments. It's the > kind of stuff that gets outdated before long if it's kept separate. I'm not sure I buy this here - we don't have (but perhaps should?) a convenient location for an o

Re: Proposal for Signal Detection Refactoring

On Thu, Jan 17, 2019 at 6:38 PM Andres Freund 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

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 @@ > > Protocol

Re: Proposal for Signal Detection Refactoring

Latest patch, simpler but answers the key questions as code comments On Mon, Dec 3, 2018 at 6:56 AM Chris Travers wrote: > On Thu, Nov 29, 2018 at 8:46 PM Dmitry Dolgov <9erthali...@gmail.com> > wrote: > >> > On Wed, Oct 10, 2018 at 7:10 PM Chris Travers >> wrote: >> > >> >> More generally, I'd

Re: Proposal for Signal Detection Refactoring

On Thu, Nov 29, 2018 at 8:46 PM Dmitry Dolgov <9erthali...@gmail.com> wrote: > > On Wed, Oct 10, 2018 at 7:10 PM Chris Travers > wrote: > > > >> More generally, I'd like this material to be code comments. It's the > >> kind of stuff that gets outdated before long if it's kept separate. > > > > T

Re: Proposal for Signal Detection Refactoring

> On Wed, Oct 10, 2018 at 7:10 PM Chris Travers > wrote: > >> More generally, I'd like this material to be code comments. It's the >> kind of stuff that gets outdated before long if it's kept separate. > > The problem is that code comments are not going to be good places to document > "how do I

Re: Proposal for Signal Detection Refactoring

On Tue, Oct 9, 2018 at 4:04 PM Peter Eisentraut < peter.eisentr...@2ndquadrant.com> wrote: > On 01/10/2018 14:00, Chris Travers wrote: > > > > > > On Wed, Sep 26, 2018 at 9:54 AM Chris Travers > > wrote: > > > > > > > > On Tue, Sep 25, 2018 at 3:23 PM Tom Lane

Re: Proposal for Signal Detection Refactoring

On October 9, 2018 6:58:18 AM PDT, Peter Eisentraut wrote: >On 25/09/2018 02:23, Andres Freund wrote: >>> My point was just to reduce the number of variables used and ease >>> debugger lookups with what is on the stack. >> I'm not sure a bitflag really gives you that - before gdb gives you >th

Re: Proposal for Signal Detection Refactoring

On 01/10/2018 14:00, Chris Travers wrote: > > > On Wed, Sep 26, 2018 at 9:54 AM Chris Travers > wrote: > > > > On Tue, Sep 25, 2018 at 3:23 PM Tom Lane > wrote: > > Chris Travers

Re: Proposal for Signal Detection Refactoring

On 25/09/2018 02:23, Andres Freund wrote: >> My point was just to reduce the number of variables used and ease >> debugger lookups with what is on the stack. > I'm not sure a bitflag really gives you that - before gdb gives you the > plain value, afterwards you need to know the enum values and do b

Re: Proposal for Signal Detection Refactoring

Moving this to documentation due to a general consensus that abstracting this is not necessarily worth it. If we don't want to refactor and abstract this, it is worth documenting the design as to how things work now so that others who face bugs can consult docs instead of trying to determine ac

Re: Proposal for Signal Detection Refactoring

On Wed, Sep 26, 2018 at 9:54 AM Chris Travers wrote: > > > On Tue, Sep 25, 2018 at 3:23 PM Tom Lane wrote: > >> Chris Travers writes: >> > However, what I think one could do is use a struct of volatile >> > sig_atomic_t members and macros for checking/setting. Simply writing a >> > value is s

Re: Proposal for Signal Detection Refactoring

On Wed, Sep 26, 2018 at 09:50:00AM +0200, Chris Travers wrote: > On Wed, Sep 26, 2018 at 12:41 AM Michael Paquier > wrote: >> Agreed. Are there any objections with the proposal of changing the >> interruption pending flags in miscadmin.h to use sig_atomic_t? >> ClientConnectionLost would gain PGD

Re: Proposal for Signal Detection Refactoring

On Tue, Sep 25, 2018 at 3:23 PM Tom Lane wrote: > Chris Travers writes: > > However, what I think one could do is use a struct of volatile > > sig_atomic_t members and macros for checking/setting. Simply writing a > > value is safe in C89 and higher. > > Yeah, we could group those flags in a s

Re: Proposal for Signal Detection Refactoring

On Wed, Sep 26, 2018 at 12:41 AM Michael Paquier wrote: > On Mon, Sep 24, 2018 at 09:27:35PM -0700, Andres Freund wrote: > > On 2018-09-25 11:50:47 +0900, Michael Paquier wrote: > >> PGDLLIMPORT changes don't get back-patched as well... > > > > We've been more aggressive with that lately, and I t

Re: Proposal for Signal Detection Refactoring

On Mon, Sep 24, 2018 at 09:27:35PM -0700, Andres Freund wrote: > On 2018-09-25 11:50:47 +0900, Michael Paquier wrote: >> PGDLLIMPORT changes don't get back-patched as well... > > We've been more aggressive with that lately, and I think that's good. It > just is a unnecessarily portability barrier

Re: Proposal for Signal Detection Refactoring

Chris Travers writes: > However, what I think one could do is use a struct of volatile > sig_atomic_t members and macros for checking/setting. Simply writing a > value is safe in C89 and higher. Yeah, we could group those flags in a struct, but what's the point? regards

Re: Proposal for Signal Detection Refactoring

On Mon, Sep 24, 2018 at 7:06 PM Andres Freund 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 > > > wrote: > > > > > First, thanks for taking the time to write this. Its very helpful. > > > Additiona

Re: Proposal for Signal Detection Refactoring

On Tue, Sep 25, 2018 at 3:03 AM Tom Lane wrote: > Michael Paquier writes: > > And then within separate signal handlers things like: > > void > > StatementCancelHandler(SIGNAL_ARGS) > > { > > [...] > > signalPendingFlags |= PENDING_INTERRUPT | PENDING_CANCEL_QUERY; > > [...] > > } > >

Re: Proposal for Signal Detection Refactoring

On 2018-09-25 11:50:47 +0900, Michael Paquier wrote: > PGDLLIMPORT changes don't get back-patched as well... We've been more aggressive with that lately, and I think that's good. It just is a unnecessarily portability barrier for extension to be judicious in applying that. - Andres

Re: Proposal for Signal Detection Refactoring

On Mon, Sep 24, 2018 at 10:39:40PM -0400, Tom Lane wrote: > I wonder why ClientConnectionLost isn't PGDLLIMPORT like the rest. Same question here. As that's kind of a separate discussion, I left it out. Now I don't mind changing that at the same time as that's harmless, and as that's only a patc

Re: Proposal for Signal Detection Refactoring

Michael Paquier writes: > Let's change it then. ClientConnectionLost needs also to be changed as > miscadmin.h tells that it could be used in a signal handler. What do you > think about the attached? Looks reasonable to me (I've not tested though). I wonder why ClientConnectionLost isn't PGDLLI

Re: Proposal for Signal Detection Refactoring

On Mon, Sep 24, 2018 at 09:38:11PM -0400, Tom Lane wrote: > Yeah, in principle any global variable touched by a signal handler should > be sig_atomic_t. I don't know of any modern platform where using "bool" > is unsafe, but per the C standard it could be. The case that would be > worrisome is if

Re: Proposal for Signal Detection Refactoring

Michael Paquier writes: > At the same time, all the pending flags in miscadmin.h could be switched > to sig_atomic_t if we were to be correct, no? The counters could be > higher than 256 so that's not really possible. Yeah, in principle any global variable touched by a signal handler should be

Re: Proposal for Signal Detection Refactoring

On Mon, Sep 24, 2018 at 09:03:49PM -0400, Tom Lane wrote: > You could only fix that by blocking all signal handling during the > handler, which would be expensive and rather pointless. > > I do not think that it's readily possible to improve on the current > situation with one sig_atomic_t per fla

Re: Proposal for Signal Detection Refactoring

Michael Paquier writes: > And then within separate signal handlers things like: > void > StatementCancelHandler(SIGNAL_ARGS) > { > [...] > signalPendingFlags |= PENDING_INTERRUPT | PENDING_CANCEL_QUERY; > [...] > } AFAICS this still wouldn't work. The machine code is still going to l

Re: Proposal for Signal Detection Refactoring

On Mon, Sep 24, 2018 at 05:23:56PM -0700, Andres Freund wrote: > On 2018-09-25 08:57:25 +0900, Michael Paquier wrote: >> Anyway, putting the back-patching pain aside, and just for my own >> knowledge... Andres, would it be fine to just use one sig_atomic_t >> field which can be set from different

Re: Proposal for Signal Detection Refactoring

On 2018-09-25 08:57:25 +0900, Michael Paquier wrote: > On Mon, Sep 24, 2018 at 10:06:40AM -0700, Andres Freund wrote: > > 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? >

Re: Proposal for Signal Detection Refactoring

On Mon, Sep 24, 2018 at 10:06:40AM -0700, Andres Freund wrote: > 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? My point was just to reduce the number of variables used and ea

Re: Proposal for Signal Detection Refactoring

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

Re: Proposal for Signal Detection Refactoring

Very minor revision On Mon, Sep 24, 2018 at 11:45 AM Chris Travers wrote: > > 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

Re: Proposal for Signal Detection Refactoring

I did some more reading. On Mon, Sep 24, 2018 at 10:15 AM Chris Travers 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 > wrote: > >> >> There could be value in refactoring things so a

Re: Proposal for Signal Detection Refactoring

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 wrote: > On Fri, Sep 21, 2018 at 12:35:46PM +0200, Chris Travers wrote: > > I understand how lock levels don't fit a simple hierarchy but at least > > w

Re: Proposal for Signal Detection Refactoring

On Fri, Sep 21, 2018 at 12:35:46PM +0200, Chris Travers wrote: > I understand how lock levels don't fit a simple hierarchy but at least > when it comes to what is going to be aborted on a signal, I am having > trouble understanding the problem here. It may be possible to come with a clear hierarch

Re: Proposal for Signal Detection Refactoring

On Fri, Sep 21, 2018 at 6:46 AM Michael Paquier wrote: > On Thu, Sep 20, 2018 at 03:08:34PM +0200, Chris Travers wrote: > > So here's a small patch. I will add it for the next commit fest unless > > anyone has any reason I shouldn't. > > - return InterruptPending && (QueryCancelPending ||

Re: Proposal for Signal Detection Refactoring

On 2018-09-21 13:46:11 +0900, Michael Paquier wrote: > On Thu, Sep 20, 2018 at 03:08:34PM +0200, Chris Travers wrote: > > So here's a small patch. I will add it for the next commit fest unless > > anyone has any reason I shouldn't. > > - return InterruptPending && (QueryCancelPending || Pro

Re: Proposal for Signal Detection Refactoring

On Thu, Sep 20, 2018 at 03:08:34PM +0200, Chris Travers wrote: > So here's a small patch. I will add it for the next commit fest unless > anyone has any reason I shouldn't. - return InterruptPending && (QueryCancelPending || ProcDiePending); + return PENDING_INTERRUPT_LEVEL() >= QUERY

Proposal for Signal Detection Refactoring

After the patch we did for the race condition here, one point which was brought up which I thought was quite relevant was the fact that checking global flags feels a little ugly. I would like to set up a patch which refactors this as follows: 1. We create an enum of cancellation levels: NO_I