Date: Tue, 9 Apr 2019 15:50:27 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
Cc: "Shinoda, Noriyoshi (PN Japan A&PS Delivery)" <noriyoshi.shinoda@hpe.com>,
	Sergei Kornilov <sk@zsrv.org>,
	"pgsql-hackers@lists.postgresql.org" <pgsql-hackers@lists.postgresql.org>
Subject: Re: REINDEX CONCURRENTLY 2.0
Message-ID: <20190409065027.GC19558@paquier.xyz>
References: <64744529-d34e-9aae-e7e3-e2263c24fb3b@2ndquadrant.com>
 <7380711553760479@iva5-0acfc31d2b43.qloud-c.yandex.net>
 <13fb3e06-3b66-e878-26d8-0d77e7d50160@2ndquadrant.com>
 <14715291553845163@iva7-8175209a746b.qloud-c.yandex.net>
 <20190329080426.GA1954@paquier.xyz>
 <776f5793-3687-2ba9-913b-a3d5c820bafe@2ndquadrant.com>
 <20190329092122.GC1954@paquier.xyz>
 <DF4PR8401MB11964EDB77C860078C343BEBEE5A0@DF4PR8401MB1196.NAMPRD84.PROD.OUTLOOK.COM>
 <ec449ece-5e7f-f1f2-6e6c-81e305e9af81@2ndquadrant.com>
 <20190401064343.GA16093@paquier.xyz>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="gE7i1rD7pdK0Ng3j"
Content-Disposition: inline
In-Reply-To: <20190401064343.GA16093@paquier.xyz>
Precedence: bulk


--gE7i1rD7pdK0Ng3j
Content-Type: multipart/mixed; boundary="ABTtc+pdwF7KHXCz"
Content-Disposition: inline


--ABTtc+pdwF7KHXCz
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Mon, Apr 01, 2019 at 03:43:43PM +0900, Michael Paquier wrote:
> And I would have expected concur_reindex_part1v1_c1_idx to still be
> part of the partition tree.  I think that the issue is in
> index_concurrently_create_copy() where we create the new index with
> index_create() without setting parentIndexRelid, causing the
> dependency to be lost.  This parameter ought to be set to the OID of
> the parent index so I think that we need to look at the ancestors of
> the index if relispartition is set, and use get_partition_ancestors()
> for that purpose.

And here is the patch to address this issue.  It happens that a bit
more than the dependency switch was lacking here:
- At swap time, we need to have the new index definition track
relispartition from the old index.
- Again at swap time, the inheritance link needs to be updated between
the old/new index and its parent when reindexing a partition index.

Tracking the OID of the parent via index_concurrently_create_copy() is
not a bright idea as we would finish with the impossibility to drop
invalid indexes if the REINDEX CONCURRENTLY failed in the middle (just
added some manual elog(ERROR) to test that).  I have added a comment
before making the index duplica.  I have also expanded the regression
tests so as we have more coverage for all that, finishing with the
attached which keeps partition trees consistent across the operations.
Thoughts?
--
Michael

--ABTtc+pdwF7KHXCz
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="reindex-conc-partition.patch"
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 9b1d546791..9c6c305c1e 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -39,6 +39,7 @@
 #include "catalog/heap.h"
 #include "catalog/index.h"
 #include "catalog/objectaccess.h"
+#include "catalog/partition.h"
 #include "catalog/pg_am.h"
 #include "catalog/pg_collation.h"
 #include "catalog/pg_constraint.h"
@@ -1263,7 +1264,12 @@ index_concurrently_create_copy(Relation heapRelation=
, Oid oldIndexId, const char
 		indexColNames =3D lappend(indexColNames, NameStr(att->attname));
 	}
=20
-	/* Now create the new index */
+	/*
+	 * Now create the new index.  Note that for partition indexes the
+	 * partitionining dependency is switched at swap time to ensure the
+	 * consistency of the operation at the same time dependencies are
+	 * switched, so parentIndexRelid should never be set here.
+	 */
 	newIndexId =3D index_create(heapRelation,
 							  newName,
 							  InvalidOid,	/* indexRelationId */
@@ -1395,6 +1401,9 @@ index_concurrently_swap(Oid newIndexId, Oid oldIndexI=
d, const char *oldName)
 	namestrcpy(&newClassForm->relname, NameStr(oldClassForm->relname));
 	namestrcpy(&oldClassForm->relname, oldName);
=20
+	/* Copy partitioning flag to track inheritance properly */
+	newClassForm->relispartition =3D oldClassForm->relispartition;
+
 	CatalogTupleUpdate(pg_class, &oldClassTuple->t_self, oldClassTuple);
 	CatalogTupleUpdate(pg_class, &newClassTuple->t_self, newClassTuple);
=20
@@ -1555,31 +1564,28 @@ index_concurrently_swap(Oid newIndexId, Oid oldInde=
xId, const char *oldName)
 	}
