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

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(at)lelarge(dot)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(at)gmail(dot)com> writes:
> On Mon, Jun 21, 2010 at 12:46 PM, Guillaume Lelarge
> <guillaume(at)lelarge(dot)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(at)gmail(dot)com> writes:
>> On Mon, Jun 21, 2010 at 12:46 PM, Guillaume Lelarge
>> <guillaume(at)lelarge(dot)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(at)gmail(dot)com> writes:
>>> On Mon, Jun 21, 2010 at 12:46 PM, Guillaume Lelarge
>>> <guillaume(at)lelarge(dot)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(at)lelarge(dot)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(at)lelarge(dot)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(at)momjian(dot)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(at)momjian(dot)us> wrote:
> Tom Lane wrote:
>> Guillaume Lelarge <guillaume(at)lelarge(dot)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(at)gmail(dot)com> writes:
> On Tue, Jun 29, 2010 at 9:42 PM, Bruce Momjian <bruce(at)momjian(dot)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(at)gmail(dot)com> writes:
>> On Tue, Jun 29, 2010 at 9:42 PM, Bruce Momjian <bruce(at)momjian(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Tue, Jun 29, 2010 at 9:42 PM, Bruce Momjian <bruce(at)momjian(dot)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(at)gmail(dot)com> writes:
>>> On Tue, Jun 29, 2010 at 9:42 PM, Bruce Momjian <bruce(at)momjian(dot)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(at)lelarge(dot)info> wrote:
>>>> On Tue, Jun 29, 2010 at 9:42 PM, Bruce Momjian <bruce(at)momjian(dot)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(at)gmail(dot)com> wrote:
> On Thu, Jul 1, 2010 at 5:30 AM, Guillaume Lelarge
> <guillaume(at)lelarge(dot)info> wrote:
>>>>> On Tue, Jun 29, 2010 at 9:42 PM, Bruce Momjian <bruce(at)momjian(dot)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(at)gmail(dot)com> wrote:
>> On Thu, Jul 1, 2010 at 5:30 AM, Guillaume Lelarge
>> <guillaume(at)lelarge(dot)info> wrote:
>>>>>> On Tue, Jun 29, 2010 at 9:42 PM, Bruce Momjian <bruce(at)momjian(dot)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(at)lelarge(dot)info> wrote:
> Le 01/07/2010 17:54, Robert Haas a écrit :
>> On Thu, Jul 1, 2010 at 10:18 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>> On Thu, Jul 1, 2010 at 5:30 AM, Guillaume Lelarge
>>> <guillaume(at)lelarge(dot)info> wrote:
>>>>>>> On Tue, Jun 29, 2010 at 9:42 PM, Bruce Momjian <bruce(at)momjian(dot)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(at)lelarge(dot)info> wrote:
>> Le 01/07/2010 17:54, Robert Haas a écrit :
>>> On Thu, Jul 1, 2010 at 10:18 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>>> On Thu, Jul 1, 2010 at 5:30 AM, Guillaume Lelarge
>>>> <guillaume(at)lelarge(dot)info> wrote:
>>>>>>>> On Tue, Jun 29, 2010 at 9:42 PM, Bruce Momjian <bruce(at)momjian(dot)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