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

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) ?
    REINDEXOPT_VERBOSE : 0;

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 
get_tablespace_oid().

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.


Regards
-- 
Alexey Kondratov

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