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
