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>
Cc: Sergei Kornilov <sk@zsrv.org>
Cc: pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>
Subject: Re: REINDEX CONCURRENTLY 2.0
Date: Tue, 9 Apr 2019 15:50:27 +0900
Message-ID: <20190409065027.GC19558@paquier.xyz> (raw)
In-Reply-To: <20190401064343.GA16093@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>
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
Attachments:
[text/x-diff] reindex-conc-partition.patch (13.3K, ../20190409065027.GC19558@paquier.xyz/2-reindex-conc-partition.patch)
download | inline diff:diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.cindex 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.cindex 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.hindex 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.outindex 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.sqlindex 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
[application/pgp-signature] signature.asc (832B, ../20190409065027.GC19558@paquier.xyz/3-signature.asc)
download
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: michael@paquier.xyz, peter.eisentraut@2ndquadrant.com, noriyoshi.shinoda@hpe.com, sk@zsrv.org, pgsql-hackers@lists.postgresql.org
Subject: Re: REINDEX CONCURRENTLY 2.0
In-Reply-To: <20190409065027.GC19558@paquier.xyz>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox