Thread: Patch for ALTER DATABASE WITH TABLESPACE

Patch for ALTER DATABASE WITH TABLESPACE

From
Guillaume Lelarge
Date:
Hi,

Here is my patch to add the ALTER DATABASE WITH TABLESPACE statement. It
is part of the TODO list. It intends to allow the move of all relations
of a database in its new default tablespace.

Comments welcome.

Regards.


--
Guillaume.
 http://www.postgresqlfr.org
 http://dalibo.com

Attachment

Re: Patch for ALTER DATABASE WITH TABLESPACE

From
Bernd Helmle
Date:
--On Samstag, Oktober 25, 2008 23:50:47 +0200 Guillaume Lelarge
<guillaume@lelarge.info> wrote:

> Hi,
>
> Here is my patch to add the ALTER DATABASE WITH TABLESPACE statement. It
> is part of the TODO list. It intends to allow the move of all relations
> of a database in its new default tablespace.
>
> Comments welcome.

I had a first look on this and in my opinion the patch looks reasonable. I
moved the usage of heap_modifytuple() to the new heap_modify_tuple() API
(see attached new diff) and did other minor cleanups.

However, i'm not satisfied with the syntax, which is currently ALTER
DATABASE name TABLESPACE foo. We use all over the place SET TABLESPACE
(e.g. for tables and indexes) and SET SCHEMA for namespaces even, so this
looks inconsistent. However, hacking this requires a little bit more
parser-foo, a quick hack shows reduce conflicts due to SetResetClause rule.
So what do we want in this case?

I did some minor additions in the docs as well.

--
  Thanks

                    Bernd
Attachment

Re: Patch for ALTER DATABASE WITH TABLESPACE

From
Guillaume Lelarge
Date:
Bernd Helmle a écrit :
> --On Samstag, Oktober 25, 2008 23:50:47 +0200 Guillaume Lelarge
> <guillaume@lelarge.info> wrote:
> 
>> Here is my patch to add the ALTER DATABASE WITH TABLESPACE statement. It
>> is part of the TODO list. It intends to allow the move of all relations
>> of a database in its new default tablespace.
>>
>> Comments welcome.
> 
> I had a first look on this and in my opinion the patch looks reasonable.
> I moved the usage of heap_modifytuple() to the new heap_modify_tuple()
> API (see attached new diff) and did other minor cleanups.
> 

OK.

> However, i'm not satisfied with the syntax, which is currently ALTER
> DATABASE name TABLESPACE foo. We use all over the place SET TABLESPACE
> (e.g. for tables and indexes) and SET SCHEMA for namespaces even, so
> this looks inconsistent. However, hacking this requires a little bit
> more parser-foo, a quick hack shows reduce conflicts due to
> SetResetClause rule. So what do we want in this case?
> 

My first intent was to use SET TABLESPACE. But the other parameter
available in the ALTER DATABASE statement use the WITH syntax. So, to be
coherent with the actual ALTER DATABASE statement, I used the WITH syntax.

I know this is not coherent with ALTER TABLE, but it is with ALTER DATABASE.

Anyway, if many think I need to change this, I'll try it.

> I did some minor additions in the docs as well.
> 

Thanks for your review.


-- 
Guillaume.http://www.postgresqlfr.orghttp://dalibo.com


Re: Patch for ALTER DATABASE WITH TABLESPACE

From
Tom Lane
Date:
Guillaume Lelarge <guillaume@lelarge.info> writes:
> Bernd Helmle a �crit :
>> However, i'm not satisfied with the syntax, which is currently ALTER
>> DATABASE name TABLESPACE foo. We use all over the place SET TABLESPACE
>> (e.g. for tables and indexes) and SET SCHEMA for namespaces even, so
>> this looks inconsistent. However, hacking this requires a little bit
>> more parser-foo, a quick hack shows reduce conflicts due to
>> SetResetClause rule. So what do we want in this case?

> My first intent was to use SET TABLESPACE. But the other parameter
> available in the ALTER DATABASE statement use the WITH syntax. So, to be
> coherent with the actual ALTER DATABASE statement, I used the WITH syntax.

FWIW, bison seems perfectly happy with this:
 AlterDatabaseStmt:             ALTER DATABASE database_name opt_with alterdb_opt_list                  {
     AlterDatabaseStmt *n = makeNode(AlterDatabaseStmt);                     n->dbname = $3;
n->options= $5;                     $$ = (Node *)n;                  }
 
