Thread: [PATCH] ProcessInterrupts_hook
Hi folks
A few times lately I've been doing things in extensions that've made me want to be able to run my own code whenever InterruptPending is true and CHECK_FOR_INTERRUPTS() calls ProcessInterrupts()
So here's a simple patch to add ProcessInterrupts_hook. It follows the usual pattern like ProcessUtility_hook and standard_ProcessUtility.
Why? Because sometimes I want most of the behaviour of die(), but the option to override it with some bgworker-specific choices occasionally. HOLD_INTERRUPTS() is too big a hammer.
What I really want to go along with this is a way for any backend to observe the postmaster's pmState and its "Shutdown" variable's value, so any backend can tell if we're in FastShutdown, SmartShutdown, etc. Copies in shmem only obviously. But I'm not convinced it's right to just copy these vars as-is to shmem, and I don't want to use the memory for a ProcSignal slot for something that won't be relevant for most backends for most of the postmaster lifetime. Ideas welcomed.
Attachment
On Mon, Jan 18, 2021 at 3:00 AM Craig Ringer <craig.ringer@enterprisedb.com> wrote: > A few times lately I've been doing things in extensions that've made me want to be able to run my own code whenever InterruptPendingis true and CHECK_FOR_INTERRUPTS() calls ProcessInterrupts() I've wanted this in the past, too, so +1 from me. > What I really want to go along with this is a way for any backend to observe the postmaster's pmState and its "Shutdown"variable's value, so any backend can tell if we're in FastShutdown, SmartShutdown, etc. Copies in shmem only obviously.But I'm not convinced it's right to just copy these vars as-is to shmem, and I don't want to use the memory fora ProcSignal slot for something that won't be relevant for most backends for most of the postmaster lifetime. Ideas welcomed. I've wanted something along this line, too, but what I was thinking about was more along the lines of having the postmaster signal the backends when a smart shutdown happened. After all when a fast shutdown happens the backends already get told to terminate, and that seems like it ought to be enough: I'm not sure backends have any business caring about why they are being asked to terminate. But they might well want to know whether a smart shutdown is in progress, and right now there's no way for them to know that. -- Robert Haas EDB: http://www.enterprisedb.com
Robert Haas <robertmhaas@gmail.com> writes:
> On Mon, Jan 18, 2021 at 3:00 AM Craig Ringer
> <craig.ringer@enterprisedb.com> wrote:
>> A few times lately I've been doing things in extensions that've made me want to be able to run my own code whenever
InterruptPendingis true and CHECK_FOR_INTERRUPTS() calls ProcessInterrupts()
> I've wanted this in the past, too, so +1 from me.
I dunno, this seems pretty scary and easily abusable. There's not all
that much that can be done safely in ProcessInterrupts(), and we should
not be encouraging extensions to think they can add random processing
there.
>> What I really want to go along with this is a way for any backend to observe the postmaster's pmState and its
"Shutdown"variable's value, so any backend can tell if we're in FastShutdown, SmartShutdown, etc.
> I've wanted something along this line, too, but what I was thinking
> about was more along the lines of having the postmaster signal the
> backends when a smart shutdown happened.
We're about halfway there already, see 7e784d1dc. I didn't do the
other half because it wasn't necessary to the problem, but exposing
the shutdown state more fully seems reasonable.
regards, tom lane
On Mon, Jan 18, 2021 at 11:56 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > I've wanted this in the past, too, so +1 from me. > > I dunno, this seems pretty scary and easily abusable. There's not all > that much that can be done safely in ProcessInterrupts(), and we should > not be encouraging extensions to think they can add random processing > there. We've had this disagreement before about other things, and I just don't agree. If somebody uses a hook for something wildly unsafe, that will break their stuff, not ours. That's not to say I endorse adding hooks for random purposes in random places. In particular, if it's impossible to use a particular hook in a reasonably safe way, that's a sign that the hook is badly-designed and that we should not have it. But, that's not the case here. I care more about smart extension authors being able to do useful things than I do about the possibility that dumb extension authors will do stupid things. We can't really prevent the latter anyway: this is open source. > We're about halfway there already, see 7e784d1dc. I didn't do the > other half because it wasn't necessary to the problem, but exposing > the shutdown state more fully seems reasonable. Ah, I hadn't realized. -- Robert Haas EDB: http://www.enterprisedb.com
On Tue, 19 Jan 2021, 02:01 Robert Haas, <robertmhaas@gmail.com> wrote:
On Mon, Jan 18, 2021 at 11:56 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > I've wanted this in the past, too, so +1 from me.
>
> I dunno, this seems pretty scary and easily abusable. There's not all
> that much that can be done safely in ProcessInterrupts(), and we should
> not be encouraging extensions to think they can add random processing
> there.
We've had this disagreement before about other things, and I just
don't agree. If somebody uses a hook for something wildly unsafe, that
will break their stuff, not ours.
Generally yeah.
And we have no shortage of hooks with plenty of error or abuse potential and few safeguards already. I'd argue that in C code any external code is inherently unsafe anyway. So it's mainly down to whether the hook actively encourages unsafe actions without providing commensurate benefits, and whether there's a better/safer way to achieve the same thing.
That's not to say I endorse adding
hooks for random purposes in random places. In particular, if it's
impossible to use a particular hook in a reasonably safe way, that's a
sign that the hook is badly-designed and that we should not have it.
Yep. Agreed.
Any hook is possible to abuse or write incorrectly, from simple fmgr loadable functions right on up.
The argument that a hook could be abused would apply just as well to exposing pqsignal() itself to extensions. Probably more so. Also to anything like ProcessUtility_hook.
> We're about halfway there already, see 7e784d1dc. I didn't do the
> other half because it wasn't necessary to the problem, but exposing
> the shutdown state more fully seems reasonable.
Excellent, I'll take a look. Thanks.
On Tue, 19 Jan 2021 at 12:44, Craig Ringer <craig.ringer@enterprisedb.com> wrote:
> We're about halfway there already, see 7e784d1dc. I didn't do the
> other half because it wasn't necessary to the problem, but exposing
> the shutdown state more fully seems reasonable.Excellent, I'll take a look. Thanks.
That looks very handy already.
Extending it to be set before SIGTERM too would be handy.
My suggestion, which I'm happy to post in patch form if you think it's reasonable:
* Change QuitSignalReason to ExitSignalReason to cover both SIGTERM (fast) and SIGQUIT (immediate)
* Rename PMQUIT_FOR_STOP to PMEXIT_IMMEDIATE_SHUTDOWN
* Add enumeration values PMEXIT_SMART_SHUTDOWN and PMEXIT_FAST_SHUTDOWN
* For a fast shutdown, in pmdie()'s SIGINT case call SetExitSignalReason(PMEXIT_FAST_SHUTDOWN), so that when PostmasterStateMachine() calls SignalSomeChildren(SIGTERM, ...) in response to PM_STOP_BACKENDS, the reason is already available.
* For smart shutdown, in pmdie()'s SIGTERM case call SetExitSignalReason(PMEXIT_SMART_SHUTDOWN) and set the latch of every live backend. There isn't any appropriate PROCSIG so unless we want to overload PROCSIG_WALSND_INIT_STOPPING (ick), but I think it'd generally be sufficient to check GetExitSignalReason() in backend main loops.
The fast shutdown case seems like a no-brainer extension of your existing patch.
I'm not entirely sure about the smart shutdown case. I don't want to add a ProcSignal slot just for this and the info isn't urgent anyway. I think that by checking for postmaster shutdown in the backend main loop we'd be able to support eager termination of idle backends on smart shutdown (immediately, or after an idle grace period), which is something I've wanted for quite some time. It shouldn't be significantly expensive especially in the idle loop.
Thoughts?
(Also I want a hook in PostgresMain's idle loop for things like this).
On 1/19/21 1:42 AM, Craig Ringer wrote: > On Tue, 19 Jan 2021 at 12:44, Craig Ringer > <craig.ringer@enterprisedb.com <mailto:craig.ringer@enterprisedb.com>> > wrote: > > > We're about halfway there already, see 7e784d1dc. I didn't > do the > > other half because it wasn't necessary to the problem, but > exposing > > the shutdown state more fully seems reasonable. > > Excellent, I'll take a look. Thanks. > > That looks very handy already. > > Extending it to be set before SIGTERM too would be handy. > > My suggestion, which I'm happy to post in patch form if you think it's > reasonable <snip> Tom, Robert, and thoughts on the proposals in [1]? Craig, based on the state of this proposal (i.e. likely not a candidate for PG14) I think it makes sense to move it to the next CF. Regards, -- -David david@pgmasters.net [1] https://www.postgresql.org/message-id/CAGRY4nyNfscmQiZBCNT7cBYnQxJLAAVCGz%2BGZAQDAco1Fbb01w%40mail.gmail.com
David Steele <david@pgmasters.net> writes: > On 1/19/21 1:42 AM, Craig Ringer wrote: >> My suggestion, which I'm happy to post in patch form if you think it's >> reasonable <snip> > Tom, Robert, and thoughts on the proposals in [1]? > https://www.postgresql.org/message-id/CAGRY4nyNfscmQiZBCNT7cBYnQxJLAAVCGz%2BGZAQDAco1Fbb01w%40mail.gmail.com No objection to generalizing the state passed through pmsignal.c. I'm not very comfortable about the idea of having the postmaster set child processes' latches ... that doesn't sound terribly safe from the standpoint of not allowing the postmaster to mess with shared memory state that could cause it to block or crash. If we already do that elsewhere, then OK, but I don't think we do. regards, tom lane
On Fri, Mar 19, 2021 at 3:25 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > David Steele <david@pgmasters.net> writes: > > On 1/19/21 1:42 AM, Craig Ringer wrote: > >> My suggestion, which I'm happy to post in patch form if you think it's > >> reasonable <snip> > > > Tom, Robert, and thoughts on the proposals in [1]? > > https://www.postgresql.org/message-id/CAGRY4nyNfscmQiZBCNT7cBYnQxJLAAVCGz%2BGZAQDAco1Fbb01w%40mail.gmail.com > > No objection to generalizing the state passed through pmsignal.c. > > I'm not very comfortable about the idea of having the postmaster set > child processes' latches ... that doesn't sound terribly safe from the > standpoint of not allowing the postmaster to mess with shared memory > state that could cause it to block or crash. If we already do that > elsewhere, then OK, but I don't think we do. It should be unnecessary anyway. We changed it a while back to make any SIGUSR1 set the latch .... -- Robert Haas EDB: http://www.enterprisedb.com
Robert Haas <robertmhaas@gmail.com> writes:
> On Fri, Mar 19, 2021 at 3:25 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I'm not very comfortable about the idea of having the postmaster set
>> child processes' latches ... that doesn't sound terribly safe from the
>> standpoint of not allowing the postmaster to mess with shared memory
>> state that could cause it to block or crash. If we already do that
>> elsewhere, then OK, but I don't think we do.
> It should be unnecessary anyway. We changed it a while back to make
> any SIGUSR1 set the latch ....
Hmm, so the postmaster could send SIGUSR1 without setting any particular
pmsignal reason? Yeah, I suppose that could work. Or we could recast
this as being a new pmsignal reason.
regards, tom lane
On Sat, 20 Mar 2021 at 03:46, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Robert Haas <robertmhaas@gmail.com> writes:
> On Fri, Mar 19, 2021 at 3:25 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I'm not very comfortable about the idea of having the postmaster set
>> child processes' latches ... that doesn't sound terribly safe from the
>> standpoint of not allowing the postmaster to mess with shared memory
>> state that could cause it to block or crash. If we already do that
>> elsewhere, then OK, but I don't think we do.
> It should be unnecessary anyway. We changed it a while back to make
> any SIGUSR1 set the latch ....
Hmm, so the postmaster could send SIGUSR1 without setting any particular
pmsignal reason? Yeah, I suppose that could work. Or we could recast
this as being a new pmsignal reason.
I'd be fine with either way.
I don't expect to be able to get to working on a concrete patch for this any time soon, so I'll be leaving it here unless someone else needs to pick it up for their extension work. The in-principle agreement is there for future work anyway.
On Tue, Jun 29, 2021 at 01:32:26PM +0800, Craig Ringer wrote: > On Sat, 20 Mar 2021 at 03:46, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > > Robert Haas <robertmhaas@gmail.com> writes: > > > On Fri, Mar 19, 2021 at 3:25 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > >> I'm not very comfortable about the idea of having the postmaster set > > >> child processes' latches ... that doesn't sound terribly safe from the > > >> standpoint of not allowing the postmaster to mess with shared memory > > >> state that could cause it to block or crash. If we already do that > > >> elsewhere, then OK, but I don't think we do. > > > > > It should be unnecessary anyway. We changed it a while back to make > > > any SIGUSR1 set the latch .... > > > > Hmm, so the postmaster could send SIGUSR1 without setting any particular > > pmsignal reason? Yeah, I suppose that could work. Or we could recast > > this as being a new pmsignal reason. > > > > I'd be fine with either way. > > I don't expect to be able to get to working on a concrete patch for this > any time soon, so I'll be leaving it here unless someone else needs to pick > it up for their extension work. The in-principle agreement is there for > future work anyway. Hi Craig, There is still a CF entry for this. Should we close it as withdrawn? or maybe RwF? -- Jaime Casanova Director de Servicios Profesionales SystemGuards - Consultores de PostgreSQL
On Sat, 2 Oct 2021 at 01:24, Jaime Casanova <jcasanov@systemguards.com.ec> wrote:
On Tue, Jun 29, 2021 at 01:32:26PM +0800, Craig Ringer wrote:
> On Sat, 20 Mar 2021 at 03:46, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> > Robert Haas <robertmhaas@gmail.com> writes:
> > > On Fri, Mar 19, 2021 at 3:25 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > >> I'm not very comfortable about the idea of having the postmaster set
> > >> child processes' latches ... that doesn't sound terribly safe from the
> > >> standpoint of not allowing the postmaster to mess with shared memory
> > >> state that could cause it to block or crash. If we already do that
> > >> elsewhere, then OK, but I don't think we do.
> >
> > > It should be unnecessary anyway. We changed it a while back to make
> > > any SIGUSR1 set the latch ....
> >
> > Hmm, so the postmaster could send SIGUSR1 without setting any particular
> > pmsignal reason? Yeah, I suppose that could work. Or we could recast
> > this as being a new pmsignal reason.
> >
>
> I'd be fine with either way.
>
> I don't expect to be able to get to working on a concrete patch for this
> any time soon, so I'll be leaving it here unless someone else needs to pick
> it up for their extension work. The in-principle agreement is there for
> future work anyway.
Hi Craig,
There is still a CF entry for this. Should we close it as withdrawn? or
maybe RwF?
I'm not going to get time for it now, so I think marking it withdrawn is reasonable.
I think it's well worth doing and Tom seems to think it's not a crazy idea, but I'm no longer working on the software that needed it, and don't see a lot of other people calling for it, so it can wait until someone else needs it.