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

From: Michael Paquier <michael(at)paquier(dot)xyz>
To: Alexey Kondratov <a(dot)kondratov(at)postgrespro(dot)ru>
Cc: Justin Pryzby <pryzby(at)telsasoft(dot)com>, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, Masahiko Sawada <masahiko(dot)sawada(at)2ndquadrant(dot)com>, Steve Singer <steve(at)ssinger(dot)info>, pgsql-hackers(at)lists(dot)postgresql(dot)org, Robert Haas <robertmhaas(at)gmail(dot)com>, Alexander Korotkov <a(dot)korotkov(at)postgrespro(dot)ru>, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>, Jose Luis Tallon <jltallon(at)adv-solutions(dot)net>
Subject: Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
Date: 2020-12-01 02:46:55
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Mon, Nov 30, 2020 at 05:12:42PM +0300, Alexey Kondratov wrote:
> Thanks. I have rebased the remaining patches on top of 873ea9ee to use
> 'utility_option_list' instead of 'common_option_list'.

Thanks, that helps a lot. I have gone through 0002, and tweaked it as
the attached (note that this patch is also interesting for another
thing in development: backend-side reindex filtering of
collation-sensitive indexes). Does that look right to you?

These are mostly matters of consistency with the other commands using
DefElem, but I think that it is important to get things right:
- Having the list of options in parsenodes.h becomes incorrect,
because these get now only used at execution time, like VACUUM. So I
have moved that to cluster.h and index.h.
- Let's use an enum for REINDEX, like the others.
- Having parse_reindex_params() in utility.c is wrong for something
aimed at being used only for REINDEX, so I have moved that to
indexcmds.c, and renamed the routine to be more consistent with the
rest. I think that we could more here by having an ExecReindex() that
does all the work based on object types, but I have left that out for
now to keep the change minimal.
- Switched one of the existing tests to stress CONCURRENTLY within
- Indented the whole.

A couple of extra things below.

* CLUSTER [VERBOSE] <qualified_name> [ USING <index_name> ]
+ * CLUSTER [VERBOSE] [(options)] <qualified_name> [ USING <index_name> ]
This line is wrong, and should be:
CLUSTER [ (options) ] <qualified_name> [ USING <index_name> ]

+CLUSTER [VERBOSE] [ ( <replaceable class="parameter">option</replaceable>
+CLUSTER [VERBOSE] [ ( <replaceable class="parameter">option</replaceable> [, ...] ) ]
The docs in cluster.sgml are wrong as well, you can have VERBOSE as a
single option or as a parenthesized option, but never both at the same
time. On the contrary, psql completion got that right. I was first a
bit surprised that you would not allow the parenthesized set for the
case where a relation is not specified in the command, but I agree
that this does not seem worth the extra complexity now as this thread
aims at being able to use TABLESPACE which makes little sense

+ VERBOSE [ <replaceable class="parameter">boolean</replaceable> ]
Forgot about CONCURRENTLY as an option here, as this becomes

Attachment Content-Type Size
v32-reindex-cluster-gram.patch text/x-diff 20.3 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Thomas Munro 2020-12-01 02:48:31 Re: BUG #16663: DROP INDEX did not free up disk space: idle connection hold file marked as deleted
Previous Message 2020-12-01 02:46:07 RE: [Patch] Optimize dropping of relation buffers using dlist