MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Date: Wed, 23 Dec 2020 19:12:11 +0300
From: Alexey Kondratov <a.kondratov@postgrespro.ru>
To: Justin Pryzby <pryzby@telsasoft.com>
Cc: Zhihong Yu <zyu@yugabyte.com>, Michael Paquier <michael@paquier.xyz>,
 Alvaro Herrera <alvherre@alvh.no-ip.org>, Peter Eisentraut
 <peter.eisentraut@enterprisedb.com>, 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
In-Reply-To: <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>
 <20201223052200.GP30237@telsasoft.com>
User-Agent: Roundcube Webmail/1.4.0
Message-ID: <f64e9fb8e3aa323d135db789322dbae3@postgrespro.ru>
Precedence: bulk

On 2020-12-23 08:22, Justin Pryzby wrote:
> 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.

Yes, I have not noticed that we would have already assigned 
tablespaceOid to InvalidOid in this case. Back to the v7 we were doing 
this assignment a bit later, so this could make sense, but now it seems 
to be redundant. For some reason I have mixed these refactorings 
separated by a dozen of versions...


Thanks
-- 
Alexey Kondratov

Postgres Professional https://www.postgrespro.com
Russian Postgres Company