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