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

On 2020-09-09 18:36, Justin Pryzby wrote:
> Rebased on a6642b3ae: Add support for partitioned tables and indexes in 
> REINDEX
> 
> So now this includes the new functionality and test for reindexing a
> partitioned table onto a new tablespace.  That part could use some 
> additional
> review.
> 

I have finally had a look on your changes regarding partitioned tables.

+set_rel_tablespace(Oid indexid, char *tablespace)
+{
+	Oid tablespaceOid = tablespace ? get_tablespace_oid(tablespace, false) 
:
+		InvalidOid;

You pass a tablespace name to set_rel_tablespace(), but it is already 
parsed into the Oid before. So I do not see why we need this extra work 
here instead of just passing Oid directly.

Also set_rel_tablespace() does not check for a no-op case, i.e. if 
requested tablespace is the same as before.

+	/*
+	 * 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)

Just noticed that this check is not completely correct as well, since it 
does not check for MyDatabaseTableSpace (stored as InvalidOid) logic.

I put these small fixes directly into the attached 0003.

Also, I thought about your comment above set_rel_tablespace() and did a 
bit 'extreme' refactoring, which is attached as a separated patch 0004. 
The only one doubtful change IMO is reordering of RelationDropStorage() 
operation inside reindex_index(). However, it only schedules unlinking 
of physical storage at transaction commit, so this refactoring seems to 
be safe.

If there will be no objections I would merge it with 0003.

On 2020-09-09 16:03, Alexey Kondratov wrote:
> On 2020-09-09 15:22, Michael Paquier wrote:
>> 
>> By the way, skimming through the patch set, I was wondering if we
>> could do the refactoring of patch 0005 as a first step
>> 
> 
> Yes, I did it with intention to put as a first patch, but wanted to
> get some feedback. It's easier to refactor the last patch without
> rebasing others.
> 
>> 
>> until I
>> noticed this part:
>> +common_option_name:
>>         NonReservedWord { $$ = $1; }
>> 	| analyze_keyword { $$ = "analyze"; }
>> This is not a good idea as you make ANALYZE an option available for
>> all the commands involved in the refactoring.  A portion of that could
>> be considered though, like the use of common_option_arg.
>> 
> 
> From the grammar perspective ANY option is available for any command
> that uses parenthesized option list. All the checks and validations
> are performed at the corresponding command code.
> This analyze_keyword is actually doing only an ANALYZE word
> normalization if it's used as an option. Why it could be harmful?
> 

Michael has not replied since then, but he was relatively positive about 
0005 initially, so I put it as a first patch now.


Regards
-- 
Alexey Kondratov

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