Thread: Re: [COMMITTERS] pgsql: Add ALTER TABLESPACE ... MOVE command

Stephen Frost wrote:
> Add ALTER TABLESPACE ... MOVE command
> 
> This adds a 'MOVE' sub-command to ALTER TABLESPACE which allows moving sets of
> objects from one tablespace to another.  This can be extremely handy and avoids
> a lot of error-prone scripting.  ALTER TABLESPACE ... MOVE will only move
> objects the user owns, will notify the user if no objects were found, and can
> be used to move ALL objects or specific types of objects (TABLES, INDEXES, or
> MATERIALIZED VIEWS).

I just noticed that this commit added the new commands under the
"ALTER THING name RENAME TO" production (which were originally for
RenameStmt); since commit d86d51a95 had already added some
AlterTableSpaceOptionsStmt nodes to the possible results, maybe this
wasn't so bad in itself; but still it seems quite unlike the way we
organize our parse productions.

If we don't want to add new productions for these new node types, I
think at least this comment update is warranted:


diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 2867fa2..359bb8c 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -7009,6 +7009,8 @@ opt_force:    FORCE                                    {  $$ = TRUE;
}/****************************************************************************** * ALTER THING name RENAME TO newname
 
+ * ALTER TABLESPACE name MOVE blah
+ * ALTER TABLESPACE name SET/RESET blah *
*****************************************************************************/


Other thoughts?

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Stephen Frost wrote:
>> Add ALTER TABLESPACE ... MOVE command

> I just noticed that this commit added the new commands under the
> "ALTER THING name RENAME TO" production (which were originally for
> RenameStmt); since commit d86d51a95 had already added some
> AlterTableSpaceOptionsStmt nodes to the possible results, maybe this
> wasn't so bad in itself; but still it seems quite unlike the way we
> organize our parse productions.

Ick.  That's just plain sloppy.  Please create a separate production,
*and* a separate comment header.

Commit d86d51a95 was pretty damn awful in this regard as well, but
let's clean them both up, not make it worse.

Existing precedent would suggest inventing two new productions named the
same as the parse node types they produce, viz AlterTableSpaceMoveStmt
and AlterTableSpaceOptionsStmt.
        regards, tom lane

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Commit d86d51a95 was pretty damn awful in this regard as well, but
> let's clean them both up, not make it worse.

Fair enough.  I recall being a bit surprised at it also but didn't spend
much time thinking about it.  I'll clean it up.
Thanks,
    Stephen

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Ick.  That's just plain sloppy.  Please create a separate production,
> *and* a separate comment header.

Done.

> Commit d86d51a95 was pretty damn awful in this regard as well, but
> let's clean them both up, not make it worse.

Yeah, I think I noticed this in passing but probably did the same as
Robert and figured "oh, I'm just playing with this, I'll go back and
clean it up later.." and then promptly forgot about it 'cause it worked
just fine.

> Existing precedent would suggest inventing two new productions named the
> same as the parse node types they produce, viz AlterTableSpaceMoveStmt
> and AlterTableSpaceOptionsStmt.

This felt a bit like overkill to me for these few, so I just did them
under a single 'AlterTblSpcStmt'.  I'm not against going back and
changing it if others feel differently.
Thanks,
    Stephen