Thread: Interrupting long external library calls

Interrupting long external library calls

From
Mark Cave-Ayland
Date:
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.


Re: Interrupting long external library calls

From
Heikki Linnakangas
Date:
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


Re: Interrupting long external library calls

From
Mark Cave-Ayland
Date:
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.


Re: Interrupting long external library calls

From
Heikki Linnakangas
Date:
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


Re: Interrupting long external library calls

From
Sandro Santilli
Date:
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------'
 



Re: Interrupting long external library calls

From
Heikki Linnakangas
Date:
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


Re: Interrupting long external library calls

From
Sandro Santilli
Date:
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; 


Re: Interrupting long external library calls

From
Heikki Linnakangas
Date:
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


Re: Interrupting long external library calls

From
Sandro Santilli
Date:
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;


Re: Interrupting long external library calls

From
Florian Pflug
Date:
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



Re: Interrupting long external library calls

From
Sandro Santilli
Date:
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------'
 



Re: Interrupting long external library calls

From
Tom Lane
Date:
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


Re: Interrupting long external library calls

From
Simon Riggs
Date:
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


Re: Interrupting long external library calls

From
Florian Pflug
Date:
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



Re: Interrupting long external library calls

From
Sandro Santilli
Date:
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------'
 



Re: Interrupting long external library calls

From
Sandro Santilli
Date:
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; 


Re: Interrupting long external library calls

From
Sandro Santilli
Date:
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; 


Re: Interrupting long external library calls

From
Florian Pflug
Date:
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



Re: Interrupting long external library calls

From
Sandro Santilli
Date:
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------'
 



Re: Interrupting long external library calls

From
Florian Pflug
Date:
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