+             | ALTER DATABASE database_name SET TABLESPACE name
+                  {
+                     AlterDatabaseStmt *n = makeNode(AlterDatabaseStmt);
+                     n->dbname = $3;
+                     ...
+                     $$ = (Node *)n;
+                  }         ;

Not sure what Bernd tried exactly, but it can be done.

I see the point about the parallel to CREATE DATABASE, but on the other
hand we also have ALTER DATABASE SET for parameters.  I suspect people
are more likely to expect the SET syntax.
        regards, tom lane



Re: Patch for ALTER DATABASE WITH TABLESPACE

From
Bernd Helmle
Date:
--On Dienstag, November 04, 2008 14:56:44 -0500 Tom Lane 
<tgl@sss.pgh.pa.us> wrote:

[...]

>
> Not sure what Bernd tried exactly, but it can be done.
>

Cool, i didn't recognize the obvious possibility to add a separate rule for 
this. I've just extended the alterdb_opt_item with SET TABLESPACE, which 
lead to a shift/reduce.

> I see the point about the parallel to CREATE DATABASE, but on the other
> hand we also have ALTER DATABASE SET for parameters.  I suspect people
> are more likely to expect the SET syntax.
>

Yes, that seems logical to me, too. So i think we should go for it. 
Guillaume?


--  Thanks
                   Bernd


Re: Patch for ALTER DATABASE WITH TABLESPACE

From
Guillaume Lelarge
Date:
Bernd Helmle a écrit :
> --On Dienstag, November 04, 2008 14:56:44 -0500 Tom Lane
> <tgl@sss.pgh.pa.us> wrote:
> 
> [...]
> 
>>
>> Not sure what Bernd tried exactly, but it can be done.
>>
> 
> Cool, i didn't recognize the obvious possibility to add a separate rule
> for this. I've just extended the alterdb_opt_item with SET TABLESPACE,
> which lead to a shift/reduce.
> 
>> I see the point about the parallel to CREATE DATABASE, but on the other
>> hand we also have ALTER DATABASE SET for parameters.  I suspect people
>> are more likely to expect the SET syntax.
>>
> 
> Yes, that seems logical to me, too. So i think we should go for it.
> Guillaume?
> 

I'm OK for this. I also think it's a better way, more logical to do it.
Should I provide a complete new patch with Bernd's and Tom's changes?


-- 
Guillaume.http://www.postgresqlfr.orghttp://dalibo.com


Re: Patch for ALTER DATABASE WITH TABLESPACE

From
Tom Lane
Date:
Guillaume Lelarge <guillaume@lelarge.info> writes:
> Should I provide a complete new patch with Bernd's and Tom's changes?

Please --- it's better if you integrate it since you know the patch
already.
        regards, tom lane


Re: Patch for ALTER DATABASE WITH TABLESPACE

From
Guillaume Lelarge
Date:
Tom Lane a écrit :
> Guillaume Lelarge <guillaume@lelarge.info> writes:
>> Should I provide a complete new patch with Bernd's and Tom's changes?
>
> Please --- it's better if you integrate it since you know the patch
> already.
>

I worked with Bernd's patch and replace the WITH syntax with the SET
one. It works AFAICS, but I'm not sure this is the best way to do it.
I'm no bison-guru.


--
Guillaume.
 http://www.postgresqlfr.org
 http://dalibo.com

Attachment

Re: Patch for ALTER DATABASE WITH TABLESPACE

From
Bernd Helmle
Date:
--On Mittwoch, November 05, 2008 01:10:22 +0100 Guillaume Lelarge
<guillaume@lelarge.info> wrote:

> Tom Lane a écrit :
>> Guillaume Lelarge <guillaume@lelarge.info> writes:
>>> Should I provide a complete new patch with Bernd's and Tom's changes?
>>
>> Please --- it's better if you integrate it since you know the patch
>> already.
>>
>
> I worked with Bernd's patch and replace the WITH syntax with the SET
> one. It works AFAICS, but I'm not sure this is the best way to do it.
> I'm no bison-guru.

I'm okay with this. However, i found some other issues:

* We really should error out when trying to copy into the same tablespace
the database already lives in. The code doesn't do any dangerous in this
case, but we should tell the DBA that he did something wrong and error out
if src_tblspcoid == dst_tblspoid is true. And i think we can avoid to call
database_file_update_needed() in this case then.

* The current implementation cannot merge a tablespace used by some
database objects already, for example:

CREATE DATABASE bernd;
CREATE DATABASE test;
CREATE TABLESPACE db1 LOCATION '/home/bernd/tmp/pgsql/temp1';
CREATE TABLESPACE db2 LOCATION '/home/bernd/tmp/pgsql/temp2';

