Date: Tue, 1 Sep 2020 05:12:19 -0500
From: Justin Pryzby <pryzby@telsasoft.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: Alexey Kondratov <a.kondratov@postgrespro.ru>,
	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: <20200901101219.GB5450@telsasoft.com>
References: <a0fd6af4bdf3cd209a69cea112ab10ea@postgrespro.ru>
 <20200406184406.GF2228@telsasoft.com>
 <9b6a6cec134451550b74aa2ffd1446d0@postgrespro.ru>
 <20200407204406.GR2228@telsasoft.com>
 <20200412013352.GA2491@telsasoft.com>
 <20200426175614.GT28974@telsasoft.com>
 <20200809110252.GD17986@paquier.xyz>
 <20200810022443.GI20473@telsasoft.com>
 <20200811053945.GG17986@paquier.xyz>
 <20200811070922.GP20473@telsasoft.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200811070922.GP20473@telsasoft.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk

This patch seems to be missing a call to RelationAssumeNewRelfilenode() in
reindex_index().

That's maybe the related to the cause of the crashes I pointed out earlier this
year.

Alexey's v4 patch changed RelationSetNewRelfilenode() to accept a tablespace
parameter, but Michael seemed to object to that.  However that seems cleaner
and ~30 line shorter.

Michael, would you comment on that ?  The v4 patch and your comments are here.
https://www.postgresql.org/message-id/attachment/105574/v4-0001-Allow-REINDEX-and-REINDEX-CONCURRENTLY-to-change-tablespace.patch
https://www.postgresql.org/message-id/20191127035416.GG5435%40paquier.xyz

> --- a/src/backend/catalog/index.c
> +++ b/src/backend/catalog/index.c
> @@ -3480,6 +3518,47 @@ reindex_index(Oid indexId, bool skip_constraint_checks, char persistence,
>  	 */
>  	CheckTableNotInUse(iRel, "REINDEX INDEX");
>  
> +	if (tablespaceOid == MyDatabaseTableSpace)
> +		tablespaceOid = InvalidOid;
> +
> +	/*
> +	 * Set the new tablespace for the relation.  Do that only in the
> +	 * case where the reindex caller wishes to enforce a new tablespace.
> +	 */
> +	if (set_tablespace &&
> +		tablespaceOid != iRel->rd_rel->reltablespace)
> +	{
> +		Relation		pg_class;
> +		Form_pg_class	rd_rel;
> +		HeapTuple		tuple;
> +
> +		/* First get a modifiable copy of the relation's pg_class row */
> +		pg_class = table_open(RelationRelationId, RowExclusiveLock);
> +
> +		tuple = SearchSysCacheCopy1(RELOID, ObjectIdGetDatum(indexId));
> +		if (!HeapTupleIsValid(tuple))
> +			elog(ERROR, "cache lookup failed for relation %u", indexId);
> +		rd_rel = (Form_pg_class) GETSTRUCT(tuple);
> +
> +		/*
> +		 * Mark the relation as ready to be dropped at transaction commit,
> +		 * before making visible the new tablespace change so as this won't
> +		 * miss things.
> +		 */
> +		RelationDropStorage(iRel);
> +
> +		/* Update the pg_class row */
> +		rd_rel->reltablespace = tablespaceOid;
> +		CatalogTupleUpdate(pg_class, &tuple->t_self, tuple);
> +
> +		heap_freetuple(tuple);
> +
> +		table_close(pg_class, RowExclusiveLock);
> +
> +		/* Make sure the reltablespace change is visible */
> +		CommandCounterIncrement();