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

On 2020-04-03 21:27, Justin Pryzby wrote:
> On Wed, Apr 01, 2020 at 08:08:36AM -0500, Justin Pryzby wrote:
>> Or maybe you'd want me to squish my changes into yours and resend
>> after any
>> review comments ?
>
> I didn't hear any feedback, so I've now squished all "parenthesized"
> and "fix"
> commits.
>

Thanks for the input, but I am afraid that the patch set became a bit
messy now. I have eyeballed it and found some inconsistencies.

const char *name; /* name of database to reindex */
- int options; /* Reindex options flags */
+ List *rawoptions; /* Raw options */
+ int options; /* Parsed options */
bool concurrent; /* reindex concurrently? */

You introduced rawoptions in the 0002, but then removed it in 0003. So
is it required or not? Probably this is a rebase artefact.

+/* XXX: reusing reindex_option_list */
+ | CLUSTER opt_verbose '(' reindex_option_list ')' qualified_name
cluster_index_specification

Could we actually simply reuse vac_analyze_option_list? From the first
sight it does just the right thing, excepting the special handling of
spelling ANALYZE/ANALYSE, but it does not seem to be a problem.

>
> 0004 reduces duplicative error handling, as a separate commit so
> Alexey can review it and/or integrate it.
>

@@ -2974,27 +2947,6 @@ ReindexRelationConcurrently(Oid relationOid, Oid
tablespaceOid, int options)
/* Open relation to get its indexes */
heapRelation = table_open(relationOid, ShareUpdateExclusiveLock);
- /*
- * We don't support moving system relations into different
tablespaces,
- * unless allow_system_table_mods=1.
- */
- if (OidIsValid(tablespaceOid) &&
- !allowSystemTableMods && IsSystemRelation(heapRelation))
- ereport(ERROR,
- (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("permission denied: \"%s\" is a system catalog",
- RelationGetRelationName(heapRelation))));

ReindexRelationConcurrently is used for all cases, but it hits different
code paths in the case of database, table and index. I have not checked
yet, but are you sure it is safe removing these validations in the case
of REINDEX CONCURRENTLY?

>
> The last two commits save a few
> dozen lines of code, but not sure they're desirable.
>

Sincerely, I do not think that passing raw strings down to the guts is a
good idea. Yes, it saves us a few checks here and there now, but it may
reduce a further reusability of these internal routines in the future.

>
> XXX: for cluster/vacuum, it might be more friendly to check before
> clustering
> the table, rather than after clustering and re-indexing.
>

Yes, I think it would be much more user-friendly.

--
Alexey Kondratov

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