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_CANCEL;

This is pretty similar to lock levels, where it is pretty hard to put a
strict monotone hierarchy when it comes to such interruptions. The new
code does not seem like an improvment either, as for example in the code
mentioned above, you know directly what are the actions involved, which
is not the case with the new code style.
--
Michael

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 || ProcDiePending);
+       return PENDING_INTERRUPT_LEVEL() >= QUERY_CANCEL;

This is pretty similar to lock levels, where it is pretty hard to put a
strict monotone hierarchy when it comes to such interruptions.

+1

Greetings,

Andres Freund

On Fri, Sep 21, 2018 at 6:46 AM Michael Paquier <michael@paquier.xyz> 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 || ProcDiePending);
+       return PENDING_INTERRUPT_LEVEL() >= QUERY_CANCEL;

This is pretty similar to lock levels, where it is pretty hard to put a
strict monotone hierarchy when it comes to such interruptions.

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.

Does anyone have any search terms I should look into the archives regarding
this issue?

I will assume this patch will be rejected then.

The new
code does not seem like an improvment either, as for example in the code
mentioned above, you know directly what are the actions involved, which
is not the case with the new code style.

The initial motivation for this patch was that it felt a little bit odd to
be using global
boolean flags for this sort of approach and I was concerned that if this
approach
proliferated it might cause problems later. As far as I know my previous
patch was
the second use of the current pattern.

So one thing I wonder is if there are ways where abstracting this would
make more sense.

--
Michael

--
Best Regards,
Chris Travers
Head of Database

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

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 hierarchy with the current
interruption types in place. Still I am not sure that the definition
you put behind is completely correct, and I think that we need to
question as well the value of putting such restrictions for future
interruption types because they would need to fit into it. That's quite
a heavy constraint to live with. There is such logic with wal_level for
example, which is something I am not completely happy with either...
But this one is a story for another time, and another thread.

Regarding your patch, it seems to me that it does not improve
readability as I mentioned up-thread because you lose sight of what can
be interrupted in a given code path, which is what the current code
shows actually nicely.

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

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@paquier.xyz> 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
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 hierarchy with the current
interruption types in place. Still I am not sure that the definition
you put behind is completely correct, and I think that we need to
question as well the value of putting such restrictions for future
interruption types because they would need to fit into it.

The future-safety issue is a really good one and it's one reason I kept the
infinite loop patch as semantically consistent with the API as I could at
the cost of some complexity.

I have another area where I think a patch would be more valuable anyway in
terms of refactoring.

That's quite
a heavy constraint to live with. There is such logic with wal_level for
example, which is something I am not completely happy with either...
But this one is a story for another time, and another thread.

From a cleanup perspective a concentric circles approach seems like it is
correct to me (which would correspond to a hierarchy of interrupts) but I
can see that assuming that all pending interrupts would be checked solely
for cleanup reasons might be a bad assumption on my part.

Regarding your patch, it seems to me that it does not improve
readability as I mentioned up-thread because you lose sight of what can
be interrupted in a given code path, which is what the current code
shows actually nicely.

So I guess there are two fundamental questions here.

1. Do we want to move away from checking global flags like this directly?
I think we do because it makes future changes possibly harder and more
complex since there is no encapsulation of logic. But I don't see a point
in putting effort into that without consensus.

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.

Thanks again for taking the time to go over the concerns in detail. It
really helps.

Best Wishes,
Chris Travers

--
Michael

--
Best Regards,
Chris Travers
Head of Database

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

I did some more reading.

On Mon, Sep 24, 2018 at 10:15 AM Chris Travers <chris.travers@adjust.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@paquier.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.

--

Best Regards,
Chris Travers
Head of Database

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

--
Best Regards,
Chris Travers
Head of Database

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

Very minor revision
On Mon, Sep 24, 2018 at 11:45 AM Chris Travers <chris.travers@adjust.com>
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 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

These would be implemented as sig_atomic_t which is defined in C89 but has
no atomic operators other than writing the full value.

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.

