Thread: Interrupting long external library calls
Hi all,
One of the issues we've been looking at with PostGIS is how to interrupt
long-running processing tasks in external libraries, particularly GEOS.
After a few tests here, it seems that even the existing SIGALRM handler
doesn't get called if statement_timeout is reached when in an external
library, e.g. with PostgreSQL 9.0/PostGIS 2.0 trunk/GEOS:
pg90@kentang:~$ ./rel/bin/postgres --single -D data postgis
PostgreSQL stand-alone backend 9.0.7
backend> create database foo;
backend> drop database foo;
backend> show statement_timeout; 1: statement_timeout (typeid = 25, len = -1, typmod = -1,
byval = f) ---- 1: statement_timeout = "500ms" (typeid = 25, len = -1, typmod
= -1, byval = f) ----
backend> set statement_timeout = '10ms';
backend> create database foo;
ERROR: canceling statement due to statement timeout
STATEMENT: create database foo;
(note the following command takes about 3-4s to run but doesn't get
cancelled)
backend> select st_segmentize('LINESTRING(0 0, 10 0)', 1e-12);
ERROR: invalid memory alloc request size 1073741824
Is there a way in which we can interrupt long-running external library
calls that are unable to call the PostgreSQL CHECK_FOR_INTERRUPTS()
macro directly? If it is possible for standard alarms to work in this
way then something along the lines of an alarm within PostgreSQL itself
that manually invokes CHECK_FOR_INTERRUPTS() every 1s while the query is
running could potentially solve the issue for us.
Many thanks,
Mark.
On 16.05.2012 13:25, Mark Cave-Ayland wrote: > One of the issues we've been looking at with PostGIS is how to interrupt > long-running processing tasks in external libraries, particularly GEOS. > After a few tests here, it seems that even the existing SIGALRM handler > doesn't get called if statement_timeout is reached when in an external > library, e.g. with PostgreSQL 9.0/PostGIS 2.0 trunk/GEOS: If you interrupt an external library call, it might leave memory in an inconsistent state, or some other such thing. It's not generally safe to interrupt arbitrary 3rd party code. However, if you're absolutely positively sure that the library function can tolerate that, you can set "ImmediateInterruptOK = true" before calling it. See e.g PGSemaphoreLock() on how that's done before starting to sleep on a semapgore. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
On 16/05/12 11:39, Heikki Linnakangas wrote: >> One of the issues we've been looking at with PostGIS is how to interrupt >> long-running processing tasks in external libraries, particularly GEOS. >> After a few tests here, it seems that even the existing SIGALRM handler >> doesn't get called if statement_timeout is reached when in an external >> library, e.g. with PostgreSQL 9.0/PostGIS 2.0 trunk/GEOS: > > If you interrupt an external library call, it might leave memory in an > inconsistent state, or some other such thing. It's not generally safe to > interrupt arbitrary 3rd party code. > > However, if you're absolutely positively sure that the library function > can tolerate that, you can set "ImmediateInterruptOK = true" before > calling it. See e.g PGSemaphoreLock() on how that's done before starting > to sleep on a semapgore. Hi Heikki, Yes that appears to work fine on the surface - just testing now to determine what state everything is left in when queries are aborted prematurely. Many thanks, Mark.
On 16.05.2012 14:30, Mark Cave-Ayland wrote: > On 16/05/12 11:39, Heikki Linnakangas wrote: > >> However, if you're absolutely positively sure that the library function >> can tolerate that, you can set "ImmediateInterruptOK = true" before >> calling it. See e.g PGSemaphoreLock() on how that's done before starting >> to sleep on a semapgore. > > Hi Heikki, > > Yes that appears to work fine on the surface - just testing now to > determine what state everything is left in when queries are aborted > prematurely. You're unlikely to catch all problems just by testing. I wouldn't trust that it's safe unless the library authors specifically mentions that it is safe to longjump out of the function at any time. Note for example that if the library function calls back to palloc/pfree, then it's not safe, because interrupting those functions is not safe. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
On Wed, May 16, 2012 at 02:46:17PM +0300, Heikki Linnakangas wrote: > On 16.05.2012 14:30, Mark Cave-Ayland wrote: > >On 16/05/12 11:39, Heikki Linnakangas wrote: > > > >>However, if you're absolutely positively sure that the library function > >>can tolerate that, you can set "ImmediateInterruptOK = true" before > >>calling it. See e.g PGSemaphoreLock() on how that's done before starting > >>to sleep on a semapgore. > > > >Hi Heikki, > > > >Yes that appears to work fine on the surface - just testing now to > >determine what state everything is left in when queries are aborted > >prematurely. > > You're unlikely to catch all problems just by testing. I wouldn't > trust that it's safe unless the library authors specifically > mentions that it is safe to longjump out of the function at any > time. Note for example that if the library function calls back to > palloc/pfree, then it's not safe, because interrupting those > functions is not safe. I'm right now getting the external library into using memory allocator wrappers so to provide an syntetized OOM condition in an aim to have a more predictable answer to that. But CHECK_FOR_INTERRUPTS doesn't return, right ? Is there another macro for just checking w/out yet acting upon it ? --strk; ,------o-. | __/ | Delivering high quality PostGIS 2.0 ! | / 2.0 | http://strk.keybit.net - http://vizzuality.com`-o------'
On 16.05.2012 15:42, Sandro Santilli wrote: > But CHECK_FOR_INTERRUPTS doesn't return, right ? > Is there another macro for just checking w/out yet acting upon it ? Hmm, no. CHECK_FOR_INTERRUPTS() checks the InterruptPending variable, but on Windows it also checks for UNBLOCKED_SIGNAL_QUEUE(). And even if InterruptPending is set, it's not totally certain that CHECK_FOR_INTERRUPTS() won't return. I think InterruptPending can be set spuriously (even if that's not possible today, I wouldn't rely on it), and if you're in a HOLD/RESUME_INTERRUPTS block, CHECK_FOR_INTERRUPTS() will do nothing even if InterruptPending is true. The only sane way to make 3rd party code interruptible is to add CHECK_FOR_INTERRUPTS() to it, in safe places. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
On Wed, May 16, 2012 at 07:30:03PM +0300, Heikki Linnakangas wrote: > On 16.05.2012 15:42, Sandro Santilli wrote: > >But CHECK_FOR_INTERRUPTS doesn't return, right ? > >Is there another macro for just checking w/out yet acting upon it ? > > Hmm, no. CHECK_FOR_INTERRUPTS() checks the InterruptPending > variable, but on Windows it also checks for > UNBLOCKED_SIGNAL_QUEUE(). And even if InterruptPending is set, it's > not totally certain that CHECK_FOR_INTERRUPTS() won't return. I > think InterruptPending can be set spuriously (even if that's not > possible today, I wouldn't rely on it), and if you're in a > HOLD/RESUME_INTERRUPTS block, CHECK_FOR_INTERRUPTS() will do nothing > even if InterruptPending is true. > > The only sane way to make 3rd party code interruptible is to add > CHECK_FOR_INTERRUPTS() to it, in safe places. No place is safe if CHECK_FOR_INTERRUPTS doesn't return. How could caller code cleanup on interruption ? --strk;
On 24.05.2012 16:04, Sandro Santilli wrote: > On Wed, May 16, 2012 at 07:30:03PM +0300, Heikki Linnakangas wrote: >> On 16.05.2012 15:42, Sandro Santilli wrote: >>> But CHECK_FOR_INTERRUPTS doesn't return, right ? >>> Is there another macro for just checking w/out yet acting upon it ? >> >> Hmm, no. CHECK_FOR_INTERRUPTS() checks the InterruptPending >> variable, but on Windows it also checks for >> UNBLOCKED_SIGNAL_QUEUE(). And even if InterruptPending is set, it's >> not totally certain that CHECK_FOR_INTERRUPTS() won't return. I >> think InterruptPending can be set spuriously (even if that's not >> possible today, I wouldn't rely on it), and if you're in a >> HOLD/RESUME_INTERRUPTS block, CHECK_FOR_INTERRUPTS() will do nothing >> even if InterruptPending is true. >> >> The only sane way to make 3rd party code interruptible is to add >> CHECK_FOR_INTERRUPTS() to it, in safe places. > > No place is safe if CHECK_FOR_INTERRUPTS doesn't return. > How could caller code cleanup on interruption ? It would only be safe to call it in places where no cleanup is necessary. I don't know if there are any such places in the geos library. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
On Thu, May 24, 2012 at 04:55:34PM +0300, Heikki Linnakangas wrote: > On 24.05.2012 16:04, Sandro Santilli wrote: > >On Wed, May 16, 2012 at 07:30:03PM +0300, Heikki Linnakangas wrote: > >>On 16.05.2012 15:42, Sandro Santilli wrote: > >>>But CHECK_FOR_INTERRUPTS doesn't return, right ? > >>>Is there another macro for just checking w/out yet acting upon it ? > >> > >>Hmm, no. CHECK_FOR_INTERRUPTS() checks the InterruptPending > >>variable, but on Windows it also checks for > >>UNBLOCKED_SIGNAL_QUEUE(). And even if InterruptPending is set, it's > >>not totally certain that CHECK_FOR_INTERRUPTS() won't return. I > >>think InterruptPending can be set spuriously (even if that's not > >>possible today, I wouldn't rely on it), and if you're in a > >>HOLD/RESUME_INTERRUPTS block, CHECK_FOR_INTERRUPTS() will do nothing > >>even if InterruptPending is true. > >> > >>The only sane way to make 3rd party code interruptible is to add > >>CHECK_FOR_INTERRUPTS() to it, in safe places. > > > >No place is safe if CHECK_FOR_INTERRUPTS doesn't return. > >How could caller code cleanup on interruption ? > > It would only be safe to call it in places where no cleanup is > necessary. I don't know if there are any such places in the geos > library. Sure, right before starting and after finishing. That is nowhere useful for a premature interruption... The whole point of the work I'm doing these days is letting the users interrupt GEOS calls which often take a long time and a lot of memory resources. The current plan is to provide custom allocators to have automatic garbage collection on interruption. Turned out not to be an easy task to bend GEOS into using palloc/pfree, but there's progress in that direction. --strk;
On May24, 2012, at 15:04 , Sandro Santilli wrote:
> On Wed, May 16, 2012 at 07:30:03PM +0300, Heikki Linnakangas wrote:
>> On 16.05.2012 15:42, Sandro Santilli wrote:
>>> But CHECK_FOR_INTERRUPTS doesn't return, right ?
>>> Is there another macro for just checking w/out yet acting upon it ?
>>
>> Hmm, no. CHECK_FOR_INTERRUPTS() checks the InterruptPending
>> variable, but on Windows it also checks for
>> UNBLOCKED_SIGNAL_QUEUE(). And even if InterruptPending is set, it's
>> not totally certain that CHECK_FOR_INTERRUPTS() won't return. I
>> think InterruptPending can be set spuriously (even if that's not
>> possible today, I wouldn't rely on it), and if you're in a
>> HOLD/RESUME_INTERRUPTS block, CHECK_FOR_INTERRUPTS() will do nothing
>> even if InterruptPending is true.
>>
>> The only sane way to make 3rd party code interruptible is to add
>> CHECK_FOR_INTERRUPTS() to it, in safe places.
>
> No place is safe if CHECK_FOR_INTERRUPTS doesn't return.
> How could caller code cleanup on interruption ?
The postgres way is to use PG_TRY/PG_CATCH to make sure stuff gets cleaned
up if an error or an interrupts occurs. You could use those to make the
third-party library exception safe, but it'll probably be a quite
invasive change :-(.
Alternatively, you could replicate the check CHECK_FOR_INTERRUPTS() does,
but then don't actually call ProcessInterrupts() but instead just make
the third-party code abort signalling an error, and call
CHECK_FOR_INTERRUPTS() after the third-party code has returned. On unix-like
systems that'd be a simple as aborting if InterruptPending is set, while
on windows you'd have to do the if(UNBLOCKED_SIGNAL_QUEUE())… stuff first,
since otherwise InterruptPending will never get set. The following function
should do the trick
bool have_pending_interrupts() { #ifdef WIN32 if (UNBLOCKED_SIGNAL_QUEUE()) pgwin32_dispatch_queued_signals();
#endif
return InterruptPending && !InterruptHoldoffCount && !CritSectionCount; }
The third-party could would then do
if (have_pending_interrupts()) return some_error;
and you'd invoke in with
state = third_party_code(); CHECK_FOR_INTERRUPTS(); if (state != success) ereport(…);
There might be slim chance for false positives with that approach, since
ProcessInterrupts() might not always ereport(), even if InterruptHoldoffCount
and CritSectionCount are zero. But they'll simply get turned into spurious
ereport() by the if(state != success) check after CHECK_FOR_INTERRUPTS, and
should be very rare, and happen only shortly after a query cancel request
was received.
best regards,
Florian Pflug
On Thu, May 24, 2012 at 04:37:04PM +0200, Florian Pflug wrote: > On May24, 2012, at 15:04 , Sandro Santilli wrote: > > On Wed, May 16, 2012 at 07:30:03PM +0300, Heikki Linnakangas wrote: > >> On 16.05.2012 15:42, Sandro Santilli wrote: > >>> But CHECK_FOR_INTERRUPTS doesn't return, right ? > >>> Is there another macro for just checking w/out yet acting upon it ? > >> > >> Hmm, no. CHECK_FOR_INTERRUPTS() checks the InterruptPending > >> variable, but on Windows it also checks for > >> UNBLOCKED_SIGNAL_QUEUE(). And even if InterruptPending is set, it's > >> not totally certain that CHECK_FOR_INTERRUPTS() won't return. I > >> think InterruptPending can be set spuriously (even if that's not > >> possible today, I wouldn't rely on it), and if you're in a > >> HOLD/RESUME_INTERRUPTS block, CHECK_FOR_INTERRUPTS() will do nothing > >> even if InterruptPending is true. > >> > >> The only sane way to make 3rd party code interruptible is to add > >> CHECK_FOR_INTERRUPTS() to it, in safe places. > > > > No place is safe if CHECK_FOR_INTERRUPTS doesn't return. > > How could caller code cleanup on interruption ? > > The postgres way is to use PG_TRY/PG_CATCH to make sure stuff gets cleaned > up if an error or an interrupts occurs. You could use those to make the > third-party library exception safe, but it'll probably be a quite > invasive change :-(. > > Alternatively, you could replicate the check CHECK_FOR_INTERRUPTS() does, I ended up providing an explicit mechanism to request interruption of whatever the library is doing, and experimented (successfully so far) requesting the interruption from a SIGINT handler. Do you see any major drawback in doing so ? So far I installed the SIGINT handler within the library itself, but I guess it could be moved out instead to have ore fine-grained control over when to request interruption. Here's the code installing the signal handler within the library: https://github.com/strk/libgeos/commit/e820ecd0469b777953c132661877c2967b10cee2 --strk; ,------o-. | __/ | Delivering high quality PostGIS 2.0 ! | / 2.0 | http://strk.keybit.net - http://vizzuality.com`-o------'
Sandro Santilli <strk@keybit.net> writes:
> I ended up providing an explicit mechanism to request interruption of
> whatever the library is doing, and experimented (successfully so far)
> requesting the interruption from a SIGINT handler.
> Do you see any major drawback in doing so ?
This seems a bit fragile. It might work all right in Postgres, where
we tend to set up signal handlers just once at process start, but ISTM
other systems might assume they can change their signal handlers at
any time. The handler itself looks less than portable anyway ---
what about the SIGINFO case?
I assume that the geos::util::Interrupt::request() call sets a flag
somewhere that's going to be periodically checked in long-running
loops. Would it be possible for the periodic checks to include a
provision for a callback into Postgres-specific glue code, wherein
you could test the same flags CHECK_FOR_INTERRUPTS does? A similar
approach might then be usable in other contexts, and it seems safer
to me than messing with a host environment's signal handling.
regards, tom lane
On 25 May 2012 17:34, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Sandro Santilli <strk@keybit.net> writes: >> I ended up providing an explicit mechanism to request interruption of >> whatever the library is doing, and experimented (successfully so far) >> requesting the interruption from a SIGINT handler. > >> Do you see any major drawback in doing so ? > > This seems a bit fragile. It might work all right in Postgres, where > we tend to set up signal handlers just once at process start, but ISTM > other systems might assume they can change their signal handlers at > any time. The handler itself looks less than portable anyway --- > what about the SIGINFO case? > > I assume that the geos::util::Interrupt::request() call sets a flag > somewhere that's going to be periodically checked in long-running > loops. Would it be possible for the periodic checks to include a > provision for a callback into Postgres-specific glue code, wherein > you could test the same flags CHECK_FOR_INTERRUPTS does? A similar > approach might then be usable in other contexts, and it seems safer > to me than messing with a host environment's signal handling. Can we do that as a macro, e.g. POSTGRES_LIBRARY_INTERRUPT_CHECK() Otherwise the fix to this problem may be worse - faulty interrupt handlers are worse than none at all. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On May26, 2012, at 12:40 , Simon Riggs wrote: > On 25 May 2012 17:34, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> I assume that the geos::util::Interrupt::request() call sets a flag >> somewhere that's going to be periodically checked in long-running >> loops. Would it be possible for the periodic checks to include a >> provision for a callback into Postgres-specific glue code, wherein >> you could test the same flags CHECK_FOR_INTERRUPTS does? A similar >> approach might then be usable in other contexts, and it seems safer >> to me than messing with a host environment's signal handling. > > Can we do that as a macro, e.g. > > POSTGRES_LIBRARY_INTERRUPT_CHECK() > > Otherwise the fix to this problem may be worse - faulty interrupt > handlers are worse than none at all. As it stands, ProcessInterrupts() sometimes returns instead of ereport()ing even if InterruptPending is set, interrupts aren't held off, and the code isn't in a critical section. That happens if QueryCancelPending (or however that's called) gets reset after a SIGINT arrived but before CHECK_FOR_INTERRUPTS() is called. Or at least that is how I interpret the comment at the bottom of that function... We could thus easily provide POSTGRES_LIBRARY_INTERRUPT_CHECK() if we're content with the (slim, probably) chance of false positives. Or we'd need to refactor things in a way that allows the hypothetical POSTGRES_LIBRARY_INTERRUPT_CHECK() to re-use the tests in ProcessInterrupts(), but without modifying any of the flags. best regards, Florian Pflug
On Fri, May 25, 2012 at 12:34:54PM -0400, Tom Lane wrote: > Sandro Santilli <strk@keybit.net> writes: > > I ended up providing an explicit mechanism to request interruption of > > whatever the library is doing, and experimented (successfully so far) > > requesting the interruption from a SIGINT handler. > > > Do you see any major drawback in doing so ? > > This seems a bit fragile. It might work all right in Postgres, where > we tend to set up signal handlers just once at process start, but ISTM > other systems might assume they can change their signal handlers at > any time. The handler itself looks less than portable anyway --- > what about the SIGINFO case? Indeed setting the handler from within the library itself was a temporary implementation to see how effective it would have been. The idea is to move the registration of the hanlder outside the library and into the user (PostGIS in this case). The library itself would only expose GEOS_requestInterrupt/GEOS_cancelInterrupt API calls. I'm guessing PostGIS should use the more portable pqsignal functions ? What should I know about SIGINFO ? > I assume that the geos::util::Interrupt::request() call sets a flag > somewhere that's going to be periodically checked in long-running > loops. Yes, this is what will happen. > Would it be possible for the periodic checks to include a > provision for a callback into Postgres-specific glue code, wherein > you could test the same flags CHECK_FOR_INTERRUPTS does? A similar > approach might then be usable in other contexts, and it seems safer > to me than messing with a host environment's signal handling. Would it be enough for the signal handler (installed by PostGIS) to check those flags and call the GEOS_requestInterrupt function when appropriate ? --strk; ,------o-. | __/ | Delivering high quality PostGIS 2.0 ! | / 2.0 | http://strk.keybit.net - http://vizzuality.com`-o------'
FYI, I finally committed the code installing a signal handler in PostGIS, using the pqsignal function: https://trac.osgeo.org/postgis/changeset/9850 It is currently only used to interrupt GEOS calls, but the idea is that it could eventually also be used to interrupt other library calls having a provision for such interruption request. GEOS itself only supports it in the 3.4 branch. In order to test it you'll need to define POSTGIS_ENABLE_GEOS_INTERRUPTIBILITY at the top of postgis/postgis_module.c - the macro is off by default due to concerns about possible consequences we may be unaware of. Your comments will be very useful to reduce (or confirm) the concerns. If we can get this confidently straight there may be the possibility to toggle the default to on before 2.0.1 ... Thanks in advance. PS: Tom, I still don't know what you mean by "the SIGINFO case". --strk; On Mon, May 28, 2012 at 08:48:21AM +0200, Sandro Santilli wrote: > On Fri, May 25, 2012 at 12:34:54PM -0400, Tom Lane wrote: > > Sandro Santilli <strk@keybit.net> writes: > > > I ended up providing an explicit mechanism to request interruption of > > > whatever the library is doing, and experimented (successfully so far) > > > requesting the interruption from a SIGINT handler. > > > > > Do you see any major drawback in doing so ? > > > > This seems a bit fragile. It might work all right in Postgres, where > > we tend to set up signal handlers just once at process start, but ISTM > > other systems might assume they can change their signal handlers at > > any time. The handler itself looks less than portable anyway --- > > what about the SIGINFO case? > > Indeed setting the handler from within the library itself was a temporary > implementation to see how effective it would have been. The idea is to > move the registration of the hanlder outside the library and into the > user (PostGIS in this case). The library itself would only expose > GEOS_requestInterrupt/GEOS_cancelInterrupt API calls. > > I'm guessing PostGIS should use the more portable pqsignal functions ? > > What should I know about SIGINFO ? > > > I assume that the geos::util::Interrupt::request() call sets a flag > > somewhere that's going to be periodically checked in long-running > > loops. > > Yes, this is what will happen. > > > Would it be possible for the periodic checks to include a > > provision for a callback into Postgres-specific glue code, wherein > > you could test the same flags CHECK_FOR_INTERRUPTS does? A similar > > approach might then be usable in other contexts, and it seems safer > > to me than messing with a host environment's signal handling. > > Would it be enough for the signal handler (installed by PostGIS) > to check those flags and call the GEOS_requestInterrupt function > when appropriate ? > > --strk;
On Sat, May 26, 2012 at 01:24:26PM +0200, Florian Pflug wrote: > On May26, 2012, at 12:40 , Simon Riggs wrote: > > On 25 May 2012 17:34, Tom Lane <tgl@sss.pgh.pa.us> wrote: > >> I assume that the geos::util::Interrupt::request() call sets a flag > >> somewhere that's going to be periodically checked in long-running > >> loops. Would it be possible for the periodic checks to include a > >> provision for a callback into Postgres-specific glue code, wherein > >> you could test the same flags CHECK_FOR_INTERRUPTS does? A similar > >> approach might then be usable in other contexts, and it seems safer > >> to me than messing with a host environment's signal handling. > > > > Can we do that as a macro, e.g. > > > > POSTGRES_LIBRARY_INTERRUPT_CHECK() > > > > Otherwise the fix to this problem may be worse - faulty interrupt > > handlers are worse than none at all. > > As it stands, ProcessInterrupts() sometimes returns instead of > ereport()ing even if InterruptPending is set, interrupts aren't > held off, and the code isn't in a critical section. That happens if > QueryCancelPending (or however that's called) gets reset after a > SIGINT arrived but before CHECK_FOR_INTERRUPTS() is called. Or at > least that is how I interpret the comment at the bottom of that > function... > > We could thus easily provide POSTGRES_LIBRARY_INTERRUPT_CHECK() if > we're content with the (slim, probably) chance of false positives. > > Or we'd need to refactor things in a way that allows the hypothetical > POSTGRES_LIBRARY_INTERRUPT_CHECK() to re-use the tests in > ProcessInterrupts(), but without modifying any of the flags. So back to this, shortly after discovering (sorry for ignorance, but I just don't care about programming in a black box environment) that windows doesn't support posix signals. If I understood correctly the CHECK_FOR_INTERRUPTS postgresql function does also take care of events dispatching within windows, so that if nothing calls that macro there's no way that a "pqsignal" handler would be invoked. Is that right ? In that case I can understand Tom's advice about providing a callback, and then I would only need to perform the "events flushing" part of from within the callback, and only for windows. Does it sound right ? --strk;
On Jun7, 2012, at 10:20 , Sandro Santilli wrote: > On Sat, May 26, 2012 at 01:24:26PM +0200, Florian Pflug wrote: >> On May26, 2012, at 12:40 , Simon Riggs wrote: >>> On 25 May 2012 17:34, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>>> I assume that the geos::util::Interrupt::request() call sets a flag >>>> somewhere that's going to be periodically checked in long-running >>>> loops. Would it be possible for the periodic checks to include a >>>> provision for a callback into Postgres-specific glue code, wherein >>>> you could test the same flags CHECK_FOR_INTERRUPTS does? A similar >>>> approach might then be usable in other contexts, and it seems safer >>>> to me than messing with a host environment's signal handling. >>> >>> Can we do that as a macro, e.g. >>> >>> POSTGRES_LIBRARY_INTERRUPT_CHECK() >>> >>> Otherwise the fix to this problem may be worse - faulty interrupt >>> handlers are worse than none at all. >> >> As it stands, ProcessInterrupts() sometimes returns instead of >> ereport()ing even if InterruptPending is set, interrupts aren't >> held off, and the code isn't in a critical section. That happens if >> QueryCancelPending (or however that's called) gets reset after a >> SIGINT arrived but before CHECK_FOR_INTERRUPTS() is called. Or at >> least that is how I interpret the comment at the bottom of that >> function... >> >> We could thus easily provide POSTGRES_LIBRARY_INTERRUPT_CHECK() if >> we're content with the (slim, probably) chance of false positives. >> >> Or we'd need to refactor things in a way that allows the hypothetical >> POSTGRES_LIBRARY_INTERRUPT_CHECK() to re-use the tests in >> ProcessInterrupts(), but without modifying any of the flags. > > So back to this, shortly after discovering (sorry for ignorance, but I > just don't care about programming in a black box environment) that windows > doesn't support posix signals. > > If I understood correctly the CHECK_FOR_INTERRUPTS postgresql function > does also take care of events dispatching within windows, so that if > nothing calls that macro there's no way that a "pqsignal" handler would > be invoked. Is that right ? Yes. > In that case I can understand Tom's advice about providing a callback, > and then I would only need to perform the "events flushing" part of > from within the callback, and only for windows. Why would you need a signal handler in the library at all, then? Just test the same flags that CHECK_FOR_INTERRUPTS does in the callback, and call your interrupt request method if they indicate "abort". (Or, slightly cleaner maybe, allow the callback to abort processing by returning false) best regards, Florian Pflug
On Thu, Jun 07, 2012 at 12:00:09PM +0200, Florian Pflug wrote: > On Jun7, 2012, at 10:20 , Sandro Santilli wrote: > > In that case I can understand Tom's advice about providing a callback, > > and then I would only need to perform the "events flushing" part of > > from within the callback, and only for windows. > > Why would you need a signal handler in the library at all, then? Just > test the same flags that CHECK_FOR_INTERRUPTS does in the callback, and > call your interrupt request method if they indicate "abort". (Or, slightly > cleaner maybe, allow the callback to abort processing by returning false) I'm just afraid that invoking a callback (from a library to user code) could be significantly slower than simply lookup a variable, especially if the interruption checking is performed very frequently. But maybe I'm being overparanoid. --strk; ,------o-. | __/ | Delivering high quality PostGIS 2.0 ! | / 2.0 | http://strk.keybit.net - http://vizzuality.com`-o------'
On Jun7, 2012, at 12:08 , Sandro Santilli wrote: > On Thu, Jun 07, 2012 at 12:00:09PM +0200, Florian Pflug wrote: >> On Jun7, 2012, at 10:20 , Sandro Santilli wrote: > >>> In that case I can understand Tom's advice about providing a callback, >>> and then I would only need to perform the "events flushing" part of >>> from within the callback, and only for windows. >> >> Why would you need a signal handler in the library at all, then? Just >> test the same flags that CHECK_FOR_INTERRUPTS does in the callback, and >> call your interrupt request method if they indicate "abort". (Or, slightly >> cleaner maybe, allow the callback to abort processing by returning false) > > I'm just afraid that invoking a callback (from a library to user code) > could be significantly slower than simply lookup a variable, especially > if the interruption checking is performed very frequently. But maybe I'm > being overparanoid. It's definitely slower, probably at least an order of magnitude. But you don't have to call that callback very often - once every few hundred milliseconds is already more then plenty. Isn't there a place in the library where you could put it where it'd be called with roughly that frequency? IMHO the advantage of not messing with signal handling in the library is more than worth the slight overhead of a callback, but YMMV. best regards, Florian Pflug