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

On Wed, Jan 20, 2021 at 03:34:39PM -0300, Alvaro Herrera wrote:
> On 2021-Jan-20, Alexey Kondratov wrote:
>> Ugh, forgot to attach the patches. Here they are.
> 
> Yeah, looks reasonable.

Patch 0002 still has a whole set of issues as I pointed out a couple
of hours ago, but if we agree on 0001 as being useful if done
independently, I'd rather get that done first.  This way or just
merging both things in a single commit is not a big deal seeing the
amount of code, so I am fine with any approach.  It may be possible
that 0001 requires more changes depending on the work to-be-done for
0002 though?

>> +	/* No work if no change in tablespace. */
>> +	oldTablespaceOid = rd_rel->reltablespace;
>> +	if (tablespaceOid != oldTablespaceOid ||
>> +		(tablespaceOid == MyDatabaseTableSpace && OidIsValid(oldTablespaceOid)))
>> +	{
>> +		/* Update the pg_class row. */
>> +		rd_rel->reltablespace = (tablespaceOid == MyDatabaseTableSpace) ?
>> +			InvalidOid : tablespaceOid;
>> +		CatalogTupleUpdate(pg_class, &tuple->t_self, tuple);
>> +
>> +		changed = true;
>> +	}
>> +
>> +	if (changed)
>> +		/* Record dependency on tablespace */
>> +		changeDependencyOnTablespace(RelationRelationId,
>> +									 reloid, rd_rel->reltablespace);
> 
> Why have a separate "if (changed)" block here instead of merging with
> the above?

Yep.

+       if (SetRelTablespace(reloid, newTableSpace))
+               /* Make sure the reltablespace change is visible */
+               CommandCounterIncrement();
At quick glance, I am wondering why you just don't do a CCI within
SetRelTablespace().
--
Michael