Re: Patch for ALTER DATABASE WITH TABLESPACE

--On Mittwoch, November 05, 2008 01:10:22 +0100 Guillaume Lelarge
<guillaume(at)lelarge(dot)info> wrote:

> Tom Lane a écrit :
>> Guillaume Lelarge <guillaume(at)lelarge(dot)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(at)oopsware(dot)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(at)sss(dot)pgh(dot)pa(dot)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(at)oopsware(dot)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

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

Guillaume Lelarge <guillaume(at)lelarge(dot)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(at)lelarge(dot)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.org
http://dalibo.com