Horton

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

Justin Pryzby <pryzby@telsasoft.com>

View thread View on postgr.es
From: Justin Pryzby <pryzby@telsasoft.com>
To: Alexey Kondratov <a.kondratov@postgrespro.ru>
Cc: Michael Paquier <michael@paquier.xyz>, Alvaro Herrera <alvherre@2ndquadrant.com>, Masahiko Sawada <masahiko.sawada@2ndquadrant.com>, Steve Singer <steve@ssinger.info>, pgsql-hackers@lists.postgresql.org, Robert Haas <robertmhaas@gmail.com>, Alexander Korotkov <a.korotkov@postgrespro.ru>, Masahiko Sawada <sawada.mshk@gmail.com>, Jose Luis Tallon <jltallon@adv-solutions.net>
Date: 2020-12-04T19:54:15Z
Lists: pgsql-hackers

Attachments

On Fri, Dec 04, 2020 at 09:40:31PM +0300, Alexey Kondratov wrote:
> >  I liked the bools, but dropped them so the patch is smaller.
> 
> I had a look on 0001 and it looks mostly fine to me except some strange
> mixture of tabs/spaces in the ExecReindex(). There is also a couple of
> meaningful comments:
> 
> -	options =
> -		(verbose ? REINDEXOPT_VERBOSE : 0) |
> -		(concurrently ? REINDEXOPT_CONCURRENTLY : 0);
> +	if (verbose)
> +		params.options |= REINDEXOPT_VERBOSE;
> 
> Why do we need this intermediate 'verbose' variable here? We only use it
> once to set a bitmask. Maybe we can do it like this:
> 
> params.options |= defGetBoolean(opt) ?
> 	REINDEXOPT_VERBOSE : 0;

That allows *setting* REINDEXOPT_VERBOSE, but doesn't *unset* it if someone
runs (VERBOSE OFF).  So I kept the bools like Michael originally had rather
than writing "else: params.options &= ~REINDEXOPT_VERBOSE"

> See also attached txt file with diff (I wonder can I trick cfbot this way,
> so it does not apply the diff).

Yes, I think that works :)
I believe it looks for *.diff and *.patch.

> +	int options;	/* bitmask of lowlevel REINDEXOPT_* */
> 
> I would prefer if the comment says '/* bitmask of ReindexOption */' as in
> the VacuumOptions, since citing the exact enum type make it easier to
> navigate source code.

Yes, thanks.

This also fixes some minor formatting and rebase issues, including broken doc/.

-- 
Justin

Commits

  1. Add TABLESPACE option to REINDEX

  2. Refactor code in tablecmds.c to check and process tablespace moves

  3. Refactor option handling of CLUSTER, REINDEX and VACUUM

  4. pg_dump: Don't use enums for defining bit mask values

  5. Refactor CLUSTER and REINDEX grammar to use DefElem for option lists

  6. Refactor parsing rules for option lists of EXPLAIN, VACUUM and ANALYZE

  7. Improve tab completion of REINDEX in psql

  8. Fix possible crash during FATAL exit from reindexing.