GIN pending clean up is not interruptable

Started by Jeff Janesover 10 years ago8 messages

jeff.janes@gmail.com

over 10 years ago

1 attachment(s)

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

Attachments:

gin_freelist_interrupt.patchapplication/octet-stream; name=gin_freelist_interrupt.patchDownload

diff --git a/src/backend/access/gin/ginfast.c b/src/backend/access/gin/ginfast.c
new file mode 100644
index c5732c3..2472e90
*** a/src/backend/access/gin/ginfast.c
--- b/src/backend/access/gin/ginfast.c
*************** ginInsertCleanup(GinState *ginstate,
*** 803,808 ****
--- 805,812 ----
  
  		if (vac_delay)
  			vacuum_delay_point();
+ 		else
+ 			CHECK_FOR_INTERRUPTS();
  
  		/*
  		 * Is it time to flush memory to disk?	Flush if we are at the end of
*************** ginInsertCleanup(GinState *ginstate,
*** 844,849 ****
--- 848,855 ----
  							   list, nlist, NULL);
  				if (vac_delay)
  					vacuum_delay_point();
+ 				else
+ 					CHECK_FOR_INTERRUPTS();
  			}
  
  			/*

Andres Freund

andres@anarazel.de

over 10 years ago

In reply to: Jeff Janes (#1)

Re: 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

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

Tom Lane

tgl@sss.pgh.pa.us

over 10 years ago

In reply to: Andres Freund (#2)

Re: GIN pending clean up is not interruptable

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

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

Jeff Janes

jeff.janes@gmail.com

over 10 years ago

In reply to: Tom Lane (#3)

1 attachment(s)

Re: GIN pending clean up is not interruptable

On Tue, Aug 11, 2015 at 5:27 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

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.

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

Attachments:

gin_pendinglist_interrupt.patchapplication/octet-stream; name=gin_pendinglist_interrupt.patchDownload

diff --git a/src/backend/access/gin/ginfast.c b/src/backend/access/gin/ginfast.c
new file mode 100644
index c5732c3..8770eb9
*** a/src/backend/access/gin/ginfast.c
--- b/src/backend/access/gin/ginfast.c
*************** ginInsertCleanup(GinState *ginstate,
*** 801,808 ****
  		 */
  		processPendingPage(&accum, &datums, page, FirstOffsetNumber);
  
! 		if (vac_delay)
! 			vacuum_delay_point();
  
  		/*
  		 * Is it time to flush memory to disk?	Flush if we are at the end of
--- 801,807 ----
  		 */
  		processPendingPage(&accum, &datums, page, FirstOffsetNumber);
  
! 		vacuum_delay_point();
  
  		/*
  		 * Is it time to flush memory to disk?	Flush if we are at the end of
*************** ginInsertCleanup(GinState *ginstate,
*** 842,849 ****
  			{
  				ginEntryInsert(ginstate, attnum, key, category,
  							   list, nlist, NULL);
! 				if (vac_delay)
! 					vacuum_delay_point();
  			}
  
  			/*
--- 841,847 ----
  			{
  				ginEntryInsert(ginstate, attnum, key, category,
  							   list, nlist, NULL);
! 				vacuum_delay_point();
  			}
  
  			/*
*************** ginInsertCleanup(GinState *ginstate,
*** 923,929 ****
  		/*
  		 * Read next page in pending list
  		 */
! 		CHECK_FOR_INTERRUPTS();
  		buffer = ReadBuffer(index, blkno);
  		LockBuffer(buffer, GIN_SHARE);
  		page = BufferGetPage(buffer);
--- 921,927 ----
  		/*
  		 * Read next page in pending list
  		 */
! 		vacuum_delay_point();
  		buffer = ReadBuffer(index, blkno);
  		LockBuffer(buffer, GIN_SHARE);
  		page = BufferGetPage(buffer);

Andres Freund

andres@anarazel.de

over 10 years ago

In reply to: Jeff Janes (#4)

Re: GIN pending clean up is not interruptable

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

Fujii Masao

masao.fujii@gmail.com

over 10 years ago

In reply to: Andres Freund (#5)

Re: GIN pending clean up is not interruptable

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

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

Andres Freund

andres@anarazel.de

over 10 years ago

In reply to: Fujii Masao (#6)

Re: GIN pending clean up is not interruptable

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

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

Fujii Masao

masao.fujii@gmail.com

over 10 years ago

In reply to: Andres Freund (#7)

Re: GIN pending clean up is not interruptable

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

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