Re: [HACKERS] ALTER TABLESPACE MOVE command tag tweak
Alvaro, * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: Thanks. One more tweak --- the whole reason for fiddling with this is to ensure that event triggers support this operation. Therefore this node should be handled by ProcessUtilitySlow, not standard_ProcessUtility, as in the attached patch. [...] I propose this for 9.4 too. Done. I bet they have! Have fun, Thanks! I'm trying to. :) Stephen signature.asc Description: Digital signature
Re: [HACKERS] ALTER TABLESPACE MOVE command tag tweak
Stephen Frost wrote: Alvaro, * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: ALTER TABLE ALL IN TABLESPACE xyz which AFAICS should work since ALL is already a reserved keyword. Pushed to master and REL9_4_STABLE. Thanks. One more tweak --- the whole reason for fiddling with this is to ensure that event triggers support this operation. Therefore this node should be handled by ProcessUtilitySlow, not standard_ProcessUtility, as in the attached patch. (I checked the documentation for necessary updates; turns out that the table in the event triggers chapter says that ddl_command_end etc support the command ALTER TABLE, and since this is the tag returned by the new ALTER TABLE ALL IN TABLESPACE command, there is no update needed. In fact, one can argue that the table is wrong currently because it doesn't say that ALTER TABLE ALL IN TABLESPACE is not supported.) I propose this for 9.4 too. Apologies on it taking so long- things have a bit interesting for me over the past month or two. :) I bet they have! Have fun, -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c index 40ac47f..e2c2d3d 100644 --- a/src/backend/tcop/utility.c +++ b/src/backend/tcop/utility.c @@ -507,10 +507,6 @@ standard_ProcessUtility(Node *parsetree, AlterTableSpaceOptions((AlterTableSpaceOptionsStmt *) parsetree); break; - case T_AlterTableMoveAllStmt: - AlterTableMoveAll((AlterTableMoveAllStmt *) parsetree); - break; - case T_TruncateStmt: ExecuteTruncate((TruncateStmt *) parsetree); break; @@ -1296,6 +1292,10 @@ ProcessUtilitySlow(Node *parsetree, AlterTSConfiguration((AlterTSConfigurationStmt *) parsetree); break; + case T_AlterTableMoveAllStmt: +AlterTableMoveAll((AlterTableMoveAllStmt *) parsetree); +break; + case T_DropStmt: ExecDropStmt((DropStmt *) parsetree, isTopLevel); break; -- 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] ALTER TABLESPACE MOVE command tag tweak
Alvaro, * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: Stephen Frost wrote: * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: ALTER TABLE ALL IN TABLESPACE xyz which AFAICS should work since ALL is already a reserved keyword. Pushed to master and REL9_4_STABLE. Thanks. One more tweak --- the whole reason for fiddling with this is to ensure that event triggers support this operation. Therefore this node should be handled by ProcessUtilitySlow, not standard_ProcessUtility, as in the attached patch. Ah, right, of course. I recall looking for what else might need to be changed and apparently missed that distinction in the call sites. (I checked the documentation for necessary updates; turns out that the table in the event triggers chapter says that ddl_command_end etc support the command ALTER TABLE, and since this is the tag returned by the new ALTER TABLE ALL IN TABLESPACE command, there is no update needed. In fact, one can argue that the table is wrong currently because it doesn't say that ALTER TABLE ALL IN TABLESPACE is not supported.) Heh, yes, true. I propose this for 9.4 too. Agreed. Looks pretty straight-forward, will update soon. Apologies on it taking so long- things have a bit interesting for me over the past month or two. :) I bet they have! Have fun, Thanks! :) Stephen signature.asc Description: Digital signature
Re: [HACKERS] ALTER TABLESPACE MOVE command tag tweak
Fujii, * Fujii Masao (masao.fu...@gmail.com) wrote: On Fri, Aug 22, 2014 at 8:14 AM, Stephen Frost sfr...@snowman.net wrote: * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: ALTER TABLE ALL IN TABLESPACE xyz which AFAICS should work since ALL is already a reserved keyword. Pushed to master and REL9_4_STABLE. You seem to have forgotten to update tab-complete.c. Attached patch updates that. Pushed. Thanks again, Stephen signature.asc Description: Digital signature
Re: [HACKERS] ALTER TABLESPACE MOVE command tag tweak
On Fri, Aug 22, 2014 at 8:14 AM, Stephen Frost sfr...@snowman.net wrote:
Alvaro,
* Alvaro Herrera (alvhe...@2ndquadrant.com) wrote:
ALTER TABLE ALL IN TABLESPACE xyz
which AFAICS should work since ALL is already a reserved keyword.
Pushed to master and REL9_4_STABLE.
You seem to have forgotten to update tab-complete.c.
Attached patch updates that.
Regards,
--
Fujii Masao
*** a/src/bin/psql/tab-complete.c
--- b/src/bin/psql/tab-complete.c
***
*** 953,958 psql_completion(const char *text, int start, int end)
--- 953,965
/* ALTER */
+ /* ALTER TABLE */
+ else if (pg_strcasecmp(prev2_wd, ALTER) == 0
+ pg_strcasecmp(prev_wd, TABLE) == 0)
+ {
+ COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_tables,
+ UNION SELECT 'ALL IN TABLESPACE');
+ }
/*
* complete with what you can alter (TABLE, GROUP, USER, ...) unless we're
* in ALTER TABLE sth ALTER
***
*** 970,975 psql_completion(const char *text, int start, int end)
--- 977,1001
COMPLETE_WITH_LIST(list_ALTER);
}
+ /* ALTER TABLE,INDEX,MATERIALIZED VIEW xxx ALL IN TABLESPACE xxx */
+ else if (pg_strcasecmp(prev4_wd, ALL) == 0
+ pg_strcasecmp(prev3_wd, IN) == 0
+ pg_strcasecmp(prev2_wd, TABLESPACE) == 0)
+ {
+ static const char *const list_ALTERALLINTSPC[] =
+ {SET TABLESPACE, OWNED BY, NULL};
+
+ COMPLETE_WITH_LIST(list_ALTERALLINTSPC);
+ }
+ /* ALTER TABLE,INDEX,MATERIALIZED VIEW xxx ALL IN TABLESPACE xxx OWNED BY */
+ else if (pg_strcasecmp(prev6_wd, ALL) == 0
+ pg_strcasecmp(prev5_wd, IN) == 0
+ pg_strcasecmp(prev4_wd, TABLESPACE) == 0
+ pg_strcasecmp(prev2_wd, OWNED) == 0
+ pg_strcasecmp(prev4_wd, BY) == 0)
+ {
+ COMPLETE_WITH_QUERY(Query_for_list_of_roles);
+ }
/* ALTER AGGREGATE,FUNCTION name */
else if (pg_strcasecmp(prev3_wd, ALTER) == 0
(pg_strcasecmp(prev2_wd, AGGREGATE) == 0 ||
***
*** 1106, psql_completion(const char *text, int start, int end)
--- 1132,1144
COMPLETE_WITH_LIST(list_ALTER_FOREIGN_TABLE);
}
+ /* ALTER INDEX */
+ else if (pg_strcasecmp(prev2_wd, ALTER) == 0
+ pg_strcasecmp(prev_wd, INDEX) == 0)
+ {
+ COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_indexes,
+ UNION SELECT 'ALL IN TABLESPACE');
+ }
/* ALTER INDEX name */
else if (pg_strcasecmp(prev3_wd, ALTER) == 0
pg_strcasecmp(prev2_wd, INDEX) == 0)
***
*** 1169,1175 psql_completion(const char *text, int start, int end)
pg_strcasecmp(prev2_wd, MATERIALIZED) == 0
pg_strcasecmp(prev_wd, VIEW) == 0)
{
! COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_matviews, NULL);
}
/* ALTER USER,ROLE name */
--- 1202,1209
pg_strcasecmp(prev2_wd, MATERIALIZED) == 0
pg_strcasecmp(prev_wd, VIEW) == 0)
{
! COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_matviews,
! UNION SELECT 'ALL IN TABLESPACE');
}
/* ALTER USER,ROLE name */
***
*** 1742,1753 psql_completion(const char *text, int start, int end)
COMPLETE_WITH_CONST(IDENTITY);
}
! /* ALTER TABLESPACE foo with RENAME TO, OWNER TO, SET, RESET, MOVE */
else if (pg_strcasecmp(prev3_wd, ALTER) == 0
pg_strcasecmp(prev2_wd, TABLESPACE) == 0)
{
static const char *const list_ALTERTSPC[] =
! {RENAME TO, OWNER TO, SET, RESET, MOVE, NULL};
COMPLETE_WITH_LIST(list_ALTERTSPC);
}
--- 1776,1787
COMPLETE_WITH_CONST(IDENTITY);
}
! /* ALTER TABLESPACE foo with RENAME TO, OWNER TO, SET, RESET */
else if (pg_strcasecmp(prev3_wd, ALTER) == 0
pg_strcasecmp(prev2_wd, TABLESPACE) == 0)
{
static const char *const list_ALTERTSPC[] =
! {RENAME TO, OWNER TO, SET, RESET, NULL};
COMPLETE_WITH_LIST(list_ALTERTSPC);
}
***
*** 1769,1795 psql_completion(const char *text, int start, int end)
COMPLETE_WITH_LIST(list_TABLESPACEOPTIONS);
}
- /* ALTER TABLESPACE foo MOVE ALL|TABLES|INDEXES|MATERIALIZED VIEWS */
- else if (pg_strcasecmp(prev4_wd, ALTER) == 0
- pg_strcasecmp(prev3_wd, TABLESPACE) == 0
- pg_strcasecmp(prev_wd, MOVE) == 0)
- {
- static const char *const list_TABLESPACEMOVETARGETS[] =
- {ALL, TABLES, INDEXES, MATERIALIZED VIEWS, NULL};
-
- COMPLETE_WITH_LIST(list_TABLESPACEMOVETARGETS);
- }
- else if ((pg_strcasecmp(prev4_wd, TABLESPACE) == 0
- pg_strcasecmp(prev2_wd, MOVE) == 0) ||
- (pg_strcasecmp(prev5_wd, TABLESPACE) == 0
- pg_strcasecmp(prev3_wd, MOVE) == 0
- pg_strcasecmp(prev2_wd, MATERIALIZED) == 0))
- {
- static const char *const list_TABLESPACEMOVEOPTIONS[] =
- {OWNED BY, TO, NULL};
-
- COMPLETE_WITH_LIST(list_TABLESPACEMOVEOPTIONS);
- }
/* ALTER TEXT SEARCH */
else if (pg_strcasecmp(prev3_wd, ALTER) == 0
--- 1803,1808
***
*** 2791,2799 psql_completion(const char *text, int start, int end)
* but we may as well tab-complete both: perhaps some users prefer one
Re: [HACKERS] ALTER TABLESPACE MOVE command tag tweak
Fujii, * Fujii Masao (masao.fu...@gmail.com) wrote: You seem to have forgotten to update tab-complete.c. Good point, absolutely correct. I have a bad tendency to do that. Attached patch updates that. Many thanks, will review and apply soon. Thanks again! Stephen signature.asc Description: Digital signature
Re: [HACKERS] ALTER TABLESPACE MOVE command tag tweak
Alvaro, * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: ALTER TABLE ALL IN TABLESPACE xyz which AFAICS should work since ALL is already a reserved keyword. Pushed to master and REL9_4_STABLE. Apologies on it taking so long- things have a bit interesting for me over the past month or two. :) Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] ALTER TABLESPACE MOVE command tag tweak
* Peter Eisentraut (pete...@gmx.net) wrote:
On 8/17/14 5:19 PM, Stephen Frost wrote:
Alvaro, all,
* Stephen Frost (sfr...@snowman.net) wrote:
As mentioned, I'll add this to the ALTER TABLE documentation and remove
it from the TABLESPACE docs. That's not done yet but I should have time
in the next few days to get that done also and will then commit it all
to master and back-patch to 9.4, barring objections.
Here's a first pass with the documentation changes included. Feedback
welcome, and I'll review it again and plan to commit it on Tuesday.
One thing that is not clear from this (before or after) is whether all
{tables|indexes|etc} in the tablespace actually means in this
tablespace and in the current database. Presumably it does.
Yes, only those in the current database. There was a mention of that in
the original docs. I'll add it to the new ones.
Thanks,
Stephen
signature.asc
Description: Digital signature
Re: [HACKERS] ALTER TABLESPACE MOVE command tag tweak
Alvaro, all,
* Alvaro Herrera (alvhe...@2ndquadrant.com) wrote:
Can you be more specific on the exact grammar you're considering? The
proposal above,
ALTER TABLE ON ALL TABLES IN TABLESPACE xyz
doesn't seem very good to me. I would think it'd be more like
ALTER ALL TABLES IN TABLESPACE xyz
but then if you return ALTER TABLE as a command tag that might be a bit
strange. Maybe
ALTER TABLE ALL IN TABLESPACE xyz
which AFAICS should work since ALL is already a reserved keyword.
I've implemented the 'ALTER TABLE ALL IN TABLESPACE ...' appproach, as
discussed. Patch attached.
Also, how would we document this? Would we have it in the same page as
all the ALTER TABLE variants, or would we create a separate page for
ALTER TABLE ALL? Keeping in mind that in the future we might want to
allow things such as ALTER TABLE ALL IN SCHEMA xyz it might be better to
have the selection logic documented neatly in its own little page
instead of together with the ALTER TABLE mess which is already rather
large.
As mentioned, I'll add this to the ALTER TABLE documentation and remove
it from the TABLESPACE docs. That's not done yet but I should have time
in the next few days to get that done also and will then commit it all
to master and back-patch to 9.4, barring objections.
Thanks,
Stephen
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index c86b999..feceed7 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -50,6 +50,7 @@
#include commands/tablespace.h
#include commands/trigger.h
#include commands/typecmds.h
+#include commands/user.h
#include executor/executor.h
#include foreign/foreign.h
#include miscadmin.h
@@ -9204,6 +9205,176 @@ ATExecSetTableSpace(Oid tableOid, Oid newTableSpace, LOCKMODE lockmode)
}
/*
+ * Alter Table ALL ... SET TABLESPACE
+ *
+ * Allows a user to move all objects of some type in a given tablespace in the
+ * current database to another tablespace. Objects can be chosen based on the
+ * owner of the object also, to allow users to move only their objects.
+ * The user must have CREATE rights on the new tablespace, as usual. The main
+ * permissions handling is done by the lower-level table move function.
+ *
+ * All to-be-moved objects are locked first. If NOWAIT is specified and the
+ * lock can't be acquired then we ereport(ERROR).
+ */
+Oid
+AlterTableMoveAll(AlterTableMoveAllStmt *stmt)
+{
+ List *relations = NIL;
+ ListCell *l;
+ ScanKeyData key[1];
+ Relation rel;
+ HeapScanDesc scan;
+ HeapTuple tuple;
+ Oid orig_tablespaceoid;
+ Oid new_tablespaceoid;
+ List *role_oids = roleNamesToIds(stmt-roles);
+
+ /* Ensure we were not asked to move something we can't */
+ if (stmt-objtype != OBJECT_TABLE stmt-objtype != OBJECT_INDEX
+ stmt-objtype != OBJECT_MATVIEW)
+ ereport(ERROR,
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg(only tables, indexes, and materialized views exist in tablespaces)));
+
+ /* Get the orig and new tablespace OIDs */
+ orig_tablespaceoid = get_tablespace_oid(stmt-orig_tablespacename, false);
+ new_tablespaceoid = get_tablespace_oid(stmt-new_tablespacename, false);
+
+ /* Can't move shared relations in to or out of pg_global */
+ /* This is also checked by ATExecSetTableSpace, but nice to stop earlier */
+ if (orig_tablespaceoid == GLOBALTABLESPACE_OID ||
+ new_tablespaceoid == GLOBALTABLESPACE_OID)
+ ereport(ERROR,
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg(cannot move relations in to or out of pg_global tablespace)));
+
+ /*
+ * Must have CREATE rights on the new tablespace, unless it is the
+ * database default tablespace (which all users implicitly have CREATE
+ * rights on).
+ */
+ if (OidIsValid(new_tablespaceoid) new_tablespaceoid != MyDatabaseTableSpace)
+ {
+ AclResult aclresult;
+
+ aclresult = pg_tablespace_aclcheck(new_tablespaceoid, GetUserId(),
+ ACL_CREATE);
+ if (aclresult != ACLCHECK_OK)
+ aclcheck_error(aclresult, ACL_KIND_TABLESPACE,
+ get_tablespace_name(new_tablespaceoid));
+ }
+
+ /*
+ * Now that the checks are done, check if we should set either to
+ * InvalidOid because it is our database's default tablespace.
+ */
+ if (orig_tablespaceoid == MyDatabaseTableSpace)
+ orig_tablespaceoid = InvalidOid;
+
+ if (new_tablespaceoid == MyDatabaseTableSpace)
+ new_tablespaceoid = InvalidOid;
+
+ /* no-op */
+ if (orig_tablespaceoid == new_tablespaceoid)
+ return new_tablespaceoid;
+
+ /*
+ * Walk the list of objects in the tablespace and move them. This will
+ * only find objects in our database, of course.
+ */
+ ScanKeyInit(key[0],
+Anum_pg_class_reltablespace,
+BTEqualStrategyNumber, F_OIDEQ,
+ObjectIdGetDatum(orig_tablespaceoid));
+
+ rel = heap_open(RelationRelationId, AccessShareLock);
+ scan = heap_beginscan_catalog(rel, 1, key);
+ while ((tuple = heap_getnext(scan, ForwardScanDirection)) != NULL)
+ {
+ Oid relOid =
Re: [HACKERS] ALTER TABLESPACE MOVE command tag tweak
Alvaro, all, * Stephen Frost (sfr...@snowman.net) wrote: As mentioned, I'll add this to the ALTER TABLE documentation and remove it from the TABLESPACE docs. That's not done yet but I should have time in the next few days to get that done also and will then commit it all to master and back-patch to 9.4, barring objections. Here's a first pass with the documentation changes included. Feedback welcome, and I'll review it again and plan to commit it on Tuesday. Thanks, Stephen diff --git a/doc/src/sgml/ref/alter_index.sgml b/doc/src/sgml/ref/alter_index.sgml index 94a7af0..85159a0 100644 --- a/doc/src/sgml/ref/alter_index.sgml +++ b/doc/src/sgml/ref/alter_index.sgml @@ -25,6 +25,8 @@ ALTER INDEX [ IF EXISTS ] replaceable class=PARAMETERname/replaceable RENA ALTER INDEX [ IF EXISTS ] replaceable class=PARAMETERname/replaceable SET TABLESPACE replaceable class=PARAMETERtablespace_name/replaceable ALTER INDEX [ IF EXISTS ] replaceable class=PARAMETERname/replaceable SET ( replaceable class=PARAMETERstorage_parameter/replaceable = replaceable class=PARAMETERvalue/replaceable [, ... ] ) ALTER INDEX [ IF EXISTS ] replaceable class=PARAMETERname/replaceable RESET ( replaceable class=PARAMETERstorage_parameter/replaceable [, ... ] ) +ALTER INDEX ALL IN TABLESPACE replaceable class=PARAMETERname/replaceable [ OWNED BY replaceable class=PARAMETERrole_name/replaceable [, ... ] ] +SET TABLESPACE replaceable class=PARAMETERnew_tablespace/replaceable [ NOWAIT ] /synopsis /refsynopsisdiv @@ -63,6 +65,17 @@ ALTER INDEX [ IF EXISTS ] replaceable class=PARAMETERname/replaceable RESE para This form changes the index's tablespace to the specified tablespace and moves the data file(s) associated with the index to the new tablespace. + To change the tablespace of an index, you must own the index and have + literalCREATE/literal privilege on the new tablespace. + All indexes in a tablespace can be moved by using the + literalALL IN TABLESPACE/literal form, which will lock all indexes + to be moved and then move each one. This form also supports + literalOWNED BY/literal, which will only move indexes owned by the + roles specified. If the literalNOWAIT/literal option is specified + then the command will fail if it is unable to acquire all of the locks + required immediately. Note that system catalogs will not be moved by + this command, use commandALTER DATABASE/command or explicit + commandALTER INDEX/command invocations instead if desired. See also xref linkend=SQL-CREATETABLESPACE. /para diff --git a/doc/src/sgml/ref/alter_materialized_view.sgml b/doc/src/sgml/ref/alter_materialized_view.sgml index 1932eeb..b0759fc 100644 --- a/doc/src/sgml/ref/alter_materialized_view.sgml +++ b/doc/src/sgml/ref/alter_materialized_view.sgml @@ -29,6 +29,8 @@ ALTER MATERIALIZED VIEW [ IF EXISTS ] replaceable class=parametername/repla RENAME TO replaceable class=parameternew_name/replaceable ALTER MATERIALIZED VIEW [ IF EXISTS ] replaceable class=parametername/replaceable SET SCHEMA replaceable class=parameternew_schema/replaceable +ALTER MATERIALIZED VIEW ALL IN TABLESPACE replaceable class=parametername/replaceable [ OWNED BY replaceable class=PARAMETERrole_name/replaceable [, ... ] ] +SET TABLESPACE replaceable class=PARAMETERnew_tablespace/replaceable [ NOWAIT ] phrasewhere replaceable class=PARAMETERaction/replaceable is one of:/phrase diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml index 69a1e14..83d230c 100644 --- a/doc/src/sgml/ref/alter_table.sgml +++ b/doc/src/sgml/ref/alter_table.sgml @@ -31,6 +31,8 @@ ALTER TABLE [ IF EXISTS ] replaceable class=PARAMETERname/replaceable RENAME TO replaceable class=PARAMETERnew_name/replaceable ALTER TABLE [ IF EXISTS ] replaceable class=PARAMETERname/replaceable SET SCHEMA replaceable class=PARAMETERnew_schema/replaceable +ALTER TABLE ALL IN TABLESPACE replaceable class=PARAMETERname/replaceable [ OWNED BY replaceable class=PARAMETERrole_name/replaceable [, ... ] ] +SET TABLESPACE replaceable class=PARAMETERnew_tablespace/replaceable [ NOWAIT ] phrasewhere replaceable class=PARAMETERaction/replaceable is one of:/phrase @@ -597,6 +599,17 @@ ALTER TABLE [ IF EXISTS ] replaceable class=PARAMETERname/replaceable moves the data file(s) associated with the table to the new tablespace. Indexes on the table, if any, are not moved; but they can be moved separately with additional literalSET TABLESPACE/literal commands. + All tables in a tablespace can be moved by using the + literalALL IN TABLESPACE/literal form, which will lock all tables + to be moved first and then move each one. This form also supports + literalOWNED BY/literal, which will only move tables owned by the + roles specified. If the literalNOWAIT/literal option is specified +
Re: [HACKERS] ALTER TABLESPACE MOVE command tag tweak
On 15 Aug 2014 19:06, Robert Haas robertmh...@gmail.com wrote: As for the expanded-mode changes, I thought there was consensus to revert that from 9.4. Me too. In fact, I think that's been the consensus for many months, but unless I'm mistaken it ain't done. Yeah, this is entirely my fault. I was traveling, busy, not feeling well, in various permutations but I should have gotten to it earlier. I'll take care of it tomorrow morning.
Re: [HACKERS] ALTER TABLESPACE MOVE command tag tweak
On 8/17/14 5:19 PM, Stephen Frost wrote:
Alvaro, all,
* Stephen Frost (sfr...@snowman.net) wrote:
As mentioned, I'll add this to the ALTER TABLE documentation and remove
it from the TABLESPACE docs. That's not done yet but I should have time
in the next few days to get that done also and will then commit it all
to master and back-patch to 9.4, barring objections.
Here's a first pass with the documentation changes included. Feedback
welcome, and I'll review it again and plan to commit it on Tuesday.
One thing that is not clear from this (before or after) is whether all
{tables|indexes|etc} in the tablespace actually means in this
tablespace and in the current database. Presumably it does.
--
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] ALTER TABLESPACE MOVE command tag tweak
On Thu, Aug 14, 2014 at 6:12 PM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: We really should not be making changes of this type less than a month from our ostensible release date. That is not enough time for us to notice if the changes turn out to be not as good as we think they are. If it weren't for the fact that we'll be wedded forevermore to a bad choice of syntax, I might agree with you. But at this point, the alternatives we have are to fix it now, or fix it never. I don't like #2. Or postpone the release for another month or two. There's still a few other unresolved issues, too, like the problems with psql and expanded mode; and the JSONB toast problems. The latter is relatively new, but we don't have a proposed patch for it yet unless there's on in an email I haven't read yet, and the former has been lingering for many months without getting appreciably closer to a resolution. I like releasing in September as much as anyone, but that contemplates people taking care to get known issues fixed before the second half of August. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- 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] ALTER TABLESPACE MOVE command tag tweak
Robert Haas robertmh...@gmail.com writes: On Thu, Aug 14, 2014 at 6:12 PM, Tom Lane t...@sss.pgh.pa.us wrote: If it weren't for the fact that we'll be wedded forevermore to a bad choice of syntax, I might agree with you. But at this point, the alternatives we have are to fix it now, or fix it never. I don't like #2. Or postpone the release for another month or two. There's still a few other unresolved issues, too, like the problems with psql and expanded mode; and the JSONB toast problems. The latter is relatively new, but we don't have a proposed patch for it yet unless there's on in an email I haven't read yet, and the former has been lingering for many months without getting appreciably closer to a resolution. Yeah, if we do anything about JSONB we are certainly going to need another beta release, which means final couldn't be before end of Sept. at the very earliest. Still, it's always been project policy that we ship when it's ready, not when the calendar hits some particular point. As for the expanded-mode changes, I thought there was consensus to revert that from 9.4. 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] ALTER TABLESPACE MOVE command tag tweak
On Fri, Aug 15, 2014 at 9:57 AM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: On Thu, Aug 14, 2014 at 6:12 PM, Tom Lane t...@sss.pgh.pa.us wrote: If it weren't for the fact that we'll be wedded forevermore to a bad choice of syntax, I might agree with you. But at this point, the alternatives we have are to fix it now, or fix it never. I don't like #2. Or postpone the release for another month or two. There's still a few other unresolved issues, too, like the problems with psql and expanded mode; and the JSONB toast problems. The latter is relatively new, but we don't have a proposed patch for it yet unless there's on in an email I haven't read yet, and the former has been lingering for many months without getting appreciably closer to a resolution. Yeah, if we do anything about JSONB we are certainly going to need another beta release, which means final couldn't be before end of Sept. at the very earliest. Still, it's always been project policy that we ship when it's ready, not when the calendar hits some particular point. Absolutely. As for the expanded-mode changes, I thought there was consensus to revert that from 9.4. Me too. In fact, I think that's been the consensus for many months, but unless I'm mistaken it ain't done. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- 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] ALTER TABLESPACE MOVE command tag tweak
On Wed, Aug 13, 2014 at 9:33 AM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Stephen Frost wrote: * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: Stephen Frost wrote: Alright, sounds like this is more-or-less the concensus. I'll see about making it happen shortly. Were you able to work on this? Apologies, I've been gone more-or-less all of July. I'm back now and have time to spend on this. Ping? Seriously! We really should not be making changes of this type less than a month from our ostensible release date. That is not enough time for us to notice if the changes turn out to be not as good as we think they are. The whole point of beta is to fix things while there's still enough time for further course correction if needed; if we say, oh, beta's not totally over yet, I don't have to get my changes in, then it completely defeats the purpose of having a beta in the first place. /rant -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- 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] ALTER TABLESPACE MOVE command tag tweak
Robert Haas robertmh...@gmail.com writes: We really should not be making changes of this type less than a month from our ostensible release date. That is not enough time for us to notice if the changes turn out to be not as good as we think they are. If it weren't for the fact that we'll be wedded forevermore to a bad choice of syntax, I might agree with you. But at this point, the alternatives we have are to fix it now, or fix it never. I don't like #2. 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] ALTER TABLESPACE MOVE command tag tweak
* Tom Lane (t...@sss.pgh.pa.us) wrote: Robert Haas robertmh...@gmail.com writes: We really should not be making changes of this type less than a month from our ostensible release date. That is not enough time for us to notice if the changes turn out to be not as good as we think they are. If it weren't for the fact that we'll be wedded forevermore to a bad choice of syntax, I might agree with you. But at this point, the alternatives we have are to fix it now, or fix it never. I don't like #2. I'm planning to fix it shortly, as I mentioned to Alvaro on IRC when I saw his note. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] ALTER TABLESPACE MOVE command tag tweak
Stephen Frost wrote: * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: Stephen Frost wrote: Alright, sounds like this is more-or-less the concensus. I'll see about making it happen shortly. Were you able to work on this? Apologies, I've been gone more-or-less all of July. I'm back now and have time to spend on this. Ping? -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- 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] ALTER TABLESPACE MOVE command tag tweak
Alvaro, * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: Stephen Frost wrote: Of course, we handle this in 'GRANT' with 'GRANT ON ALL TABLES', so why not 'ALTER TABLE ON ALL TABLES IN TABLESPACE blah'? that does get pretty darn verbose but is at least a bit more in-line with what we have done before.. That's not a bad line of thought --- I doubt that verbosity is critical here. Alright, sounds like this is more-or-less the concensus. I'll see about making it happen shortly. Were you able to work on this? Apologies, I've been gone more-or-less all of July. I'm back now and have time to spend on this. Can you be more specific on the exact grammar you're considering? The proposal above, ALTER TABLE ON ALL TABLES IN TABLESPACE xyz doesn't seem very good to me. I would think it'd be more like ALTER ALL TABLES IN TABLESPACE xyz but then if you return ALTER TABLE as a command tag that might be a bit strange. Maybe ALTER TABLE ALL IN TABLESPACE xyz which AFAICS should work since ALL is already a reserved keyword. Interesting idea. I wonder if we might apply that to other capabilities of ALTER TABLE too.. That is, make it a generic way to specify that all tables in the tablespace (or schema..?) should be modified in a certain way. We'd have to consider which of the ALTER TABLE operations this would work for, of course. I don't believe it'd make sense for any of the 'ADD/DROP COLUMN' or similar commands, but 'OWNER TO' would make sense... I'm not sure how much we want to work this over during beta though.. My original thought was just to adjust the grammar and hopefully minimize the other changes in this area, but it'd be good to have an idea about where we want to take this, so the grammar can support the future capabilities while not actually adding them at this time. Also, how would we document this? Would we have it in the same page as all the ALTER TABLE variants, or would we create a separate page for ALTER TABLE ALL? Keeping in mind that in the future we might want to allow things such as ALTER TABLE ALL IN SCHEMA xyz it might be better to have the selection logic documented neatly in its own little page instead of together with the ALTER TABLE mess which is already rather large. I would think we'd simply use the ALTER TABLE page as I don't think the documentation of this would be all that difficult to describe in a reasonable paragraph. A page dedicated to it feels like overkill, but perhaps that might change as we add more. Other thoughts on this? We should really get any of the changes we're doing here done soon. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] ALTER TABLESPACE MOVE command tag tweak
Stephen Frost wrote: * Tom Lane (t...@sss.pgh.pa.us) wrote: Stephen Frost sfr...@snowman.net writes: That it's more-or-less a bulk 'ALTER TABLE' operation is why I had been trying to think of a way to put it under that command. What if we had a more general way to reference 'all objects in a tablespace'? tablespace.* or ALL:TABLESAPCE? Are there other places which might benefit from being able to take and operate on all objects in a tablespace? Of course, we handle this in 'GRANT' with 'GRANT ON ALL TABLES', so why not 'ALTER TABLE ON ALL TABLES IN TABLESPACE blah'? that does get pretty darn verbose but is at least a bit more in-line with what we have done before.. That's not a bad line of thought --- I doubt that verbosity is critical here. Alright, sounds like this is more-or-less the concensus. I'll see about making it happen shortly. Stephen, Were you able to work on this? Can you be more specific on the exact grammar you're considering? The proposal above, ALTER TABLE ON ALL TABLES IN TABLESPACE xyz doesn't seem very good to me. I would think it'd be more like ALTER ALL TABLES IN TABLESPACE xyz but then if you return ALTER TABLE as a command tag that might be a bit strange. Maybe ALTER TABLE ALL IN TABLESPACE xyz which AFAICS should work since ALL is already a reserved keyword. Also, how would we document this? Would we have it in the same page as all the ALTER TABLE variants, or would we create a separate page for ALTER TABLE ALL? Keeping in mind that in the future we might want to allow things such as ALTER TABLE ALL IN SCHEMA xyz it might be better to have the selection logic documented neatly in its own little page instead of together with the ALTER TABLE mess which is already rather large. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- 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] ALTER TABLESPACE MOVE command tag tweak
* Tom Lane (t...@sss.pgh.pa.us) wrote: Stephen Frost sfr...@snowman.net writes: That it's more-or-less a bulk 'ALTER TABLE' operation is why I had been trying to think of a way to put it under that command. What if we had a more general way to reference 'all objects in a tablespace'? tablespace.* or ALL:TABLESAPCE? Are there other places which might benefit from being able to take and operate on all objects in a tablespace? Of course, we handle this in 'GRANT' with 'GRANT ON ALL TABLES', so why not 'ALTER TABLE ON ALL TABLES IN TABLESPACE blah'? that does get pretty darn verbose but is at least a bit more in-line with what we have done before.. That's not a bad line of thought --- I doubt that verbosity is critical here. Alright, sounds like this is more-or-less the concensus. I'll see about making it happen shortly. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] ALTER TABLESPACE MOVE command tag tweak
All, * Stephen Frost (sfr...@snowman.net) wrote: * Tom Lane (t...@sss.pgh.pa.us) wrote: Alvaro Herrera alvhe...@2ndquadrant.com writes: ALTER TABLESPACE MOVE is a glorified ALTER TABLE. If ALTER TABLESPACE MOVE returned ALTER TABLE as a tag, I think it'd work well too; but not ALTER TABLESPACE. Individually, since the implementation works by calling AlterTableInternal(), it already works. Now if you state that the current design in event_triggers that works by slicing CommandTag and comparing the pieces is broken, I don't disagree and I think I have now (in the patch posted in a nearby thread) some more infrastructure to do it differently. But even if we do that, I think we're going to need a way to differentiate ALTER TABLESPACE MOVE from other forms of ALTER TABLESPACE. I haven't given this much thought, though. Yeah, I'd not paid much attention to it either. Now that I look at it, ALTER TABLESPACE MOVE seems like a pretty unfortunate choice of naming all around, because (unless I'm misunderstanding) it doesn't actually alter any property of the tablespace itself. It might be a bit late to propose this, but I wonder if some syntax along the lines of I'm not against changing it- doing operations on a whole tablespace felt like it would make sense under 'ALTER TABLESPACE' to me (hence the implementation) but you're right, it's not actually changing properties of the tablespaces themselves. MOVE ALL [ TABLES | INDEXES | ... ] IN TABLESPACE foo TO bar I'm not a huge fan of new top-level constructs and re-using MOVE feels completely wrong to me as that's used for cursors.. wouldn't be less confusing. Not sure what we'd use as command tag for it though (not MOVE, since that's taken). I would have thought something under ALTER TABLE would make more sense, if we're going to change it, eg: ALTER TABLE ALL [ TABLES | INDEXES | ... ] IN TABLESPACE SET TABLESPACE ... or perhaps something like ALTER TABLES IN TABLESPACE ... Any further thoughts on this? I haven't tried to go implement anything yet but I'm definitely concerned that we may run into a keyword issue with ALTER TABLE, but I don't really want to add 'TABLES' either. Anyone have any other suggestions or ideas? Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] ALTER TABLESPACE MOVE command tag tweak
Stephen Frost wrote: Any further thoughts on this? I haven't tried to go implement anything yet but I'm definitely concerned that we may run into a keyword issue with ALTER TABLE, but I don't really want to add 'TABLES' either. Anyone have any other suggestions or ideas? I thought that the suggestion to use MOVE was the most appropriate. However I also concur that re-using MOVE from its current cursor-related meaning would be troublesome from several PoVs, so if there's a synonym of MOVE that we can use, that'd probably be better. I don't like ALTER TABLES myself; and having an ALTER TABLE command that affects several tables seems counterintuitive as well. How about RELOCATE ALL [ TABLES | INDEXES | ... ] IN TABLESPACE foo TO bar ? -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- 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] ALTER TABLESPACE MOVE command tag tweak
* Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: Stephen Frost wrote: Any further thoughts on this? I haven't tried to go implement anything yet but I'm definitely concerned that we may run into a keyword issue with ALTER TABLE, but I don't really want to add 'TABLES' either. Anyone have any other suggestions or ideas? I thought that the suggestion to use MOVE was the most appropriate. However I also concur that re-using MOVE from its current cursor-related meaning would be troublesome from several PoVs, so if there's a synonym of MOVE that we can use, that'd probably be better. I don't like ALTER TABLES myself; and having an ALTER TABLE command that affects several tables seems counterintuitive as well. How about RELOCATE ALL [ TABLES | INDEXES | ... ] IN TABLESPACE foo TO bar ? The original call for this change was to have it return the command tag of ALTER TABLE, wasn't it? Would inventing a new command tag like RELOCATE actually improve the situation..? Or is the thought that it'd still be ALTER TABLE? That it's more-or-less a bulk 'ALTER TABLE' operation is why I had been trying to think of a way to put it under that command. What if we had a more general way to reference 'all objects in a tablespace'? tablespace.* or ALL:TABLESAPCE? Are there other places which might benefit from being able to take and operate on all objects in a tablespace? Of course, we handle this in 'GRANT' with 'GRANT ON ALL TABLES', so why not 'ALTER TABLE ON ALL TABLES IN TABLESPACE blah'? that does get pretty darn verbose but is at least a bit more in-line with what we have done before.. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] ALTER TABLESPACE MOVE command tag tweak
Stephen Frost sfr...@snowman.net writes: That it's more-or-less a bulk 'ALTER TABLE' operation is why I had been trying to think of a way to put it under that command. What if we had a more general way to reference 'all objects in a tablespace'? tablespace.* or ALL:TABLESAPCE? Are there other places which might benefit from being able to take and operate on all objects in a tablespace? Of course, we handle this in 'GRANT' with 'GRANT ON ALL TABLES', so why not 'ALTER TABLE ON ALL TABLES IN TABLESPACE blah'? that does get pretty darn verbose but is at least a bit more in-line with what we have done before.. That's not a bad line of thought --- I doubt that verbosity is critical here. 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] ALTER TABLESPACE MOVE command tag tweak
Tom Lane wrote: Stephen Frost sfr...@snowman.net writes: That it's more-or-less a bulk 'ALTER TABLE' operation is why I had been trying to think of a way to put it under that command. What if we had a more general way to reference 'all objects in a tablespace'? tablespace.* or ALL:TABLESAPCE? Are there other places which might benefit from being able to take and operate on all objects in a tablespace? Of course, we handle this in 'GRANT' with 'GRANT ON ALL TABLES', so why not 'ALTER TABLE ON ALL TABLES IN TABLESPACE blah'? that does get pretty darn verbose but is at least a bit more in-line with what we have done before.. That's not a bad line of thought --- I doubt that verbosity is critical here. Yep, that works for my purposes. Also, it seems there should be no parse conflict because ON is already a reserved keyword .. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- 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] ALTER TABLESPACE MOVE command tag tweak
On Mon, Jun 23, 2014 at 05:10:02PM -0400, Stephen Frost wrote: * Stephen Frost (sfr...@snowman.net) wrote: * Tom Lane (t...@sss.pgh.pa.us) wrote: I'm not against changing it- doing operations on a whole tablespace felt like it would make sense under 'ALTER TABLESPACE' to me (hence the implementation) but you're right, it's not actually changing properties of the tablespaces themselves. MOVE ALL [ TABLES | INDEXES | ... ] IN TABLESPACE foo TO bar I'm not a huge fan of new top-level constructs and re-using MOVE feels completely wrong to me as that's used for cursors.. wouldn't be less confusing. Not sure what we'd use as command tag for it though (not MOVE, since that's taken). I would have thought something under ALTER TABLE would make more sense, if we're going to change it, eg: ALTER TABLE ALL [ TABLES | INDEXES | ... ] IN TABLESPACE SET TABLESPACE ... or perhaps something like ALTER TABLES IN TABLESPACE ... Any further thoughts on this? I haven't tried to go implement anything yet but I'm definitely concerned that we may run into a keyword issue with ALTER TABLE, but I don't really want to add 'TABLES' either. Anyone have any other suggestions or ideas? I recommend: SELECT tablespace_move(old_tablespace name, new_tablespace name); SELECT tablespace_move(old_tablespace name, new_tablespace name, relkind char); Concerning the problem that started this thread, I would raise one ALTER TABLE event per table and not fire an event for the bulk request as a whole. -- Noah Misch EnterpriseDB http://www.enterprisedb.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] ALTER TABLESPACE MOVE command tag tweak
Noah Misch wrote: Concerning the problem that started this thread, I would raise one ALTER TABLE event per table and not fire an event for the bulk request as a whole. Yes, that's how it already works. Essentially, ALTER TABLESPACE MOVE calls AlterTableInternal, and that function calls one ATExecFoo() for each table to be moved. What my code does is set up AT event collection when ALTER TABLESPACE MOVE is called, and then for each ATExecFoo() is executed, one event is added to a queue. When ALTER TABLESPACE MOVE finishes, all those events are reported together. This part is not a problem. The only part that is a problem is that the CommandTag comparison in event_trigger.c wasn't happy that a command was being passed with the ALTER TABLESPACE command tag, because tablespaces are not supported by that module --- and I think the unhappiness was only in an assert-only block (but I'm not 100% sure about that last bit, because I didn't try removing that test.) -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- 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] ALTER TABLESPACE MOVE command tag tweak
* Tom Lane (t...@sss.pgh.pa.us) wrote: Alvaro Herrera alvhe...@2ndquadrant.com writes: ALTER TABLESPACE MOVE is a glorified ALTER TABLE. If ALTER TABLESPACE MOVE returned ALTER TABLE as a tag, I think it'd work well too; but not ALTER TABLESPACE. Individually, since the implementation works by calling AlterTableInternal(), it already works. Now if you state that the current design in event_triggers that works by slicing CommandTag and comparing the pieces is broken, I don't disagree and I think I have now (in the patch posted in a nearby thread) some more infrastructure to do it differently. But even if we do that, I think we're going to need a way to differentiate ALTER TABLESPACE MOVE from other forms of ALTER TABLESPACE. I haven't given this much thought, though. Yeah, I'd not paid much attention to it either. Now that I look at it, ALTER TABLESPACE MOVE seems like a pretty unfortunate choice of naming all around, because (unless I'm misunderstanding) it doesn't actually alter any property of the tablespace itself. It might be a bit late to propose this, but I wonder if some syntax along the lines of I'm not against changing it- doing operations on a whole tablespace felt like it would make sense under 'ALTER TABLESPACE' to me (hence the implementation) but you're right, it's not actually changing properties of the tablespaces themselves. MOVE ALL [ TABLES | INDEXES | ... ] IN TABLESPACE foo TO bar I'm not a huge fan of new top-level constructs and re-using MOVE feels completely wrong to me as that's used for cursors.. wouldn't be less confusing. Not sure what we'd use as command tag for it though (not MOVE, since that's taken). I would have thought something under ALTER TABLE would make more sense, if we're going to change it, eg: ALTER TABLE ALL [ TABLES | INDEXES | ... ] IN TABLESPACE SET TABLESPACE ... or perhaps something like ALTER TABLES IN TABLESPACE ... Thanks, Stephen signature.asc Description: Digital signature
[HACKERS] ALTER TABLESPACE MOVE command tag tweak
The ALTER TABLESPACE MOVE command affects tables, not tablespaces; and as such, I think event triggers should support that command. I'm not proposing to change event triggers at this stage, but since IMO we will want to do that in 9.5, we need it to have a different command tag than plain ALTER TABLESPACE. This is so that check_ddl_tag() can compare the tag with ALTER TABLESPACE and say unsupported, and ALTER TABLESPACE MOVE and say supported. Both are currently spelled the same, which will be a problem. Therefore I propose the attached patch for 9.4. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c index 3423898..5553221 100644 --- a/src/backend/tcop/utility.c +++ b/src/backend/tcop/utility.c @@ -1806,7 +1805,7 @@ CreateCommandTag(Node *parsetree) break; case T_AlterTableSpaceMoveStmt: - tag = ALTER TABLESPACE; + tag = ALTER TABLESPACE MOVE; break; case T_CreateExtensionStmt: -- 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] ALTER TABLESPACE MOVE command tag tweak
Alvaro Herrera alvhe...@2ndquadrant.com writes: The ALTER TABLESPACE MOVE command affects tables, not tablespaces; and as such, I think event triggers should support that command. I'm not proposing to change event triggers at this stage, but since IMO we will want to do that in 9.5, we need it to have a different command tag than plain ALTER TABLESPACE. This is so that check_ddl_tag() can compare the tag with ALTER TABLESPACE and say unsupported, and ALTER TABLESPACE MOVE and say supported. Both are currently spelled the same, which will be a problem. Therefore I propose the attached patch for 9.4. Hm. While the specific change here seems harmless enough, the argument for it seems to me to indicate that the very design is broken. Do you expect event triggers to distinguish all the different subflavors of ALTER TABLE, for example, on the basis of the command tag? Backwards compatibility is going to prevent us from refining the tag strings that much. So ISTM this concern means what we'd better be thinking about is some other way for event triggers to find out what they're dealing with. A different thought is that what event triggers would probably like is for this command to be reported to them as a series of ALTER TABLE SET TABLESPACE events, one per moved table. If it's done like that then the tag for the outer ALTER TABLESPACE may not be so important. 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] ALTER TABLESPACE MOVE command tag tweak
Tom Lane wrote: Alvaro Herrera alvhe...@2ndquadrant.com writes: The ALTER TABLESPACE MOVE command affects tables, not tablespaces; and as such, I think event triggers should support that command. I'm not proposing to change event triggers at this stage, but since IMO we will want to do that in 9.5, we need it to have a different command tag than plain ALTER TABLESPACE. This is so that check_ddl_tag() can compare the tag with ALTER TABLESPACE and say unsupported, and ALTER TABLESPACE MOVE and say supported. Both are currently spelled the same, which will be a problem. Therefore I propose the attached patch for 9.4. Hm. While the specific change here seems harmless enough, the argument for it seems to me to indicate that the very design is broken. Do you expect event triggers to distinguish all the different subflavors of ALTER TABLE, for example, on the basis of the command tag? Backwards compatibility is going to prevent us from refining the tag strings that much. Actually, I don't -- I have already implemented ALTER TABLE for event triggers, and there wasn't any need to tweak the command tags there. The problem in this particular case is that TABLESPACE is a global object, thus not supported; but ALTER TABLESPACE MOVE is a command that modifies tables (which *are* supported), not tablespaces. ALTER TABLESPACE MOVE is a glorified ALTER TABLE. If ALTER TABLESPACE MOVE returned ALTER TABLE as a tag, I think it'd work well too; but not ALTER TABLESPACE. Individually, since the implementation works by calling AlterTableInternal(), it already works. Now if you state that the current design in event_triggers that works by slicing CommandTag and comparing the pieces is broken, I don't disagree and I think I have now (in the patch posted in a nearby thread) some more infrastructure to do it differently. But even if we do that, I think we're going to need a way to differentiate ALTER TABLESPACE MOVE from other forms of ALTER TABLESPACE. I haven't given this much thought, though. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- 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] ALTER TABLESPACE MOVE command tag tweak
Alvaro Herrera alvhe...@2ndquadrant.com writes: The problem in this particular case is that TABLESPACE is a global object, thus not supported; but ALTER TABLESPACE MOVE is a command that modifies tables (which *are* supported), not tablespaces. ALTER TABLESPACE MOVE is a glorified ALTER TABLE. If ALTER TABLESPACE MOVE returned ALTER TABLE as a tag, I think it'd work well too; but not ALTER TABLESPACE. Individually, since the implementation works by calling AlterTableInternal(), it already works. Now if you state that the current design in event_triggers that works by slicing CommandTag and comparing the pieces is broken, I don't disagree and I think I have now (in the patch posted in a nearby thread) some more infrastructure to do it differently. But even if we do that, I think we're going to need a way to differentiate ALTER TABLESPACE MOVE from other forms of ALTER TABLESPACE. I haven't given this much thought, though. Yeah, I'd not paid much attention to it either. Now that I look at it, ALTER TABLESPACE MOVE seems like a pretty unfortunate choice of naming all around, because (unless I'm misunderstanding) it doesn't actually alter any property of the tablespace itself. It might be a bit late to propose this, but I wonder if some syntax along the lines of MOVE ALL [ TABLES | INDEXES | ... ] IN TABLESPACE foo TO bar wouldn't be less confusing. Not sure what we'd use as command tag for it though (not MOVE, since that's taken). 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
