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

--On Samstag, Oktober 25, 2008 23:50:47 +0200 Guillaume Lelarge
<guillaume(at)lelarge(dot)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

Bernd Helmle a écrit :
> --On Samstag, Oktober 25, 2008 23:50:47 +0200 Guillaume Lelarge
> <guillaume(at)lelarge(dot)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.org
http://dalibo.com

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

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.

regards, tom lane

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.

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