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

On Wed, Sep 23, 2020 at 07:43:01PM +0300, Alexey Kondratov wrote:
> 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.

Thanks.  I rebased Alexey's latest patch on top of recent changes to cluster.c.
This puts the generic grammar changes first.  I wasn't paying much attention to
that part, so still waiting for a committer review.

-- 
Justin