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

From: Alexey Kondratov <a(dot)kondratov(at)postgrespro(dot)ru>
To: Justin Pryzby <pryzby(at)telsasoft(dot)com>
Cc: Michael Paquier <michael(at)paquier(dot)xyz>, 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-04 18:40:31
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On 2020-12-04 04:25, Justin Pryzby wrote:
> On Thu, Dec 03, 2020 at 04:12:53PM +0900, Michael Paquier wrote:
>> > +typedef struct ReindexParams {
>> > + bool concurrently;
>> > + bool verbose;
>> > + bool missingok;
>> > +
>> > + int options; /* bitmask of lowlevel REINDEXOPT_* */
>> > +} ReindexParams;
>> > +
>> By moving everything into indexcmds.c, keeping ReindexParams within it
>> makes sense to me. Now, there is no need for the three booleans
>> because options stores the same information, no?
> 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) ?

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

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

> Regarding the REINDEX patch, I think this comment is misleading:
> |+ * Even if table was moved to new tablespace,
> normally toast cannot move.
> | */
> |+ Oid toasttablespaceOid = allowSystemTableMods ?
> tablespaceOid : InvalidOid;
> | result |= reindex_relation(toast_relid, flags,
> I think it ought to say "Even if a table's indexes were moved to a new
> tablespace, its toast table's index is not normally moved"
> Right ?

Yes, I think so, we are dealing only with index tablespace changing
here. Thanks for noticing.

> Also, I don't know whether we should check for GLOBALTABLESPACE_OID
> after
> calling get_tablespace_oid(), or in the lowlevel routines. Note that
> reindex_relation is called during cluster/vacuum, and in the later
> patches, I
> moved the test from from cluster() and ExecVacuum() to
> rebuild_relation().

IIRC, I wanted to do GLOBALTABLESPACE_OID check as early as possible
(just after getting Oid), since it does not make sense to proceed
further if tablespace is set to that value. So initially there were a
lot of duplicative GLOBALTABLESPACE_OID checks, since there were a lot
of reindex entry-points (index, relation, concurrently, etc.). Now we
are going to have ExecReindex(), so there are much less entry-points and
in my opinion it is fine to keep this validation just after

However, this is mostly a sanity check. I can hardly imagine a lot of
users trying to constantly move indexes to the global tablespace, so it
is also OK to put this check deeper into guts.

Alexey Kondratov

Postgres Professional
Russian Postgres Company

Attachment Content-Type Size
refactor-ExecReindex.txt text/x-diff 1.6 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Joel Jacobson 2020-12-04 18:44:37 Re: [PATCH] Add support for leading/trailing bytea trim()ing
Previous Message Alvaro Herrera 2020-12-04 18:29:51 Re: POC: Better infrastructure for automated testing of concurrency issues