Date: Tue, 22 Dec 2020 23:22:00 -0600
From: Justin Pryzby <pryzby@telsasoft.com>
To: Zhihong Yu <zyu@yugabyte.com>
Cc: Michael Paquier <michael@paquier.xyz>,
	Alvaro Herrera <alvherre@alvh.no-ip.org>,
	Peter Eisentraut <peter.eisentraut@enterprisedb.com>,
	Alexey Kondratov <a.kondratov@postgrespro.ru>,
	Masahiko Sawada <masahiko.sawada@2ndquadrant.com>,
	Steve Singer <steve@ssinger.info>,
	pgsql-hackers@lists.postgresql.org,
	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: <20201223052200.GP30237@telsasoft.com>
References: <7ec67c56-2377-cd05-51a0-691104404abe@enterprisedb.com>
 <20201216004517.GA18498@alvherre.pgsql>
 <X9lcV/nYVU5WYf7D@paquier.xyz>
 <X+GWnexHZdt/aaVl@paquier.xyz>
 <20201222083204.GL30237@telsasoft.com>
 <X+HDFVXTYbTJMSXZ@paquier.xyz>
 <20201222211537.GO30237@telsasoft.com>
 <CALNJ-vQeFpTdTgHdU4fcO94pRcxpAC7X5ujeM06rUHoKwgO5Sg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <CALNJ-vQeFpTdTgHdU4fcO94pRcxpAC7X5ujeM06rUHoKwgO5Sg@mail.gmail.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk

On Tue, Dec 22, 2020 at 03:22:19PM -0800, Zhihong Yu wrote:
> Justin:
> For reindex_index() :
> 
> +   if (options->tablespaceOid == MyDatabaseTableSpace)
> +       options->tablespaceOid = InvalidOid;
> ...
> +   oldTablespaceOid = iRel->rd_rel->reltablespace;
> +   if (set_tablespace &&
> +       (options->tablespaceOid != oldTablespaceOid ||
> +       (options->tablespaceOid == MyDatabaseTableSpace &&
> OidIsValid(oldTablespaceOid))))
> 
> I wonder why the options->tablespaceOid == MyDatabaseTableSpace clause
> appears again in the second if statement.
> Since the first if statement would assign InvalidOid
> to options->tablespaceOid when the first if condition is satisfied.

Good question.  Alexey mentioned on Sept 23 that he added the first stanza.  to
avoid storing the DB's tablespace OID (rather than InvalidOid).

I think the 2nd half of the "or" is unnecessary since that was added setting to
options->tablespaceOid = InvalidOid.
If requesting to move to the DB's default tablespace, it'll now hit the first
part of the OR:

> +       (options->tablespaceOid != oldTablespaceOid ||

Without the first stanza setting, it would've hit the 2nd condition:

> +       (options->tablespaceOid == MyDatabaseTableSpace && OidIsValid(oldTablespaceOid))))

which means: "user requested to move a table to the DB's default tblspace, and
it was previously on a nondefault space".

So I think we can drop the 2nd half of the OR.  Thanks for noticing.

-- 
Justin