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: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>, Michael Paquier <michael@paquier.xyz>, Alexey Kondratov <a.kondratov@postgrespro.ru>, 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-12T19:45:26Z
Lists: pgsql-hackers

Attachments

On Sat, Dec 12, 2020 at 09:20:35AM +0100, Peter Eisentraut wrote:
> On 2020-12-11 21:27, Alvaro Herrera wrote:
> > By the way--  What did you think of the idea of explictly marking the
> > types used for bitmasks using types bits32 and friends, instead of plain
> > int, which is harder to spot?
> 
> If we want to make it clearer, why not turn the thing into a struct, as in
> the attached patch, and avoid the bit fiddling altogether.

I like this.
It's a lot like what I wrote as [PATCH v31 1/5] ExecReindex and ReindexParams
In my v31 patch, I moved ReindexOptions to a private structure in indexcmds.c,
with an "int options" bitmask which is passed to reindex_index() et al.  Your
patch keeps/puts ReindexOptions index.h, so it also applies to reindex_index,
which I think is good.

So I've rebased this branch on your patch.

Some thoughts:

 - what about removing the REINDEXOPT_* prefix ?
 - You created local vars with initialization like "={}". But I thought it's
   needed to include at least one struct member like "={false}", or else
   they're not guaranteed to be zerod ?
 - You passed the structure across function calls.  The usual convention is to
   pass a pointer.

I also changed the errcode and detail for this one.
		(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
		errmsg("incompatible TABLESPACE option"),
		errdetail("TABLESPACE can only be used with VACUUM FULL.")));

-- 
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.