MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Date: Thu, 26 Mar 2020 22:22:08 +0300
From: Alexey Kondratov <a.kondratov@postgrespro.ru>
To: Justin Pryzby <pryzby@telsasoft.com>
Cc: Michael Paquier <michael@paquier.xyz>, Masahiko Sawada
 <masahiko.sawada@2ndquadrant.com>, Steve Singer <steve@ssinger.info>,
 pgsql-hackers@lists.postgresql.org, Alvaro Herrera
 <alvherre@2ndquadrant.com>, Robert Haas <robertmhaas@gmail.com>, Alexander
 Korotkov <a.korotkov@postgrespro.ru>, Masahiko Sawada
 <sawada.mshk@gmail.com>, Jose Luis Tallon <jltallon@adv-solutions.net>
Subject: Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on
 the fly
In-Reply-To: <20200326180112.GH17431@telsasoft.com>
References: <3ae48673-283c-3e99-3dd8-36ebb81614b5@postgrespro.ru>
 <20191202082134.GI1696@paquier.xyz>
 <c8ed43871e171ca2077aa9e1d0ed970a@postgrespro.ru>
 <20200211164848.GO1412@telsasoft.com>
 <eb4cdddc0d6197f3fef15d36758c93fe@postgrespro.ru>
 <20200229145304.GI29456@telsasoft.com>
 <20200309200447.GA32459@telsasoft.com>
 <cb874a90-3946-91e4-4c31-5075f425b280@postgrespro.ru>
 <20200325234027.GH21443@telsasoft.com>
 <39a055a7a8cd19552f35f4cb98a8cd80@postgrespro.ru>
 <20200326180112.GH17431@telsasoft.com>
User-Agent: Roundcube Webmail/1.4.0
Message-ID: <39c239a12c4dde0a3c78e2ab5d8eb55b@postgrespro.ru>
Precedence: bulk

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