From b889658992c20011c10ecb05fd43e59ac5475d9e Mon Sep 17 00:00:00 2001 From: Justin Pryzby Date: Sat, 6 Jun 2020 17:42:23 -0500 Subject: [PATCH v2 1/3] Allow CREATE INDEX CONCURRENTLY on partitioned table --- doc/src/sgml/ref/create_index.sgml | 9 ---- src/backend/commands/indexcmds.c | 68 ++++++++++++++------------ src/test/regress/expected/indexing.out | 14 +++++- src/test/regress/sql/indexing.sql | 3 +- 4 files changed, 52 insertions(+), 42 deletions(-) diff --git a/doc/src/sgml/ref/create_index.sgml b/doc/src/sgml/ref/create_index.sgml index ff87b2d28f..e1298b8523 100644 --- a/doc/src/sgml/ref/create_index.sgml +++ b/doc/src/sgml/ref/create_index.sgml @@ -657,15 +657,6 @@ Indexes: cannot. - - Concurrent builds for indexes on partitioned tables are currently not - supported. However, you may concurrently build the index on each - partition individually and then finally create the partitioned index - non-concurrently in order to reduce the time where writes to the - partitioned table will be locked out. In this case, building the - partitioned index is a metadata only operation. - - diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index 2baca12c5f..344cabaf52 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -652,17 +652,6 @@ DefineIndex(Oid relationId, partitioned = rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE; if (partitioned) { - /* - * Note: we check 'stmt->concurrent' rather than 'concurrent', so that - * the error is thrown also for temporary tables. Seems better to be - * consistent, even though we could do it on temporary table because - * we're not actually doing it concurrently. - */ - if (stmt->concurrent) - ereport(ERROR, - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), - errmsg("cannot create index on partitioned table \"%s\" concurrently", - RelationGetRelationName(rel)))); if (stmt->excludeOpNames) ereport(ERROR, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), @@ -1139,8 +1128,26 @@ DefineIndex(Oid relationId, CreateComments(indexRelationId, RelationRelationId, 0, stmt->idxcomment); + /* save lockrelid and locktag for below */ + heaprelid = rel->rd_lockInfo.lockRelId; + SET_LOCKTAG_RELATION(heaplocktag, heaprelid.dbId, heaprelid.relId); + if (partitioned) { + /* + * Need to close the relation before recursing into children, so copy + * needed data. + */ + PartitionDesc partdesc = RelationGetPartitionDesc(rel); + int nparts = partdesc->nparts; + TupleDesc parentDesc = CreateTupleDescCopy(RelationGetDescr(rel)); + char *relname = pstrdup(RelationGetRelationName(rel)); + Oid *part_oids; + + part_oids = palloc(sizeof(Oid) * nparts); + memcpy(part_oids, partdesc->oids, sizeof(Oid) * nparts); + table_close(rel, NoLock); + /* * Unless caller specified to skip this step (via ONLY), process each * partition to make sure they all contain a corresponding index. @@ -1149,19 +1156,11 @@ DefineIndex(Oid relationId, */ if (!stmt->relation || stmt->relation->inh) { - PartitionDesc partdesc = RelationGetPartitionDesc(rel); - int nparts = partdesc->nparts; - Oid *part_oids = palloc(sizeof(Oid) * nparts); bool invalidate_parent = false; - TupleDesc parentDesc; Oid *opfamOids; pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITIONS_TOTAL, nparts); - - memcpy(part_oids, partdesc->oids, sizeof(Oid) * nparts); - - parentDesc = RelationGetDescr(rel); opfamOids = palloc(sizeof(Oid) * numberOfKeyAttributes); for (i = 0; i < numberOfKeyAttributes; i++) opfamOids[i] = get_opclass_family(classObjectId[i]); @@ -1196,9 +1195,9 @@ DefineIndex(Oid relationId, ereport(ERROR, (errcode(ERRCODE_WRONG_OBJECT_TYPE), errmsg("cannot create unique index on partitioned table \"%s\"", - RelationGetRelationName(rel)), + relname), errdetail("Table \"%s\" contains partitions that are foreign tables.", - RelationGetRelationName(rel)))); + relname))); table_close(childrel, lockmode); continue; @@ -1365,21 +1364,35 @@ DefineIndex(Oid relationId, } } + /* + * CIC needs to mark a partitioned table as VALID, which itself + * requires setting READY, which is unset for CIC (even though + * it's meaningless for an index without storage). + */ + if (concurrent) + { + if (ActiveSnapshotSet()) + PopActiveSnapshot(); + CommitTransactionCommand(); + StartTransactionCommand(); + index_set_state_flags(indexRelationId, INDEX_CREATE_SET_READY); + + CommandCounterIncrement(); + index_set_state_flags(indexRelationId, INDEX_CREATE_SET_VALID); + } + /* * Indexes on partitioned tables are not themselves built, so we're * done here. */ - table_close(rel, NoLock); if (!OidIsValid(parentIndexId)) pgstat_progress_end_command(); return address; } + table_close(rel, NoLock); if (!concurrent) { - /* Close the heap and we're done, in the non-concurrent case */ - table_close(rel, NoLock); - /* If this is the top-level index, we're done. */ if (!OidIsValid(parentIndexId)) pgstat_progress_end_command(); @@ -1387,11 +1400,6 @@ DefineIndex(Oid relationId, return address; } - /* save lockrelid and locktag for below, then close rel */ - heaprelid = rel->rd_lockInfo.lockRelId; - SET_LOCKTAG_RELATION(heaplocktag, heaprelid.dbId, heaprelid.relId); - table_close(rel, NoLock); - /* * For a concurrent build, it's important to make the catalog entries * visible to other transactions before we start to build the index. That diff --git a/src/test/regress/expected/indexing.out b/src/test/regress/expected/indexing.out index f78865ef81..17fa77e317 100644 --- a/src/test/regress/expected/indexing.out +++ b/src/test/regress/expected/indexing.out @@ -50,11 +50,21 @@ select relname, relkind, relhassubclass, inhparent::regclass (8 rows) drop table idxpart; --- Some unsupported features +-- CIC on partitioned table create table idxpart (a int, b int, c text) partition by range (a); create table idxpart1 partition of idxpart for values from (0) to (10); create index concurrently on idxpart (a); -ERROR: cannot create index on partitioned table "idxpart" concurrently +\d idxpart1 + Table "public.idxpart1" + Column | Type | Collation | Nullable | Default +--------+---------+-----------+----------+--------- + a | integer | | | + b | integer | | | + c | text | | | +Partition of: idxpart FOR VALUES FROM (0) TO (10) +Indexes: + "idxpart1_a_idx" btree (a) + drop table idxpart; -- Verify bugfix with query on indexed partitioned table with no partitions -- https://postgr.es/m/20180124162006.pmapfiznhgngwtjf@alvherre.pgsql diff --git a/src/test/regress/sql/indexing.sql b/src/test/regress/sql/indexing.sql index 35d159f41b..19a21eba9d 100644 --- a/src/test/regress/sql/indexing.sql +++ b/src/test/regress/sql/indexing.sql @@ -29,10 +29,11 @@ select relname, relkind, relhassubclass, inhparent::regclass where relname like 'idxpart%' order by relname; drop table idxpart; --- Some unsupported features +-- CIC on partitioned table create table idxpart (a int, b int, c text) partition by range (a); create table idxpart1 partition of idxpart for values from (0) to (10); create index concurrently on idxpart (a); +\d idxpart1 drop table idxpart; -- Verify bugfix with query on indexed partitioned table with no partitions -- 2.17.0