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

On Mon, Apr 06, 2020 at 08:43:46PM +0300, Alexey Kondratov wrote:
> Thanks for the input, but I am afraid that the patch set became a bit messy
> now. I have eyeballed it and found some inconsistencies.
> 
>  	const char *name;			/* name of database to reindex */
> -	int			options;		/* Reindex options flags */
> +	List		*rawoptions;		/* Raw options */
> +	int		options;			/* Parsed options */
>  	bool		concurrent;		/* reindex concurrently? */
> 
> You introduced rawoptions in the 0002, but then removed it in 0003. So is it
> required or not? Probably this is a rebase artefact.

You're right; I first implemented REINDEX() and when I later did CLUSTER(), I
did it better, so I went back and did REINDEX() that way, but it looks like I
maybe fixup!ed the wrong commit.  Fixed now.

> +/* XXX: reusing reindex_option_list */
> +			| CLUSTER opt_verbose '(' reindex_option_list ')' qualified_name
> cluster_index_specification
> 
> Could we actually simply reuse vac_analyze_option_list? From the first sight
> it does just the right thing, excepting the special handling of spelling
> ANALYZE/ANALYSE, but it does not seem to be a problem.

Hm, do you mean to let cluster.c reject the other options like "analyze" ?
I'm not sure why that would be better than reusing reindex?
I think the suggestion will probably be to just copy+paste the reindex option
list and rename it to cluster (possibly with the explanation that they're
separate and independant and so their behavior shouldn't be tied together).

> > 0004 reduces duplicative error handling, as a separate commit so
> > Alexey can review it and/or integrate it.
> 
> ReindexRelationConcurrently is used for all cases, but it hits different
> code paths in the case of database, table and index. I have not checked yet,
> but are you sure it is safe removing these validations in the case of
> REINDEX CONCURRENTLY?

You're right about the pg_global case, fixed.  System catalogs can't be
reindexed CONCURRENTLY, so they're already caught by that check.

> > XXX: for cluster/vacuum, it might be more friendly to check before
> > clustering
> > the table, rather than after clustering and re-indexing.
> 
> Yes, I think it would be much more user-friendly.

I realized it's not needed or useful to check indexes in advance of clustering,
since 1) a mapped index will be on a mapped relation, which is already checked;
2) a system index will be on a system relation.  Right ?

-- we already knew that
ts=# SELECT COUNT(1) FROM pg_index i JOIN pg_class a ON i.indrelid=a.oid JOIN pg_class b ON i.indexrelid=b.oid WHERE a.relnamespace!=b.relnamespace;
count | 0

-- not true in general, but true here and true for system relations
ts=# SELECT COUNT(1) FROM pg_index i JOIN pg_class a ON i.indrelid=a.oid JOIN pg_class b ON i.indexrelid=b.oid WHERE a.reltablespace != b.reltablespace;
count | 0

-- 
Justin