On 2020-03-26 21:01, Justin Pryzby wrote:
@@ -262,7 +280,7 @@ cluster(ClusterStmt *stmt, bool isTopLevel)
* and error messages should refer to the operation as VACUUM not
CLUSTER.
*/
void
-cluster_rel(Oid tableOid, Oid indexOid, int options)
+cluster_rel(Oid tableOid, Oid indexOid, Oid tablespaceOid, int
options)
Add a comment here about the tablespaceOid parameter, like the other
functions
where it's added.
The permission checking is kind of duplicitive, so I'd suggest to
factor it
out. Ideally we'd only have one place that checks for
pg_global/system/mapped.
It needs to check that it's not a system relation, or that
system_table_mods
are allowed, and in any case that if it's a mapped rel, that it's not
being
moved. I would pass a boolean indicating if the tablespace is being
changed.
Yes, but I wanted to make sure first that all necessary validations are
there to do not miss something as I did last time. I do not like
repetitive code either, so I would like to introduce more common check
after reviewing the code as a whole.
Another issue is this:
+VACUUM ( FULL [, ...] ) [ TABLESPACE <replaceable
class="parameter">new_tablespace</replaceable> ] [ <replaceable
class="parameter">table_and_columns</replaceable> [, ...] ]
As you mentioned in your v1 patch, in the other cases, "tablespace
[tablespace]" is added at the end of the command rather than in the
middle. I
wasn't able to make that work, maybe because "tablespace" isn't a fully
reserved word (?). I didn't try with "SET TABLESPACE", although I
understand
it'd be better without "SET".
Initially I tried "SET TABLESPACE", but also failed to completely get
rid of shift/reduce conflicts. I will try to rewrite VACUUM's part again
with OptTableSpace. Maybe I will manage it this time.
I will take into account all your text edits as well.
Thanks
--
Alexey Kondratov
Postgres Professional https://www.postgrespro.com
Russian Postgres Company
