Date: Thu, 26 Mar 2020 13:01:12 -0500
From: Justin Pryzby <pryzby@telsasoft.com>
To: Alexey Kondratov <a.kondratov@postgrespro.ru>
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
Message-ID: <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>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <39a055a7a8cd19552f35f4cb98a8cd80@postgrespro.ru>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk

> I included your new solution regarding this part from 0004 into 0001. It
> seems that at least a tip of the problem was in that we tried to change
> tablespace to pg_default being already there.

Right, causing it to try to drop that filenode twice.

> +++ b/doc/src/sgml/ref/cluster.sgml
> +      The name of a specific tablespace to store clustered relations.

Could you phrase these like you did in the comments:
" the name of the tablespace where the clustered relation is to be rebuilt."

> +++ b/doc/src/sgml/ref/reindex.sgml
> +      The name of a specific tablespace to store rebuilt indexes.

" The name of a tablespace where indexes will be rebuilt"

> +++ b/doc/src/sgml/ref/vacuum.sgml
> +      The name of a specific tablespace to write a new copy of the table.

> +      This specifies a tablespace, where all rebuilt indexes will be created.

say "specifies the tablespace where", with no comma.

> +			else if (!OidIsValid(classtuple->relfilenode))
> +			{
> +				/*
> +				 * Skip all mapped relations.
> +				 * relfilenode == 0 checks after that, similarly to
> +				 * RelationIsMapped().

I would say "OidIsValid(relfilenode) checks for that, ..."

> @@ -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.

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".

-- 
Justin