Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace onthe fly - Mailing list pgsql-hackers

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