Re: "stuck spinlock"

From: Andres Freund <andres(at)2ndquadrant(dot)com>
To: Robert Haas <robertmhaas(at)gmail(dot)com>
Cc: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, PostgreSQL-development Hackers <pgsql-hackers(at)postgresql(dot)org>, Christophe Pettus <xof(at)thebuild(dot)com>, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>
Subject: Re: "stuck spinlock"
Date: 2013-12-14 12:42:10
Message-ID: 20131214124210.GD25520@awork2.anarazel.de
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On 2013-12-13 13:39:42 -0500, Robert Haas wrote:
> On Fri, Dec 13, 2013 at 1:15 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> > Agreed on not going forward like now, but I don't really see how they
> > could usefully use die(). I think we should just mandate that every
> > bgworker conneced to shared memory registers a sigterm handler - we
> > could put a check into BackgroundWorkerUnblockSignals(). We should leave
> > the current handler in for unconnected one though...
> > bgworkers are supposed to be written as a loop around procLatch, so
> > adding a !got_sigterm, probably isn't too hard.
>
> I think the !got_sigterm thing is complete bunk. If a background
> worker is running SQL queries, it really ought to honor a query cancel
> or sigterm at the next CHECK_FOR_INTERRUPTS().

I am not convinced by the necessity of that, not in general. After all,
the code is using a bgworker and not a normal backend for a reason. If
you e.g. have queing code, it very well might need to serialize its
state to disk before shutting down. Checking whether the bgworker should
shut down every iteration of the mainloop sounds appropriate to me for
such cases.

But I think we should provide a default handler that does the necessary
things to interrupt queries, so bgworker authors don't have to do it
themselves and, just as importantly, we can more easily add new stuff
there.

> +static void
> +handle_sigterm(SIGNAL_ARGS)
> +{
> + int save_errno = errno;
> +
> + if (MyProc)
> + SetLatch(&MyProc->procLatch);
> +
> + if (!proc_exit_inprogress)
> + {
> + InterruptPending = true;
> + ProcDiePending = true;
> + }
> +
> + errno = save_errno;
> +}
>
> ...but I'm not 100% sure that's right, either.

If you want a bgworker to behave as close as a normal backend we should
probably really do the full dance die() does. Specifically call
ProcessInterrupts() immediately if ImmediateInterruptOK allows it,
otherwise we'd just continue waiting for locks and similar.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Andres Freund 2013-12-14 12:54:23 Re: PoC: Partial sort
Previous Message Andres Freund 2013-12-14 12:23:48 Re: "stuck spinlock"