Cannot cancel the change of a tablespace
Hi,
Today, I tried to cancel the change of a tablespace for a table (ALTER
TABLE ... SET TABLESPACE). I got the "Cancel request sent" but the query
continued and finally succeed. It was a big issue for my customer, and I
wanted to look more into that issue. So, I got a look at the source code
and found we didn't check for interrupts in this part of the code. I
added them, and it seems to work as I wanted.
I added a CHECK_FOR_INTERRUPTS call in the copy_relation_data(),
copy_dir(), and copy_file() functions. Works for me on ALTER TABLE ...
SET TABLESPACE and ALTER DATABASE ... SET TABLESPACE, in 9.0 and 8.4.
Not sure we really want that change, and it don't feel like a bug to me.
Should I add it to to the next commitfest?
Comments?
--
Guillaume
http://www.postgresql.fr
http://dalibo.com
Attachments:
tbs_interrups_v1.patchtext/x-patch; name=tbs_interrups_v1.patchDownload
Index: src/backend/commands/tablecmds.c
===================================================================
RCS file: /opt/cvsroot_postgresql/pgsql/src/backend/commands/tablecmds.c,v
retrieving revision 1.330
diff -c -p -c -r1.330 tablecmds.c
*** src/backend/commands/tablecmds.c 28 Apr 2010 16:10:41 -0000 1.330
--- src/backend/commands/tablecmds.c 21 Jun 2010 16:33:30 -0000
*************** copy_relation_data(SMgrRelation src, SMg
*** 7049,7054 ****
--- 7049,7057 ----
for (blkno = 0; blkno < nblocks; blkno++)
{
+ /* If we got a cancel signal during the copy of the data, quit */
+ CHECK_FOR_INTERRUPTS();
+
smgrread(src, forkNum, blkno, buf);
/* XLOG stuff */
Index: src/port/copydir.c
===================================================================
RCS file: /opt/cvsroot_postgresql/pgsql/src/port/copydir.c,v
retrieving revision 1.36
diff -c -p -c -r1.36 copydir.c
*** src/port/copydir.c 1 Mar 2010 14:54:00 -0000 1.36
--- src/port/copydir.c 21 Jun 2010 16:33:30 -0000
***************
*** 23,28 ****
--- 23,29 ----
#include <sys/stat.h>
#include "storage/fd.h"
+ #include "miscadmin.h"
/*
* On Windows, call non-macro versions of palloc; we can't reference
*************** copydir(char *fromdir, char *todir, bool
*** 67,72 ****
--- 68,76 ----
while ((xlde = ReadDir(xldir, fromdir)) != NULL)
{
+ /* If we got a cancel signal during the copy of the directory, quit */
+ CHECK_FOR_INTERRUPTS();
+
struct stat fst;
if (strcmp(xlde->d_name, ".") == 0 ||
*************** copy_file(char *fromfile, char *tofile)
*** 172,177 ****
--- 176,184 ----
*/
for (offset = 0;; offset += nbytes)
{
+ /* If we got a cancel signal during the copy of the file, quit */
+ CHECK_FOR_INTERRUPTS();
+
nbytes = read(srcfd, buffer, COPY_BUF_SIZE);
if (nbytes < 0)
ereport(ERROR,
On Mon, 2010-06-21 at 18:46 +0200, Guillaume Lelarge wrote:
Today, I tried to cancel the change of a tablespace for a table (ALTER
TABLE ... SET TABLESPACE). I got the "Cancel request sent" but the query
continued and finally succeed. It was a big issue for my customer, and I
wanted to look more into that issue. So, I got a look at the source code
and found we didn't check for interrupts in this part of the code. I
added them, and it seems to work as I wanted.I added a CHECK_FOR_INTERRUPTS call in the copy_relation_data(),
copy_dir(), and copy_file() functions. Works for me on ALTER TABLE ...
SET TABLESPACE and ALTER DATABASE ... SET TABLESPACE, in 9.0 and 8.4.Not sure we really want that change, and it don't feel like a bug to me.
Should I add it to to the next commitfest?
Patch looks fine to me. Seems important.
Will apply tomorrow to 9.0, barring objections.
--
Simon Riggs www.2ndQuadrant.com
On Mon, Jun 21, 2010 at 12:46 PM, Guillaume Lelarge
<guillaume@lelarge.info> wrote:
Today, I tried to cancel the change of a tablespace for a table (ALTER
TABLE ... SET TABLESPACE). I got the "Cancel request sent" but the query
continued and finally succeed. It was a big issue for my customer, and I
wanted to look more into that issue. So, I got a look at the source code
and found we didn't check for interrupts in this part of the code. I
added them, and it seems to work as I wanted.I added a CHECK_FOR_INTERRUPTS call in the copy_relation_data(),
copy_dir(), and copy_file() functions. Works for me on ALTER TABLE ...
SET TABLESPACE and ALTER DATABASE ... SET TABLESPACE, in 9.0 and 8.4.Not sure we really want that change, and it don't feel like a bug to me.
Should I add it to to the next commitfest?
Adding a CHECK_FOR_INTERRUPTS() to copy_relation_data seems like it
ought to be OK (though I haven't tested), but copydir() is in
src/port, and I fear that putting CHECK_FOR_INTERRUPTS() in there
might cause problems.
I think that whatever portion of this we end up applying should be back-patched.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company
Robert Haas <robertmhaas@gmail.com> writes:
On Mon, Jun 21, 2010 at 12:46 PM, Guillaume Lelarge
<guillaume@lelarge.info> wrote:I added a CHECK_FOR_INTERRUPTS call in the copy_relation_data(),
copy_dir(), and copy_file() functions. Works for me on ALTER TABLE ...
SET TABLESPACE and ALTER DATABASE ... SET TABLESPACE, in 9.0 and 8.4.
Adding a CHECK_FOR_INTERRUPTS() to copy_relation_data seems like it
ought to be OK (though I haven't tested), but copydir() is in
src/port, and I fear that putting CHECK_FOR_INTERRUPTS() in there
might cause problems.
copydir.c is already backend-specific thanks to all the ereport calls.
If we ever tried to make it usable in frontend code, we could easily
deal with CHECK_FOR_INTERRUPTS() via #ifndef FRONTEND --- changing the
error management would be far more painful.
regards, tom lane
Le 23/06/2010 22:54, Tom Lane a �crit :
Robert Haas <robertmhaas@gmail.com> writes:
On Mon, Jun 21, 2010 at 12:46 PM, Guillaume Lelarge
<guillaume@lelarge.info> wrote:I added a CHECK_FOR_INTERRUPTS call in the copy_relation_data(),
copy_dir(), and copy_file() functions. Works for me on ALTER TABLE ...
SET TABLESPACE and ALTER DATABASE ... SET TABLESPACE, in 9.0 and 8.4.Adding a CHECK_FOR_INTERRUPTS() to copy_relation_data seems like it
ought to be OK (though I haven't tested), but copydir() is in
src/port, and I fear that putting CHECK_FOR_INTERRUPTS() in there
might cause problems.copydir.c is already backend-specific thanks to all the ereport calls.
If we ever tried to make it usable in frontend code, we could easily
deal with CHECK_FOR_INTERRUPTS() via #ifndef FRONTEND --- changing the
error management would be far more painful.
I'm not sure I get it right. Do I need to do something on the patch so
that it can get commited?
--
Guillaume
http://www.postgresql.fr
http://dalibo.com
Le 23/06/2010 23:29, Guillaume Lelarge a �crit :
Le 23/06/2010 22:54, Tom Lane a �crit :
Robert Haas <robertmhaas@gmail.com> writes:
On Mon, Jun 21, 2010 at 12:46 PM, Guillaume Lelarge
<guillaume@lelarge.info> wrote:I added a CHECK_FOR_INTERRUPTS call in the copy_relation_data(),
copy_dir(), and copy_file() functions. Works for me on ALTER TABLE ...
SET TABLESPACE and ALTER DATABASE ... SET TABLESPACE, in 9.0 and 8.4.Adding a CHECK_FOR_INTERRUPTS() to copy_relation_data seems like it
ought to be OK (though I haven't tested), but copydir() is in
src/port, and I fear that putting CHECK_FOR_INTERRUPTS() in there
might cause problems.copydir.c is already backend-specific thanks to all the ereport calls.
If we ever tried to make it usable in frontend code, we could easily
deal with CHECK_FOR_INTERRUPTS() via #ifndef FRONTEND --- changing the
error management would be far more painful.I'm not sure I get it right. Do I need to do something on the patch so
that it can get commited?
Still not sure what to do right now for this patch :)
Could it be applied? or should I work on it? (and if yes on the latter,
to do what?)
Thanks.
--
Guillaume
http://www.postgresql.fr
http://dalibo.com
Guillaume Lelarge <guillaume@lelarge.info> writes:
Still not sure what to do right now for this patch :)
Put it on the commitfest list, if you didn't already.
regards, tom lane
Tom Lane wrote:
Guillaume Lelarge <guillaume@lelarge.info> writes:
Still not sure what to do right now for this patch :)
Put it on the commitfest list, if you didn't already.
So this is not something we want fixed for 9.0, as indicated by Simon?
I don't see the patch on the commit-fest page yet.
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ None of us is going to be here forever. +
On Tue, Jun 29, 2010 at 9:42 PM, Bruce Momjian <bruce@momjian.us> wrote:
Tom Lane wrote:
Guillaume Lelarge <guillaume@lelarge.info> writes:
Still not sure what to do right now for this patch :)
Put it on the commitfest list, if you didn't already.
So this is not something we want fixed for 9.0, as indicated by Simon?
I don't see the patch on the commit-fest page yet.
I tend to think we should fix it for 9.0, but could be talked out of
it if someone has a compelling argument to make.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company
Robert Haas <robertmhaas@gmail.com> writes:
On Tue, Jun 29, 2010 at 9:42 PM, Bruce Momjian <bruce@momjian.us> wrote:
So this is not something we want fixed for 9.0, as indicated by Simon?
I don't see the patch on the commit-fest page yet.
I tend to think we should fix it for 9.0, but could be talked out of
it if someone has a compelling argument to make.
Er, maybe I lost count, but I thought you were the one objecting to
the patch.
regards, tom lane
Le 30/06/2010 05:25, Tom Lane a �crit :
Robert Haas <robertmhaas@gmail.com> writes:
On Tue, Jun 29, 2010 at 9:42 PM, Bruce Momjian <bruce@momjian.us> wrote:
So this is not something we want fixed for 9.0, as indicated by Simon?
I don't see the patch on the commit-fest page yet.I tend to think we should fix it for 9.0, but could be talked out of
it if someone has a compelling argument to make.Er, maybe I lost count, but I thought you were the one objecting to
the patch.
You're right. Robert questioned the use of CHECK_FOR_INTERRUPTS() in
code available in the src/port directory. I don't see what issue could
result with this. He also said that whatever would be commited should be
back-patched.
I can still add it for the next commit fest, I just don't want this
patch to get lost. Though I won't be able to do this before getting back
from work.
Thanks.
--
Guillaume
http://www.postgresql.fr
http://dalibo.com
On Tue, Jun 29, 2010 at 11:25 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Robert Haas <robertmhaas@gmail.com> writes:
On Tue, Jun 29, 2010 at 9:42 PM, Bruce Momjian <bruce@momjian.us> wrote:
So this is not something we want fixed for 9.0, as indicated by Simon?
I don't see the patch on the commit-fest page yet.I tend to think we should fix it for 9.0, but could be talked out of
it if someone has a compelling argument to make.Er, maybe I lost count, but I thought you were the one objecting to
the patch.
No, I just wasn't sure whether it was safe. If it's safe, I'm 100% in
favor of applying it and back-patching.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company
Le 30/06/2010 06:53, Guillaume Lelarge a �crit :
Le 30/06/2010 05:25, Tom Lane a �crit :
Robert Haas <robertmhaas@gmail.com> writes:
On Tue, Jun 29, 2010 at 9:42 PM, Bruce Momjian <bruce@momjian.us> wrote:
So this is not something we want fixed for 9.0, as indicated by Simon?
I don't see the patch on the commit-fest page yet.I tend to think we should fix it for 9.0, but could be talked out of
it if someone has a compelling argument to make.Er, maybe I lost count, but I thought you were the one objecting to
the patch.You're right. Robert questioned the use of CHECK_FOR_INTERRUPTS() in
code available in the src/port directory. I don't see what issue could
result with this. He also said that whatever would be commited should be
back-patched.I can still add it for the next commit fest, I just don't want this
patch to get lost. Though I won't be able to do this before getting back
from work.
Finally, I added it to the next commit fest. Robert can work on it
before if he wants to (or has the time).
https://commitfest.postgresql.org/action/patch_view?id=331
--
Guillaume
http://www.postgresql.fr
http://dalibo.com
On Thu, Jul 1, 2010 at 5:30 AM, Guillaume Lelarge
<guillaume@lelarge.info> wrote:
On Tue, Jun 29, 2010 at 9:42 PM, Bruce Momjian <bruce@momjian.us> wrote:
So this is not something we want fixed for 9.0, as indicated by Simon?
I don't see the patch on the commit-fest page yet.Finally, I added it to the next commit fest. Robert can work on it
before if he wants to (or has the time).
I'd been avoiding working on this because Simon had said he was going
to commit it, but I can pick it up. I've committed and back-patched
(to 8.0, as 7.4 does not have tablespaces) the fix for ALTER TABLE ..
SET TABLESPACE. I'll take a look at the rest of it as well.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company
On Thu, Jul 1, 2010 at 10:18 AM, Robert Haas <robertmhaas@gmail.com> wrote:
On Thu, Jul 1, 2010 at 5:30 AM, Guillaume Lelarge
<guillaume@lelarge.info> wrote:On Tue, Jun 29, 2010 at 9:42 PM, Bruce Momjian <bruce@momjian.us> wrote:
So this is not something we want fixed for 9.0, as indicated by Simon?
I don't see the patch on the commit-fest page yet.Finally, I added it to the next commit fest. Robert can work on it
before if he wants to (or has the time).I'd been avoiding working on this because Simon had said he was going
to commit it, but I can pick it up. I've committed and back-patched
(to 8.0, as 7.4 does not have tablespaces) the fix for ALTER TABLE ..
SET TABLESPACE. I'll take a look at the rest of it as well.
It looks like we have two reasonable choices here:
- We could backpatch this only to 8.4, where ALTER DATABASE .. SET
TABLESPACE was introduced.
- Or, since this also makes it easier to interrupt CREATE DATABASE new
TEMPLATE = some_big_database, we could back-patch it all the way to
8.1, which is the first release where we use copydir() rather than
invoking cp -r (except on Windows, where copydir() has always been
used, but releases < 8.2 aren't supported on Windows anyway).
Since I can't remember anyone complaining about difficulty
interrupting CREATE DATABASE, I'm inclined to go back only to 8.4, and
will do that a bit later.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company
Le 01/07/2010 17:54, Robert Haas a �crit :
On Thu, Jul 1, 2010 at 10:18 AM, Robert Haas <robertmhaas@gmail.com> wrote:
On Thu, Jul 1, 2010 at 5:30 AM, Guillaume Lelarge
<guillaume@lelarge.info> wrote:On Tue, Jun 29, 2010 at 9:42 PM, Bruce Momjian <bruce@momjian.us> wrote:
So this is not something we want fixed for 9.0, as indicated by Simon?
I don't see the patch on the commit-fest page yet.Finally, I added it to the next commit fest. Robert can work on it
before if he wants to (or has the time).I'd been avoiding working on this because Simon had said he was going
to commit it, but I can pick it up. I've committed and back-patched
(to 8.0, as 7.4 does not have tablespaces) the fix for ALTER TABLE ..
SET TABLESPACE. I'll take a look at the rest of it as well.It looks like we have two reasonable choices here:
- We could backpatch this only to 8.4, where ALTER DATABASE .. SET
TABLESPACE was introduced.- Or, since this also makes it easier to interrupt CREATE DATABASE new
TEMPLATE = some_big_database, we could back-patch it all the way to
8.1, which is the first release where we use copydir() rather than
invoking cp -r (except on Windows, where copydir() has always been
used, but releases < 8.2 aren't supported on Windows anyway).Since I can't remember anyone complaining about difficulty
interrupting CREATE DATABASE, I'm inclined to go back only to 8.4, and
will do that a bit later.
I agree that a backpatch to 8.4 seems enough.
--
Guillaume
http://www.postgresql.fr
http://dalibo.com
On Thu, Jul 1, 2010 at 12:11 PM, Guillaume Lelarge
<guillaume@lelarge.info> wrote:
Le 01/07/2010 17:54, Robert Haas a écrit :
On Thu, Jul 1, 2010 at 10:18 AM, Robert Haas <robertmhaas@gmail.com> wrote:
On Thu, Jul 1, 2010 at 5:30 AM, Guillaume Lelarge
<guillaume@lelarge.info> wrote:On Tue, Jun 29, 2010 at 9:42 PM, Bruce Momjian <bruce@momjian.us> wrote:
So this is not something we want fixed for 9.0, as indicated by Simon?
I don't see the patch on the commit-fest page yet.Finally, I added it to the next commit fest. Robert can work on it
before if he wants to (or has the time).I'd been avoiding working on this because Simon had said he was going
to commit it, but I can pick it up. I've committed and back-patched
(to 8.0, as 7.4 does not have tablespaces) the fix for ALTER TABLE ..
SET TABLESPACE. I'll take a look at the rest of it as well.It looks like we have two reasonable choices here:
- We could backpatch this only to 8.4, where ALTER DATABASE .. SET
TABLESPACE was introduced.- Or, since this also makes it easier to interrupt CREATE DATABASE new
TEMPLATE = some_big_database, we could back-patch it all the way to
8.1, which is the first release where we use copydir() rather than
invoking cp -r (except on Windows, where copydir() has always been
used, but releases < 8.2 aren't supported on Windows anyway).Since I can't remember anyone complaining about difficulty
interrupting CREATE DATABASE, I'm inclined to go back only to 8.4, and
will do that a bit later.I agree that a backpatch to 8.4 seems enough.
Done.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company
Le 01/07/2010 22:13, Robert Haas a �crit :
On Thu, Jul 1, 2010 at 12:11 PM, Guillaume Lelarge
<guillaume@lelarge.info> wrote:Le 01/07/2010 17:54, Robert Haas a �crit :
On Thu, Jul 1, 2010 at 10:18 AM, Robert Haas <robertmhaas@gmail.com> wrote:
On Thu, Jul 1, 2010 at 5:30 AM, Guillaume Lelarge
<guillaume@lelarge.info> wrote:On Tue, Jun 29, 2010 at 9:42 PM, Bruce Momjian <bruce@momjian.us> wrote:
So this is not something we want fixed for 9.0, as indicated by Simon?
I don't see the patch on the commit-fest page yet.Finally, I added it to the next commit fest. Robert can work on it
before if he wants to (or has the time).I'd been avoiding working on this because Simon had said he was going
to commit it, but I can pick it up. I've committed and back-patched
(to 8.0, as 7.4 does not have tablespaces) the fix for ALTER TABLE ..
SET TABLESPACE. I'll take a look at the rest of it as well.It looks like we have two reasonable choices here:
- We could backpatch this only to 8.4, where ALTER DATABASE .. SET
TABLESPACE was introduced.- Or, since this also makes it easier to interrupt CREATE DATABASE new
TEMPLATE = some_big_database, we could back-patch it all the way to
8.1, which is the first release where we use copydir() rather than
invoking cp -r (except on Windows, where copydir() has always been
used, but releases < 8.2 aren't supported on Windows anyway).Since I can't remember anyone complaining about difficulty
interrupting CREATE DATABASE, I'm inclined to go back only to 8.4, and
will do that a bit later.I agree that a backpatch to 8.4 seems enough.
Done.
Thanks, Robert.
--
Guillaume
http://www.postgresql.fr
http://dalibo.com