=20
 	/*
-	 * Move all dependencies on the old index to the new one
+	 * Move all dependencies of and on the old index to the new one.
 	 */
-
-	if (OidIsValid(indexConstraintOid))
-	{
-		ObjectAddress myself,
-					referenced;
-
-		/* Change to having the new index depend on the constraint */
-		deleteDependencyRecordsForClass(RelationRelationId, oldIndexId,
-										ConstraintRelationId, DEPENDENCY_INTERNAL);
-
-		myself.classId =3D RelationRelationId;
-		myself.objectId =3D newIndexId;
-		myself.objectSubId =3D 0;
-
-		referenced.classId =3D ConstraintRelationId;
-		referenced.objectId =3D indexConstraintOid;
-		referenced.objectSubId =3D 0;
-
-		recordDependencyOn(&myself, &referenced, DEPENDENCY_INTERNAL);
-	}
-
+	changeDependenciesOf(RelationRelationId, oldIndexId, newIndexId);
 	changeDependenciesOn(RelationRelationId, oldIndexId, newIndexId);
=20
+	/*
+	 * Inheritance needs to be swapped for partition indexes.
+	 */
+	if (get_rel_relispartition(oldIndexId))
+	{
+		List   *ancestors =3D get_partition_ancestors(oldIndexId);
+		Oid		parentIndexRelid =3D linitial_oid(ancestors);
+
+		/* Remove the old inheritance link first */
+		DeleteInheritsTuple(oldIndexId, parentIndexRelid);
+
+		/* Then add the new one */
+		StoreSingleInheritance(newIndexId, parentIndexRelid, 1);
+
+		list_free(ancestors);
+	}
+
 	/*
 	 * Copy over statistics from old to new index
 	 */
diff --git a/src/backend/catalog/pg_depend.c b/src/backend/catalog/pg_depen=
d.c
index d63bf5e56d..f7caedcc02 100644
--- a/src/backend/catalog/pg_depend.c
+++ b/src/backend/catalog/pg_depend.c
@@ -395,6 +395,62 @@ changeDependencyFor(Oid classId, Oid objectId,
 	return count;
 }
