Thread: GIN pending clean up is not interruptable

On 2015-08-11 15:07:15 -0700, Jeff Janes wrote:
> When a user backend (as opposed to vacuum or autoanalyze) gets burdened
> with cleaning up the GIN pending list, it does not
> call CHECK_FOR_INTERRUPTS().
> 
> Since cleaning does a lot of random IO, it can take a long time and it is
> not nice to be uninterruptable.

Agreed.

> The attached patch adds an else branch to call CHECK_FOR_INTERRUPTS().
> 
> But I think we could instead just call vacuum_delay_point unconditionally.
> It calls CHECK_FOR_INTERRUPTS(), and if not in a throttled vacuum it does
> nothing else.  (That is how ANALYZE handles it.)

Hm, I find that not exactly pretty. I'd rather just add an unconditional
CHECK_FOR_INTERRUPTS to the function.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:
> On 2015-08-11 15:07:15 -0700, Jeff Janes wrote:
>> The attached patch adds an else branch to call CHECK_FOR_INTERRUPTS().
>> 
>> But I think we could instead just call vacuum_delay_point unconditionally.
>> It calls CHECK_FOR_INTERRUPTS(), and if not in a throttled vacuum it does
>> nothing else.  (That is how ANALYZE handles it.)

> Hm, I find that not exactly pretty. I'd rather just add an unconditional
> CHECK_FOR_INTERRUPTS to the function.

CHECK_FOR_INTERRUPTS is very cheap.  But I tend to agree that you should
be using vacuum_delay_point.
        regards, tom lane

On 2015-08-12 11:59:48 -0700, Jeff Janes wrote:
> Attached patch does it that way.  There was also a free-standing
> CHECK_FOR_INTERRUPTS() which had no reason that I could see not be a
> vacuum_delay_point, so I changed that one as well.

I think we should backpatch this - any arguments against?

Andres

On Thu, Sep 3, 2015 at 2:15 AM, Andres Freund <andres@anarazel.de> wrote:
> On 2015-08-12 11:59:48 -0700, Jeff Janes wrote:
>> Attached patch does it that way.  There was also a free-standing
>> CHECK_FOR_INTERRUPTS() which had no reason that I could see not be a
>> vacuum_delay_point, so I changed that one as well.

-        if (vac_delay)
-            vacuum_delay_point();
+        vacuum_delay_point();

If vac_delay is false, e.g., ginInsertCleanup() is called by the backend,
vacuum_delay_point() should not be called. No?

> I think we should backpatch this - any arguments against?

+1 for backpatch.

Regards,

-- 
Fujii Masao

On 2015-09-03 12:45:34 +0900, Fujii Masao wrote:
> On Thu, Sep 3, 2015 at 2:15 AM, Andres Freund <andres@anarazel.de> wrote:
> > On 2015-08-12 11:59:48 -0700, Jeff Janes wrote:
> >> Attached patch does it that way.  There was also a free-standing
> >> CHECK_FOR_INTERRUPTS() which had no reason that I could see not be a
> >> vacuum_delay_point, so I changed that one as well.
> 
> -        if (vac_delay)
> -            vacuum_delay_point();
> +        vacuum_delay_point();
> 
> If vac_delay is false, e.g., ginInsertCleanup() is called by the backend,
> vacuum_delay_point() should not be called. No?

No, that's the whole point of the change, we need a
CHECK_FOR_INTERRUPTS() even when called by backends. I personally think
it's rather ugly to rely on the the one in vacuum_delay_point, but Jeff
and Tom think it's better, and I can live with that.

Greetings,

Andres Freund

On Thu, Sep 3, 2015 at 7:18 PM, Andres Freund <andres@anarazel.de> wrote:
> On 2015-09-03 12:45:34 +0900, Fujii Masao wrote:
>> On Thu, Sep 3, 2015 at 2:15 AM, Andres Freund <andres@anarazel.de> wrote:
>> > On 2015-08-12 11:59:48 -0700, Jeff Janes wrote:
>> >> Attached patch does it that way.  There was also a free-standing
>> >> CHECK_FOR_INTERRUPTS() which had no reason that I could see not be a
>> >> vacuum_delay_point, so I changed that one as well.
>>
>> -        if (vac_delay)
>> -            vacuum_delay_point();
>> +        vacuum_delay_point();
>>
>> If vac_delay is false, e.g., ginInsertCleanup() is called by the backend,
>> vacuum_delay_point() should not be called. No?
>
> No, that's the whole point of the change, we need a
> CHECK_FOR_INTERRUPTS() even when called by backends. I personally think
> it's rather ugly to rely on the the one in vacuum_delay_point,

Same here.

> but Jeff
> and Tom think it's better, and I can live with that.

OK, probably I can live with that, too.

Regards,

-- 
Fujii Masao