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

On 2020-09-02 07:56, Justin Pryzby wrote:
>
> Done in the attached, which is also rebased on 1d6541666.
>
> And added RelationAssumeNewRelfilenode() as I mentioned - but I'm
> hoping to
> hear from Michael about any reason not to call
> RelationSetNewRelfilenode()
> instead of directly calling the things it would itself call.

The latest patch set immediately got a conflict with Michael's fixup
01767533, so I've rebased it first of all.

+ Prints a progress report as each table is clustered.
+<!-- When specified within parenthesis, <literal>VERBOSE</literal> may
be followed by a boolean ...-->

I think that we can remove this comment completely as it is already
explained in the docs later.

+ | CLUSTER opt_verbose '(' vac_analyze_option_list ')' qualified_name
cluster_index_specification
+ {

What's the point in allowing a mixture of old options with new
parenthesized option list? VACUUM doesn't do so. I can understand it for
REINDEX CONCURRENTLY, since parenthesized options were already there,
but not for CLUSTER.

With v23 it is possible to write:

CLUSTER VERBOSE (VERBOSE) table USING ...

which is untidy. Furthermore, 'CLUSTER VERBOSE (' is tab-completed to
'CLUSTER VERBOSE (USING'. That way I propose to only allow either new
options or old similarly to the VACUUM. See attached 0002.

- COMPLETE_WITH("VERBOSE");
+ COMPLETE_WITH("TABLESPACE|VERBOSE");

Tab completion in the CLUSTER was broken for parenthesized options, so
I've fixed it in the 0005.

Also, I noticed that you used vac_analyze_option_list instead of
reindex_option_list and I checked other option lists in the grammar.
I've found that explain_option_list and vac_analyze_option_list are
identical, so it makes sense to leave just one of them and rename it to,
e.g., common_option_list in order to use it everywhere needed (REINDEX,
VACUUM, EXPLAIN, CLUSTER, ANALYZE). The whole grammar is already
complicated enough to keep the exact duplicates and new options will be
added to the lists in the backend code, not parser. What do you think?

It is done in the 0007 attached. I think it should be applied altogether
with 0001 or before/after, but I put this as the last patch in the set
in order to easier discard it if others would disagree.

Otherwise, everything seems to be working fine. Cannot find any problems
so far.

Regards
--
Alexey Kondratov

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