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
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 = lappend(indexColNames, NameStr(att->attname));
}
- /* 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 = index_create(heapRelation,
newName,
InvalidOid, /* indexRelationId */
@@ -1395,6 +1401,9 @@ index_concurrently_swap(Oid newIndexId, Oid oldIndexId, const char *oldName)
namestrcpy(&newClassForm->relname, NameStr(oldClassForm->relname));
namestrcpy(&oldClassForm->relname, oldName);
+ /* Copy partitioning flag to track inheritance properly */
+ newClassForm->relispartition = oldClassForm->relispartition;
+
CatalogTupleUpdate(pg_class, &oldClassTuple->t_self, oldClassTuple);
CatalogTupleUpdate(pg_class, &newClassTuple->t_self, newClassTuple);
@@ -1555,31 +1564,28 @@ index_concurrently_swap(Oid newIndexId, Oid oldIndexId, const char *oldName)
}
/*
- * 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 = RelationRelationId;
- myself.objectId = newIndexId;
- myself.objectSubId = 0;
-
- referenced.classId = ConstraintRelationId;
- referenced.objectId = indexConstraintOid;
- referenced.objectSubId = 0;
-
- recordDependencyOn(&myself, &referenced, DEPENDENCY_INTERNAL);
- }
-
+ changeDependenciesOf(RelationRelationId, oldIndexId, newIndexId);
changeDependenciesOn(RelationRelationId, oldIndexId, newIndexId);
+ /*
+ * Inheritance needs to be swapped for partition indexes.
+ */
+ if (get_rel_relispartition(oldIndexId))
+ {
+ List *ancestors = get_partition_ancestors(oldIndexId);
+ Oid parentIndexRelid = 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_depend.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;
}
+/*
+ * Adjust all dependency records to come from a different object of the same 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 = 0;
+ Relation depRel;
+ ScanKeyData key[2];
+ SysScanDesc scan;
+ HeapTuple tup;
+
+ depRel = 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 = systable_beginscan(depRel, DependDependerIndexId, true,
+ NULL, 2, key);
+
+ while (HeapTupleIsValid((tup = systable_getnext(scan))))
+ {
+ Form_pg_depend depform = (Form_pg_depend) GETSTRUCT(tup);
+
+ /* make a modifiable copy */
+ tup = heap_copytuple(tup);
+ depform = (Form_pg_depend) GETSTRUCT(tup);
+
+ depform->objid = 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 same type
*
diff --git a/src/include/catalog/dependency.h b/src/include/catalog/dependency.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 objectId,
Oid refClassId, Oid oldRefObjectId,
Oid newRefObjectId);
+extern long changeDependenciesOf(Oid classId, Oid oldObjectId,
+ Oid newObjectId);
+
extern long changeDependenciesOn(Oid refClassId, Oid oldRefObjectId,
Oid newRefObjectId);
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)
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_part_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_part_index_0_2;
+SELECT relid, parentrelid, level FROM pg_partition_tree('concur_reindex_part_index')
+ ORDER BY relid, level;
+ relid | parentrelid | level
+-------------------------------+-----------------------------+-------
+ 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_part_index')
+ ORDER BY relid, level;
+ relid | parentrelid | level
+-------------------------------+-----------------------------+-------
+ 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_part_index')
+ ORDER BY relid, level;
+ relid | parentrelid | level
+-------------------------------+-----------------------------+-------
+ 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_part_index')
+ ORDER BY relid, level;
+ relid | parentrelid | level
+-------------------------------+-----------------------------+-------
+ 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/create_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_part_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_part_index_0_2;
+SELECT relid, parentrelid, level FROM pg_partition_tree('concur_reindex_part_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_part_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_part_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_part_index')
+ ORDER BY relid, level;
+DROP TABLE concur_reindex_part;
-- Check errors
-- Cannot run inside a transaction block
signature.asc
Description: PGP signature
