Thread: Patch for ALTER DATABASE WITH TABLESPACE
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
--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
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
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
--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
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
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
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
--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
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
--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
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
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
--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
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
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
--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
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
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