=20
+/*
+ * Adjust all dependency records to come from a different object of the sa=
me type
+ *
+ * classId/oldObjectId specify the old referencing object.
+ * newObjectId is the new referencing object (must be of class classId).
+ *
+ * Returns the number of records updated.
+ */
+long
+changeDependenciesOf(Oid classId, Oid oldObjectId,
+					 Oid newObjectId)
+{
+	long		count =3D 0;
+	Relation	depRel;
+	ScanKeyData key[2];
+	SysScanDesc scan;
+	HeapTuple	tup;
+
+	depRel =3D table_open(DependRelationId, RowExclusiveLock);
+
+	ScanKeyInit(&key[0],
+				Anum_pg_depend_classid,
+				BTEqualStrategyNumber, F_OIDEQ,
+				ObjectIdGetDatum(classId));
+	ScanKeyInit(&key[1],
+				Anum_pg_depend_objid,
+				BTEqualStrategyNumber, F_OIDEQ,
+				ObjectIdGetDatum(oldObjectId));
+
+	scan =3D systable_beginscan(depRel, DependDependerIndexId, true,
+							  NULL, 2, key);
+
+	while (HeapTupleIsValid((tup =3D systable_getnext(scan))))
+	{
+		Form_pg_depend depform =3D (Form_pg_depend) GETSTRUCT(tup);
+
+		/* make a modifiable copy */
+		tup =3D heap_copytuple(tup);
+		depform =3D (Form_pg_depend) GETSTRUCT(tup);
+
+		depform->objid =3D newObjectId;
+
+		CatalogTupleUpdate(depRel, &tup->t_self, tup);
+
+		heap_freetuple(tup);
+
+		count++;
+	}
+
+	systable_endscan(scan);
+
+	table_close(depRel, RowExclusiveLock);
+
+	return count;
+}
+
 /*
  * Adjust all dependency records to point to a different object of the sam=
e type
  *
diff --git a/src/include/catalog/dependency.h b/src/include/catalog/depende=
ncy.h
index 4f9dde9df9..57545b70d8 100644
--- a/src/include/catalog/dependency.h
+++ b/src/include/catalog/dependency.h
@@ -199,6 +199,9 @@ extern long changeDependencyFor(Oid classId, Oid object=
Id,
 					Oid refClassId, Oid oldRefObjectId,
 					Oid newRefObjectId);
=20
+extern long changeDependenciesOf(Oid classId, Oid oldObjectId,
+								 Oid newObjectId);
+
 extern long changeDependenciesOn(Oid refClassId, Oid oldRefObjectId,
 								 Oid newRefObjectId);
=20
diff --git a/src/test/regress/expected/create_index.out b/src/test/regress/=
expected/create_index.out
index 388d709875..9b2a4e0324 100644
--- a/src/test/regress/expected/create_index.out
+++ b/src/test/regress/expected/create_index.out
@@ -3309,6 +3309,91 @@ SELECT obj_description('testcomment_idx1'::regclass,=
 'pg_class');
 (1 row)
=20
 DROP TABLE testcomment;
+-- Partitions
+-- Create partition table layer.
+CREATE TABLE concur_reindex_part (c1 int, c2 int) PARTITION BY RANGE (c1);
+CREATE TABLE concur_reindex_part_0 PARTITION OF concur_reindex_part
+  FOR VALUES FROM (0) TO (10) PARTITION BY list (c2);
+CREATE TABLE concur_reindex_part_0_1 PARTITION OF concur_reindex_part_0
+  FOR VALUES IN (1);
+CREATE TABLE concur_reindex_part_0_2 PARTITION OF concur_reindex_part_0
+  FOR VALUES IN (2);
+-- This partitioned table should remain with no partitions.
+CREATE TABLE concur_reindex_part_10 PARTITION OF concur_reindex_part
+  FOR VALUES FROM (10) TO (20) PARTITION BY list (c2);
+-- Create partition index layer.
+CREATE INDEX concur_reindex_part_index ON ONLY concur_reindex_part (c1);
+CREATE INDEX concur_reindex_part_index_0 ON ONLY concur_reindex_part_0 (c1=
);
+ALTER INDEX concur_reindex_part_index ATTACH PARTITION concur_reindex_part=
_index_0;
+-- This partitioned index should remain with no partitions.
+CREATE INDEX concur_reindex_part_index_10 ON ONLY concur_reindex_part_10 (=
c1);
+ALTER INDEX concur_reindex_part_index ATTACH PARTITION concur_reindex_part=
_index_10;
+CREATE INDEX concur_reindex_part_index_0_1 ON ONLY concur_reindex_part_0_1=
 (c1);
+ALTER INDEX concur_reindex_part_index_0 ATTACH PARTITION concur_reindex_pa=
rt_index_0_1;
+CREATE INDEX concur_reindex_part_index_0_2 ON ONLY concur_reindex_part_0_2=
 (c1);
+ALTER INDEX concur_reindex_part_index_0 ATTACH PARTITION concur_reindex_pa=
rt_index_0_2;
+SELECT relid, parentrelid, level FROM pg_partition_tree('concur_reindex_pa=
rt_index')
+  ORDER BY relid, level;
+             relid             |         parentrelid         | level=20
+-------------------------------+-----------------------------+-------
+ concur_reindex_part_index     |                             |     0
+ concur_reindex_part_index_0   | concur_reindex_part_index   |     1
+ concur_reindex_part_index_10  | concur_reindex_part_index   |     1
+ concur_reindex_part_index_0_1 | concur_reindex_part_index_0 |     2
+ concur_reindex_part_index_0_2 | concur_reindex_part_index_0 |     2
+(5 rows)
+
+-- REINDEX fails for partitioned indexes
+REINDEX INDEX concur_reindex_part_index_10;
+ERROR:  REINDEX is not yet implemented for partitioned indexes
+REINDEX INDEX CONCURRENTLY concur_reindex_part_index_10;
+ERROR:  REINDEX is not yet implemented for partitioned indexes
+-- REINDEX is a no-op for partitioned tables
+REINDEX TABLE concur_reindex_part_10;
+WARNING:  REINDEX of partitioned tables is not yet implemented, skipping "=
concur_reindex_part_10"
+NOTICE:  table "concur_reindex_part_10" has no indexes
+REINDEX TABLE CONCURRENTLY concur_reindex_part_10;
+WARNING:  REINDEX of partitioned tables is not yet implemented, skipping "=
concur_reindex_part_10"
+NOTICE:  table "concur_reindex_part_10" has no indexes
+SELECT relid, parentrelid, level FROM pg_partition_tree('concur_reindex_pa=
rt_index')
+  ORDER BY relid, level;
+             relid             |         parentrelid         | level=20
+-------------------------------+-----------------------------+-------
+ concur_reindex_part_index     |                             |     0
+ concur_reindex_part_index_0   | concur_reindex_part_index   |     1
+ concur_reindex_part_index_10  | concur_reindex_part_index   |     1
+ concur_reindex_part_index_0_1 | concur_reindex_part_index_0 |     2
+ concur_reindex_part_index_0_2 | concur_reindex_part_index_0 |     2
+(5 rows)
+
+-- REINDEX should preserve dependencies of partition tree.
+REINDEX INDEX CONCURRENTLY concur_reindex_part_index_0_1;
+REINDEX INDEX CONCURRENTLY concur_reindex_part_index_0_2;
+SELECT relid, parentrelid, level FROM pg_partition_tree('concur_reindex_pa=
rt_index')
+  ORDER BY relid, level;
+             relid             |         parentrelid         | level=20
+-------------------------------+-----------------------------+-------
+ concur_reindex_part_index     |                             |     0
+ concur_reindex_part_index_0   | concur_reindex_part_index   |     1
+ concur_reindex_part_index_10  | concur_reindex_part_index   |     1
+ concur_reindex_part_index_0_1 | concur_reindex_part_index_0 |     2
+ concur_reindex_part_index_0_2 | concur_reindex_part_index_0 |     2
+(5 rows)
+
+REINDEX TABLE CONCURRENTLY concur_reindex_part_0_1;
+REINDEX TABLE CONCURRENTLY concur_reindex_part_0_2;
+SELECT relid, parentrelid, level FROM pg_partition_tree('concur_reindex_pa=
rt_index')
+  ORDER BY relid, level;
+             relid             |         parentrelid         | level=20
+-------------------------------+-----------------------------+-------
+ concur_reindex_part_index     |                             |     0
+ concur_reindex_part_index_0   | concur_reindex_part_index   |     1
+ concur_reindex_part_index_10  | concur_reindex_part_index   |     1
+ concur_reindex_part_index_0_1 | concur_reindex_part_index_0 |     2
+ concur_reindex_part_index_0_2 | concur_reindex_part_index_0 |     2
+(5 rows)
+
+DROP TABLE concur_reindex_part;
 -- Check errors
 -- Cannot run inside a transaction block
 BEGIN;
diff --git a/src/test/regress/sql/create_index.sql b/src/test/regress/sql/c=
reate_index.sql
index 4d2535b482..b1f3767165 100644
--- a/src/test/regress/sql/create_index.sql
+++ b/src/test/regress/sql/create_index.sql
@@ -1211,6 +1211,49 @@ SELECT obj_description('testcomment_idx1'::regclass,=
 'pg_class');
 REINDEX TABLE CONCURRENTLY testcomment ;
 SELECT obj_description('testcomment_idx1'::regclass, 'pg_class');
 DROP TABLE testcomment;
+-- Partitions
+-- Create partition table layer.
+CREATE TABLE concur_reindex_part (c1 int, c2 int) PARTITION BY RANGE (c1);
+CREATE TABLE concur_reindex_part_0 PARTITION OF concur_reindex_part
+  FOR VALUES FROM (0) TO (10) PARTITION BY list (c2);
+CREATE TABLE concur_reindex_part_0_1 PARTITION OF concur_reindex_part_0
+  FOR VALUES IN (1);
+CREATE TABLE concur_reindex_part_0_2 PARTITION OF concur_reindex_part_0
+  FOR VALUES IN (2);
+-- This partitioned table should remain with no partitions.
+CREATE TABLE concur_reindex_part_10 PARTITION OF concur_reindex_part
+  FOR VALUES FROM (10) TO (20) PARTITION BY list (c2);
+-- Create partition index layer.
+CREATE INDEX concur_reindex_part_index ON ONLY concur_reindex_part (c1);
+CREATE INDEX concur_reindex_part_index_0 ON ONLY concur_reindex_part_0 (c1=
);
+ALTER INDEX concur_reindex_part_index ATTACH PARTITION concur_reindex_part=
_index_0;
+-- This partitioned index should remain with no partitions.
+CREATE INDEX concur_reindex_part_index_10 ON ONLY concur_reindex_part_10 (=
c1);
+ALTER INDEX concur_reindex_part_index ATTACH PARTITION concur_reindex_part=
_index_10;
+CREATE INDEX concur_reindex_part_index_0_1 ON ONLY concur_reindex_part_0_1=
 (c1);
+ALTER INDEX concur_reindex_part_index_0 ATTACH PARTITION concur_reindex_pa=
rt_index_0_1;
+CREATE INDEX concur_reindex_part_index_0_2 ON ONLY concur_reindex_part_0_2=
 (c1);
+ALTER INDEX concur_reindex_part_index_0 ATTACH PARTITION concur_reindex_pa=
rt_index_0_2;
+SELECT relid, parentrelid, level FROM pg_partition_tree('concur_reindex_pa=
rt_index')
+  ORDER BY relid, level;
+-- REINDEX fails for partitioned indexes
+REINDEX INDEX concur_reindex_part_index_10;
+REINDEX INDEX CONCURRENTLY concur_reindex_part_index_10;
+-- REINDEX is a no-op for partitioned tables
+REINDEX TABLE concur_reindex_part_10;
+REINDEX TABLE CONCURRENTLY concur_reindex_part_10;
+SELECT relid, parentrelid, level FROM pg_partition_tree('concur_reindex_pa=
rt_index')
+  ORDER BY relid, level;
+-- REINDEX should preserve dependencies of partition tree.
+REINDEX INDEX CONCURRENTLY concur_reindex_part_index_0_1;
+REINDEX INDEX CONCURRENTLY concur_reindex_part_index_0_2;
+SELECT relid, parentrelid, level FROM pg_partition_tree('concur_reindex_pa=
rt_index')
+  ORDER BY relid, level;
+REINDEX TABLE CONCURRENTLY concur_reindex_part_0_1;
+REINDEX TABLE CONCURRENTLY concur_reindex_part_0_2;
+SELECT relid, parentrelid, level FROM pg_partition_tree('concur_reindex_pa=
rt_index')
+  ORDER BY relid, level;
+DROP TABLE concur_reindex_part;
=20
 -- Check errors
 -- Cannot run inside a transaction block

--ABTtc+pdwF7KHXCz--

--gE7i1rD7pdK0Ng3j
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAlysQLMACgkQnvQgOdby
QH3TCQ/9Gt3gS0MRZk4BXIXNLbfCpdr5NMuCq+lxJdlhE7qQL21m7IYv6V6r2VLe
Klw9joGW4n8euGqyW/E/h1ocNcWYSSsup8ZamexLXiwVT2v1B3mj9epiM5T1gjJl
S5UapR8FqbI3NvfxHLGad3TqKqK/GwUmIHCQTjiztX1r3PkaLURrWDlASiMy3qQ1
piJnSUnISo95U2iCWllKJ8YxOMYSQsGLzs6TZIeB0Xi9zMgL7gFEqcUkETMDxD31
1hD31a8nbA+SiP8+4+1mSo8nSTmvsC9BUwT17+iWv6Tb7eCXK3EtjA6GryTs8/jd
4D7jFE2pd3PUicu4JrDTFjddeI/VlAah0xjb03KUIHC301o/mu5YFqfev2dE7Kkp
vGSyygjf/TlkWoDgwzT6FFsnSJUlOQTPnjnhJU0ggFlt6tL/lL8hclQLgWl1kMCo
eU4TYojcjZinsINF3R5Ub4bmTwcrGPVZsCkmid0UMVjUfd3xCbmnnmDi88492usU
ZZBUYemFlQlV2wCMGD5k5wBSx0lhk6YLT3iFTJr5/jY7CG8O8vPK20vBKWhfQKae
N7d6F9vFYFwlOYgBpwm0nbbj43HwmkWCOVyrvHfVsaGW9ukhVKHBNr+hYRuiXKTw
Z+7ETnP2EtN8eaqvKv5wng0gy3/f2VmTGAy/Hu91HF20Y3nIIwM=
=KTPu
-----END PGP SIGNATURE-----

--gE7i1rD7pdK0Ng3j--