Re: [HACKERS] GIN pending clean up is not interruptable

2015-09-03 Thread Andres Freund 
On 2015-09-03 12:45:34 +0900, Fujii Masao wrote:
> On Thu, Sep 3, 2015 at 2:15 AM, Andres Freund  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] GIN pending clean up is not interruptable

2015-09-03 Thread Fujii Masao 
On Thu, Sep 3, 2015 at 7:18 PM, Andres Freund  wrote:
> On 2015-09-03 12:45:34 +0900, Fujii Masao wrote:
>> On Thu, Sep 3, 2015 at 2:15 AM, Andres Freund  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] GIN pending clean up is not interruptable

2015-09-02 Thread Andres Freund 
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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] GIN pending clean up is not interruptable

2015-09-02 Thread Fujii Masao 
On Thu, Sep 3, 2015 at 2:15 AM, Andres Freund  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] GIN pending clean up is not interruptable

2015-08-12 Thread Jeff Janes 
On Tue, Aug 11, 2015 at 5:27 PM, Tom Lane t...@sss.pgh.pa.us wrote:

 Andres Freund and...@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.



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.

With this patch, ctrl-C and 'pg_ctl stop -mf' both behave nicely.

Cheers,

Jeff


gin_pendinglist_interrupt.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] GIN pending clean up is not interruptable

2015-08-11 Thread Jeff Janes 
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.

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.)

This issue is in all branches.

Cheers,

Jeff


gin_freelist_interrupt.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] GIN pending clean up is not interruptable

2015-08-11 Thread Andres Freund 
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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] GIN pending clean up is not interruptable

2015-08-11 Thread Tom Lane 
Andres Freund and...@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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers