Re: Patch for ALTER DATABASE WITH TABLESPACE
Patch for ALTER DATABASE WITH TABLESPACE
От:
Guillaume Lelarge <guillaume@lelarge.info>
Дата:
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
Re: Patch for ALTER DATABASE WITH TABLESPACE
От:
Guillaume Lelarge <guillaume@lelarge.info>
Дата:
Tom Lane a écrit : > Guillaume Lelarge 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
Re: Patch for ALTER DATABASE WITH TABLESPACE
От:
Guillaume Lelarge <guillaume@lelarge.info>
Дата:
Tom Lane a écrit : > Bernd Helmle 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
Re: Patch for ALTER DATABASE WITH TABLESPACE
От:
Guillaume Lelarge <guillaume@lelarge.info>
Дата:
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
Re: Patch for ALTER DATABASE WITH TABLESPACE
От:
Guillaume Lelarge <guillaume@lelarge.info>
Дата:
Bernd Helmle a écrit : > --On Donnerstag, November 06, 2008 11:35:54 +0100 Guillaume Lelarge > 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
Re: Patch for ALTER DATABASE WITH TABLESPACE
От:
Bernd Helmle <mailings@oopsware.de>
Дата:
--On Samstag, Oktober 25, 2008 23:50:47 +0200 Guillaume Lelarge
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