--

Best Regards,
Chris Travers
Head of Database

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

--
Best Regards,
Chris Travers
Head of Database

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

--
Best Regards,
Chris Travers
Head of Database

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

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.travers@adjust.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@paquier.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

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 ease
debugger lookups with what is on the stack.

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 code paths? Say:
typedef enum SignalPendingType {
PENDING_INTERRUPT,
PENDING_CANCEL_QUERY,
PENDING_PROC_DIE,
PENDING_RELOAD,
PENDING_SESSION_TIMEOUT
};
extern volatile sig_atomic_t signalPendingFlags;

And then within separate signal handlers things like:
void
StatementCancelHandler(SIGNAL_ARGS)
{
[...]
signalPendingFlags |= PENDING_INTERRUPT | PENDING_CANCEL_QUERY;
[...]
}

void
PostgresSigHupHandler(SIGNAL_ARGS)
{
[...]
signalPendingFlags |= ConfigReloadPending;
[...]
}
--
Michael

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?

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 bit math
to know.

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 code paths? Say:
typedef enum SignalPendingType {
PENDING_INTERRUPT,
PENDING_CANCEL_QUERY,
PENDING_PROC_DIE,
PENDING_RELOAD,
PENDING_SESSION_TIMEOUT
};

Well, they'd have to different bits...

extern volatile sig_atomic_t signalPendingFlags;

Note that sig_atomic_t IIRC is only guaranteed to effectively be 8 bit
wide - so you couldn't have that many flags.

Greetings,

Andres Freund

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 code paths? Say:
typedef enum SignalPendingType {
PENDING_INTERRUPT,
PENDING_CANCEL_QUERY,
PENDING_PROC_DIE,
PENDING_RELOAD,
PENDING_SESSION_TIMEOUT
};

Well, they'd have to different bits...

Sure, I forgot to write the "foo = 1 << 0" and such ;)

extern volatile sig_atomic_t signalPendingFlags;

Note that sig_atomic_t IIRC is only guaranteed to effectively be 8 bit
wide - so you couldn't have that many flags.

I see. I thought that it mapped at least int, but C99 tells that it
can be as small as char. That's indeed quite limited...
--
Michael

Michael Paquier <michael@paquier.xyz> 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
look (on many machines) like "load from signalPendingFlags,
OR in some bits, store to signalPendingFlags". So there's still a
window for another signal handler to interrupt that and store some
bits that will get lost.

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

regards, tom lane

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

Okay, thanks for the confirmation.

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

Michael Paquier <michael@paquier.xyz> 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 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 setting the variable requires a load/modify/store, which
does apply to char-sized variables on some ancient platforms. I think
there's no need to worry for int-sized variables.

regards, tom lane

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 setting the variable requires a load/modify/store, which
does apply to char-sized variables on some ancient platforms. I think
there's no need to worry for int-sized variables.

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?
--
Michael

Michael Paquier <michael@paquier.xyz> 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 PGDLLIMPORT like the rest.

regards, tom lane

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 patch for HEAD. PGDLLIMPORT changes
don't get back-patched as well...
--
Michael

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

On Tue, Sep 25, 2018 at 3:03 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Michael Paquier <michael@paquier.xyz> 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
look (on many machines) like "load from signalPendingFlags,
OR in some bits, store to signalPendingFlags". So there's still a
window for another signal handler to interrupt that and store some
bits that will get lost.

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

After a fair bit of reading I think there are ways of doing this in C11 but
I don't think those are portable to C99.

In C99 (and, in practice C89, as the C99 committee noted there were no
known C89 implementations where reading was unsafe), reading or writing a
static sig_atomic_t inside a signal handler is safe, but a round-trip is
*not* guaranteed not to clobber. While I could be wrong, I think it is
only in C11 that you have any round-trip operations which are guaranteed
not to clobber in the language itself.

Basically we are a long way out to be able to consider these a single value
as flags.

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.

regards, tom lane

--
Best Regards,
Chris Travers
Head of Database

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