Re: [HACKERS] Patch for ALTER DATABASE WITH TABLESPACE
Bernd Helmle a écrit : --On Donnerstag, November 06, 2008 11:35:54 +0100 Guillaume Lelarge [EMAIL PROTECTED] 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 alterdb_tablespace_v6.patch.bz2 Description: application/bzip -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch for ALTER DATABASE WITH TABLESPACE
Guillaume Lelarge [EMAIL PROTECTED] 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch for ALTER DATABASE WITH TABLESPACE
--On Freitag, November 07, 2008 09:36:32 -0500 Tom Lane [EMAIL PROTECTED] 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch for ALTER DATABASE WITH TABLESPACE
Guillaume Lelarge [EMAIL PROTECTED] 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch for ALTER DATABASE WITH TABLESPACE
Tom Lane a écrit : Guillaume Lelarge [EMAIL PROTECTED] 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.org http://dalibo.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch for ALTER DATABASE WITH TABLESPACE
Tom Lane a écrit : Bernd Helmle [EMAIL PROTECTED] 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 alterdb_tablespace_v4.patch.bz2 Description: application/bzip -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch for ALTER DATABASE WITH TABLESPACE
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 alterdb_tablespace_v5.patch.bz2 Description: application/bzip -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch for ALTER DATABASE WITH TABLESPACE
--On Donnerstag, November 06, 2008 11:35:54 +0100 Guillaume Lelarge [EMAIL PROTECTED] 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch for ALTER DATABASE WITH TABLESPACE
--On Mittwoch, November 05, 2008 01:10:22 +0100 Guillaume Lelarge [EMAIL PROTECTED] wrote: Tom Lane a écrit : Guillaume Lelarge [EMAIL PROTECTED] 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch for ALTER DATABASE WITH TABLESPACE
Bernd Helmle [EMAIL PROTECTED] 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch for ALTER DATABASE WITH TABLESPACE
--On Mittwoch, November 05, 2008 19:20:07 -0500 Tom Lane [EMAIL PROTECTED] 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch for ALTER DATABASE WITH TABLESPACE
--On Samstag, Oktober 25, 2008 23:50:47 +0200 Guillaume Lelarge [EMAIL PROTECTED] 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 alterdb_tablespace_v2.patch.bz2 Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch for ALTER DATABASE WITH TABLESPACE
Bernd Helmle a écrit : --On Samstag, Oktober 25, 2008 23:50:47 +0200 Guillaume Lelarge [EMAIL PROTECTED] 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.org http://dalibo.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch for ALTER DATABASE WITH TABLESPACE
Guillaume Lelarge [EMAIL PROTECTED] 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch for ALTER DATABASE WITH TABLESPACE
--On Dienstag, November 04, 2008 14:56:44 -0500 Tom Lane [EMAIL PROTECTED] 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch for ALTER DATABASE WITH TABLESPACE
Bernd Helmle a écrit : --On Dienstag, November 04, 2008 14:56:44 -0500 Tom Lane [EMAIL PROTECTED] 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.org http://dalibo.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch for ALTER DATABASE WITH TABLESPACE
Guillaume Lelarge [EMAIL PROTECTED] 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch for ALTER DATABASE WITH TABLESPACE
Tom Lane a écrit : Guillaume Lelarge [EMAIL PROTECTED] 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 alterdb_tablespace_v3.patch.bz2 Description: application/bzip -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