\c test

CREATE TABLE foo(id INTEGER) TABLESPACE db2;

\c bernd
ALTER DATABASE test SET TABLESPACE db2;
ERROR:  could not create directory "pg_tblspc/16394/16392": Die Datei
existiert bereits

The last message tells the file already exists, which is true since this
tablespace is already in use by an object of database test. I think we have
two choices:

1) Make the error more informative. This would mean the DBA has to copy
tables, indexes etc. manually in this case.

2) Try to merge the database objects into the tablespace.

Opinions?

--  Thanks
                   Bernd


Re: Patch for ALTER DATABASE WITH TABLESPACE

From
Tom Lane
Date:
Bernd Helmle <mailings@oopsware.de> writes:
> * We really should error out when trying to copy into the same tablespace 
> the database already lives in.

No, I think that should just be a no-op.  We don't for instance throw
error when you ALTER OWNER to the existing owner.

> * The current implementation cannot merge a tablespace used by some 
> database objects already, for example:

Hmm --- there's more there than meets the eye.  To handle that case
correctly, you'd have to go into the DB's pg_class and change the
recorded tablespace for those objects to zero.  (Fail to do so, and
you've got a mess when you move the DB to yet another tablespace.)

I tend to agree that throwing an error is sufficient, as long as it's
a clear error message.
        regards, tom lane


Re: Patch for ALTER DATABASE WITH TABLESPACE

From
Bernd Helmle
Date:
--On Mittwoch, November 05, 2008 19:20:07 -0500 Tom Lane 
<tgl@sss.pgh.pa.us> wrote:

> No, I think that should just be a no-op.  We don't for instance throw
> error when you ALTER OWNER to the existing owner.
>

Hmm okay. When looking at this i was remembering the discussion we had for 
SET SCHEMA a long time ago, which indeed throws an error message:

ALTER TABLE foo SET SCHEMA public;
ERROR:  relation "foo" is already in schema "public"


--  Thanks
                   Bernd


Re: Patch for ALTER DATABASE WITH TABLESPACE

From
Guillaume Lelarge
Date:
Tom Lane a écrit :
> Bernd Helmle <mailings@oopsware.de> writes:
>> * We really should error out when trying to copy into the same tablespace
>> the database already lives in.
>
> No, I think that should just be a no-op.  We don't for instance throw
> error when you ALTER OWNER to the existing owner.
>

Moreover, ALTER TABLE SET TABLESPACE is silent when a user tries to move
an object to the tablespace it already belongs to.

>> * The current implementation cannot merge a tablespace used by some
>> database objects already, for example:
>
> Hmm --- there's more there than meets the eye.  To handle that case
> correctly, you'd have to go into the DB's pg_class and change the
> recorded tablespace for those objects to zero.  (Fail to do so, and
> you've got a mess when you move the DB to yet another tablespace.)
>
> I tend to agree that throwing an error is sufficient, as long as it's
> a clear error message.
>

OK. I added a code that checks the existence of the target tablespace
directory before executing copydir. If it found an empty directory, it
deletes it.

The error message looks like this:

postgres=# alter database test set tablespace db2;
ERROR:  some relations are already in the target tablespace "db2"
HINT:  You need to move them back to the default tablespace before using
this command.

Here is the complete test case:

postgres=# create database bernd;
CREATE DATABASE
postgres=# create database test;
CREATE DATABASE
postgres=# create tablespace db1 location
'/home/guillaume/postgresql_tblspc/db1';
CREATE TABLESPACE
postgres=# create tablespace db2 location
'/home/guillaume/postgresql_tblspc/db2';
CREATE TABLESPACE
postgres=# \c test
psql (8.4devel)
You are now connected to database "test".
test=# create table foo(id integer) tablespace db2;
CREATE TABLE
test=# \c bernd
psql (8.4devel)
You are now connected to database "bernd".
bernd=# alter database test set tablespace db2;
ERROR:  some relations are already in the target tablespace "db2"
HINT:  You need to move them back to the default tablespace before using
this command.
bernd=# \c test
psql (8.4devel)
You are now connected to database "test".
test=# alter table foo set tablespace pg_default;
ALTER TABLE
test=# \c bernd
psql (8.4devel)
You are now connected to database "bernd".
bernd=# alter database test set tablespace db2;
ALTER DATABASE

v4 patch attached.

Thanks.


--
Guillaume.
 http://www.postgresqlfr.org
 http://dalibo.com

Attachment

Re: Patch for ALTER DATABASE WITH TABLESPACE

From
Guillaume Lelarge
Date:
Guillaume Lelarge a écrit :
> v4 patch attached.
>

v5 patch attached.

Fixes two issues :

 * I forgot about Bernd's advice : "And i think we can avoid to call
   database_file_update_needed() in this case then." This is fixed.

 * I forgot to remove a debug ereport.

Sorry about this.


--
Guillaume.
 http://www.postgresqlfr.org
 http://dalibo.com

Attachment

Re: Patch for ALTER DATABASE WITH TABLESPACE

From
Bernd Helmle
Date:
--On Donnerstag, November 06, 2008 11:35:54 +0100 Guillaume Lelarge
<guillaume@lelarge.info> wrote:

> Guillaume Lelarge a écrit :
>> v4 patch attached.
>>
>
> v5 patch attached.
>

Thanks Guillaume.

Maybe this is nit-picking, but i see that you have to rmdir() an existing
empty tablespace directory to use copydir() afterwards. Maybe we can teach
copydir() to error out when trying to mkdir() an existing directory only
when forced by the caller? I see copydir() used at four places, so the
impact of this change would be minimal.

--  Thanks
                   Bernd


Re: Patch for ALTER DATABASE WITH TABLESPACE

From
Guillaume Lelarge
Date:
Bernd Helmle a écrit :
> --On Donnerstag, November 06, 2008 11:35:54 +0100 Guillaume Lelarge
> <guillaume@lelarge.info> wrote:
>
>> Guillaume Lelarge a écrit :
>>> v4 patch attached.
>>>
>>
>> v5 patch attached.
>>
>
> Thanks Guillaume.
>
> Maybe this is nit-picking, but i see that you have to rmdir() an
> existing empty tablespace directory to use copydir() afterwards. Maybe
> we can teach copydir() to error out when trying to mkdir() an existing
> directory only when forced by the caller? I see copydir() used at four
> places, so the impact of this change would be minimal.
>

I don't think this is nit-picking. I think about it too myself when I
did v4 and v5. I wasn't so sure, I prefered not to change this function
because it's used on important parts of the code (createdb and redo wal).

Anyway, I did what you asked. v6 is attached.

Thanks.


--
Guillaume.
 http://www.postgresqlfr.org
 http://dalibo.com

Attachment

Re: Patch for ALTER DATABASE WITH TABLESPACE

From
Tom Lane
Date:
Guillaume Lelarge <guillaume@lelarge.info> writes:
> Bernd Helmle a �crit :
>> Maybe this is nit-picking, but i see that you have to rmdir() an
>> existing empty tablespace directory to use copydir() afterwards. Maybe
>> we can teach copydir() to error out when trying to mkdir() an existing
>> directory only when forced by the caller? I see copydir() used at four
>> places, so the impact of this change would be minimal.

> I don't think this is nit-picking. I think about it too myself when I
> did v4 and v5. I wasn't so sure, I prefered not to change this function
> because it's used on important parts of the code (createdb and redo wal).

I like the v5 version better too.

Bernd, did you have any further comments?  If not I'll get started on
this patch.
        regards, tom lane


Re: Patch for ALTER DATABASE WITH TABLESPACE

From
Bernd Helmle
Date:
--On Freitag, November 07, 2008 09:36:32 -0500 Tom Lane <tgl@sss.pgh.pa.us> 
wrote:

> Bernd, did you have any further comments?  If not I'll get started on
> this patch.

No, i think this patch is ready for a committer then.

--  Thanks
                   Bernd


Re: Patch for ALTER DATABASE WITH TABLESPACE

From
Tom Lane
Date:
Guillaume Lelarge <guillaume@lelarge.info> writes:
> v5 patch attached.

Applied with corrections, mostly ensuring crash-safety and arranging
to clean up if the initial copy phase fails (think out-of-disk-space).
        regards, tom lane


Re: Patch for ALTER DATABASE WITH TABLESPACE

From
Guillaume Lelarge
Date:
Tom Lane a écrit :
> Guillaume Lelarge <guillaume@lelarge.info> writes:
>> v5 patch attached.
> 
> Applied with corrections, mostly ensuring crash-safety and arranging
> to clean up if the initial copy phase fails (think out-of-disk-space).
> 

Thanks for your tips and corrections. I'll go read the diff to better
understand them.

Bernd, thanks for your complete review.


-- 
Guillaume.http://www.postgresqlfr.orghttp://dalibo.com