Thread: 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
Attachment
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? -- Guillaumehttp://www.postgresql.frhttp://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. -- Guillaumehttp://www.postgresql.frhttp://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. -- Guillaumehttp://www.postgresql.frhttp://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 -- Guillaumehttp://www.postgresql.frhttp://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. -- Guillaumehttp://www.postgresql.frhttp://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. -- Guillaumehttp://www.postgresql.frhttp://dalibo.com