Date: Sun, 14 Feb 2021 20:10:50 -0600
From: Justin Pryzby <pryzby@telsasoft.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: Alexey Kondratov <a.kondratov@postgrespro.ru>,
	Alvaro Herrera <alvherre@alvh.no-ip.org>,
	Peter Eisentraut <peter.eisentraut@enterprisedb.com>,
	Masahiko Sawada <masahiko.sawada@2ndquadrant.com>,
	Steve Singer <steve@ssinger.info>,
	pgsql-hackers@lists.postgresql.org,
	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
Message-ID: <20210215021050.GA28165@telsasoft.com>
References: <20210127213658.GA736@alvherre.pgsql>
 <55c6c1526b94d2caf54c33262fe00674@postgrespro.ru>
 <e15777decc533006c9dbe4988bd66d31@postgrespro.ru>
 <YBTC0IYIGmyYfiIn@paquier.xyz>
 <1c506564e2a732ec17790bd8c34f042e@postgrespro.ru>
 <YBiro+n0767QDXSA@paquier.xyz>
 <YBpEsxZxIVn0zyFM@paquier.xyz>
 <20210203065342.GX7450@telsasoft.com>
 <YBqA8snxOp2lXLyl@paquier.xyz>
 <YBuWbzoW6W7AaMv0@paquier.xyz>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="azLHFNyN32YCQGCU"
Content-Disposition: inline
In-Reply-To: <YBuWbzoW6W7AaMv0@paquier.xyz>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk


--azLHFNyN32YCQGCU
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Thu, Feb 04, 2021 at 03:38:39PM +0900, Michael Paquier wrote:
> On Wed, Feb 03, 2021 at 07:54:42PM +0900, Michael Paquier wrote:
> > Not sure I like that.  Here is a proposal:
> > "it is recommended to separately use ALTER TABLE ONLY on them so as
> > any new partitions attached inherit the new tablespace value."
> 
> So, I have done more work on this stuff today, and applied that as of
> c5b2860.

> A second thing I have come back to is allow_system_table_mods for
> toast relations, and decided to just forbid TABLESPACE if attempting
> to use it directly on a system table even if allow_system_table_mods
> is true.  This was leading to inconsistent behaviors and weirdness in
> the concurrent case because all the indexes are processed in series
> after building a list.  As we want to ignore the move of toast indexes
> when moving the indexes of the parent table, this was leading to extra
> conditions that are not really worth supporting after thinking about
> it.  One other issue was the lack of consistency when using pg_global
> that was a no-op for the concurrent case but failed in the 
> non-concurrent case.  I have put in place more regression tests for
> all that.

Isn't this dead code ?

postgres=# REINDEX (CONCURRENTLY, TABLESPACE pg_global) TABLE pg_class;
ERROR:  0A000: cannot reindex system catalogs concurrently
LOCATION:  ReindexRelationConcurrently, indexcmds.c:3276

diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 127ba7835d..c77a9b2563 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -3260,73 +3260,66 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params)
 			{
 				if (IsCatalogRelationOid(relationOid))
 					ereport(ERROR,
 							(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
 							 errmsg("cannot reindex system catalogs concurrently")));
 
...
 
-				if (OidIsValid(params->tablespaceOid) &&
-					IsSystemRelation(heapRelation))
-					ereport(ERROR,
-							(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-							 errmsg("cannot move system relation \"%s\"",
-									RelationGetRelationName(heapRelation))));
-
@@ -3404,73 +3397,66 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params)
                                if (IsCatalogRelationOid(heapId))
 					ereport(ERROR,
 							(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
 							 errmsg("cannot reindex system catalogs concurrently")));
... 
 
-				if (OidIsValid(params->tablespaceOid) &&
-					IsSystemRelation(heapRelation))
-					ereport(ERROR,
-							(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-							 errmsg("cannot move system relation \"%s\"",
-									get_rel_name(relationOid))));
-

--azLHFNyN32YCQGCU
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
 filename="0001-Dead-code-REINDEX-CONCURRENTLY-TABLESPACE-.-c5b28604.patch"

From b4347c18bc732d30295c065ef71edaac65e68fe6 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Sun, 14 Feb 2021 19:49:52 -0600
Subject: [PATCH] Dead code: REINDEX (CONCURRENTLY, TABLESPACE ..):
 c5b286047cd698021e57a527215b48865fd4ad4e

---
 src/backend/commands/indexcmds.c | 14 --------------
 1 file changed, 14 deletions(-)

diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 127ba7835d..c77a9b2563 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -3260,73 +3260,66 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params)
 			{
 				/*
 				 * In the case of a relation, find all its indexes including
 				 * toast indexes.
 				 */
 				Relation	heapRelation;
 
 				/* Save the list of relation OIDs in private context */
 				oldcontext = MemoryContextSwitchTo(private_context);
 
 				/* Track this relation for session locks */
 				heapRelationIds = lappend_oid(heapRelationIds, relationOid);
 
 				MemoryContextSwitchTo(oldcontext);
 
 				if (IsCatalogRelationOid(relationOid))
 					ereport(ERROR,
 							(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
 							 errmsg("cannot reindex system catalogs concurrently")));
 
 				/* Open relation to get its indexes */
 				if ((params->options & REINDEXOPT_MISSING_OK) != 0)
 				{
 					heapRelation = try_table_open(relationOid,
 												  ShareUpdateExclusiveLock);
 					/* leave if relation does not exist */
 					if (!heapRelation)
 						break;
 				}
 				else
 					heapRelation = table_open(relationOid,
 											  ShareUpdateExclusiveLock);
 
-				if (OidIsValid(params->tablespaceOid) &&
-					IsSystemRelation(heapRelation))
-					ereport(ERROR,
-							(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-							 errmsg("cannot move system relation \"%s\"",
-									RelationGetRelationName(heapRelation))));
-
 				/* Add all the valid indexes of relation to list */
 				foreach(lc, RelationGetIndexList(heapRelation))
 				{
 					Oid			cellOid = lfirst_oid(lc);
 					Relation	indexRelation = index_open(cellOid,
 														   ShareUpdateExclusiveLock);
 
 					if (!indexRelation->rd_index->indisvalid)
 						ereport(WARNING,
 								(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
 								 errmsg("cannot reindex invalid index \"%s.%s\" concurrently, skipping",
 										get_namespace_name(get_rel_namespace(cellOid)),
 										get_rel_name(cellOid))));
 					else if (indexRelation->rd_index->indisexclusion)
 						ereport(WARNING,
 								(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
 								 errmsg("cannot reindex exclusion constraint index \"%s.%s\" concurrently, skipping",
 										get_namespace_name(get_rel_namespace(cellOid)),
 										get_rel_name(cellOid))));
 					else
 					{
 						ReindexIndexInfo *idx;
 
 						/* Save the list of relation OIDs in private context */
 						oldcontext = MemoryContextSwitchTo(private_context);
 
 						idx = palloc(sizeof(ReindexIndexInfo));
 						idx->indexId = cellOid;
 						/* other fields set later */
 
 						indexIds = lappend(indexIds, idx);
 
 						MemoryContextSwitchTo(oldcontext);
@@ -3404,73 +3397,66 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params)
 					ereport(ERROR,
 							(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
 							 errmsg("cannot reindex system catalogs concurrently")));
 
 				/*
 				 * Don't allow reindex for an invalid index on TOAST table, as
 				 * if rebuilt it would not be possible to drop it.  Match
 				 * error message in reindex_index().
 				 */
 				if (IsToastNamespace(get_rel_namespace(relationOid)) &&
 					!get_index_isvalid(relationOid))
 					ereport(ERROR,
 							(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
 							 errmsg("cannot reindex invalid index on TOAST table")));
 
 				/*
 				 * Check if parent relation can be locked and if it exists,
 				 * this needs to be done at this stage as the list of indexes
 				 * to rebuild is not complete yet, and REINDEXOPT_MISSING_OK
 				 * should not be used once all the session locks are taken.
 				 */
 				if ((params->options & REINDEXOPT_MISSING_OK) != 0)
 				{
 					heapRelation = try_table_open(heapId,
 												  ShareUpdateExclusiveLock);
 					/* leave if relation does not exist */
 					if (!heapRelation)
 						break;
 				}
 				else
 					heapRelation = table_open(heapId,
 											  ShareUpdateExclusiveLock);
 
-				if (OidIsValid(params->tablespaceOid) &&
-					IsSystemRelation(heapRelation))
-					ereport(ERROR,
-							(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-							 errmsg("cannot move system relation \"%s\"",
-									get_rel_name(relationOid))));
-
 				table_close(heapRelation, NoLock);
 
 				/* Save the list of relation OIDs in private context */
 				oldcontext = MemoryContextSwitchTo(private_context);
 
 				/* Track the heap relation of this index for session locks */
 				heapRelationIds = list_make1_oid(heapId);
 
 				/*
 				 * Save the list of relation OIDs in private context.  Note
 				 * that invalid indexes are allowed here.
 				 */
 				idx = palloc(sizeof(ReindexIndexInfo));
 				idx->indexId = relationOid;
 				indexIds = lappend(indexIds, idx);
 				/* other fields set later */
 
 				MemoryContextSwitchTo(oldcontext);
 				break;
 			}
 
 		case RELKIND_PARTITIONED_TABLE:
 		case RELKIND_PARTITIONED_INDEX:
 		default:
 			/* Return error if type of relation is not supported */
 			ereport(ERROR,
 					(errcode(ERRCODE_WRONG_OBJECT_TYPE),
 					 errmsg("cannot reindex this type of relation concurrently")));
 			break;
 	}
 
 	/*
 	 * Definitely no indexes, so leave.  Any checks based on
-- 
2.17.0


--azLHFNyN32YCQGCU--