Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly

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