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