MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Date: Mon, 06 Apr 2020 20:43:46 +0300
From: Alexey Kondratov <a.kondratov@postgrespro.ru>
To: Justin Pryzby <pryzby@telsasoft.com>
Cc: Michael Paquier <michael@paquier.xyz>, Masahiko Sawada
 <masahiko.sawada@2ndquadrant.com>, Steve Singer <steve@ssinger.info>,
 pgsql-hackers@lists.postgresql.org, Alvaro Herrera
 <alvherre@2ndquadrant.com>, Robert Haas <robertmhaas@gmail.com>, Alexander
 Korotkov <a.korotkov@postgrespro.ru>, Masahiko Sawada
 <sawada.mshk@gmail.com>, Jose Luis Tallon <jltallon@adv-solutions.net>
Subject: Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on
 the fly
In-Reply-To: <20200403182712.GR14618@telsasoft.com>
References: <39a055a7a8cd19552f35f4cb98a8cd80@postgrespro.ru>
 <20200326180112.GH17431@telsasoft.com>
 <20200327040106.GC20103@telsasoft.com>
 <20200328001112.GX20103@telsasoft.com>
 <f85b48beddc77c953f444cef06e1c16f@postgrespro.ru>
 <20200330183439.GQ20103@telsasoft.com>
 <624921b1ebca9897173d85c424c00646@postgrespro.ru>
 <20200401060334.GB142683@paquier.xyz> <20200401115718.GQ14618@telsasoft.com>
 <20200401130836.GT14618@telsasoft.com>
 <20200403182712.GR14618@telsasoft.com>
User-Agent: Roundcube Webmail/1.4.0
Message-ID: <a0fd6af4bdf3cd209a69cea112ab10ea@postgrespro.ru>
Precedence: bulk

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