Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly

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