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