public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v5 2/3] Allow CREATE INDEX CONCURRENTLY on partitioned table 3+ messages / 3 participants [nested] [flat]
* [PATCH v5 2/3] Allow CREATE INDEX CONCURRENTLY on partitioned table @ 2020-06-06 22:42 Justin Pryzby <[email protected]> 0 siblings, 0 replies; 3+ messages in thread From: Justin Pryzby @ 2020-06-06 22:42 UTC (permalink / raw) Note, this effectively reverts 050098b14, so take care to not reintroduce the bug it fixed. --- doc/src/sgml/ref/create_index.sgml | 9 --- src/backend/commands/indexcmds.c | 104 +++++++++++++++++++------ src/test/regress/expected/indexing.out | 57 +++++++++++++- src/test/regress/sql/indexing.sql | 16 +++- 4 files changed, 148 insertions(+), 38 deletions(-) diff --git a/doc/src/sgml/ref/create_index.sgml b/doc/src/sgml/ref/create_index.sgml index 33aa64e81d..c780dc9547 100644 --- a/doc/src/sgml/ref/create_index.sgml +++ b/doc/src/sgml/ref/create_index.sgml @@ -657,15 +657,6 @@ Indexes: cannot. </para> - <para> - 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. - </para> - </refsect2> </refsect1> diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index 68d75916ae..cdf31a47d8 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -655,17 +655,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), @@ -1100,6 +1089,9 @@ DefineIndex(Oid relationId, if (pd->nparts != 0) flags |= INDEX_CREATE_INVALID; } + else if (concurrent && OidIsValid(parentIndexId)) + /* Initial concurrent build of index partition is invalid */ + flags |= INDEX_CREATE_INVALID; if (stmt->deferrable) constr_flags |= INDEX_CONSTR_CREATE_DEFERRABLE; @@ -1152,8 +1144,17 @@ DefineIndex(Oid relationId, */ if (!stmt->relation || stmt->relation->inh) { + /* + * Need to close the relation before recursing into children, so + * copy needed data into a longlived context. + */ + + MemoryContext ind_context = AllocSetContextCreate(PortalContext, "CREATE INDEX", + ALLOCSET_DEFAULT_SIZES); + MemoryContext oldcontext = MemoryContextSwitchTo(ind_context); PartitionDesc partdesc = RelationGetPartitionDesc(rel); int nparts = partdesc->nparts; + char *relname = pstrdup(RelationGetRelationName(rel)); Oid *part_oids = palloc(sizeof(Oid) * nparts); bool invalidate_parent = false; TupleDesc parentDesc; @@ -1163,8 +1164,10 @@ DefineIndex(Oid relationId, nparts); memcpy(part_oids, partdesc->oids, sizeof(Oid) * nparts); + parentDesc = CreateTupleDescCopy(RelationGetDescr(rel)); + table_close(rel, NoLock); + MemoryContextSwitchTo(oldcontext); - parentDesc = RelationGetDescr(rel); opfamOids = palloc(sizeof(Oid) * numberOfKeyAttributes); for (i = 0; i < numberOfKeyAttributes; i++) opfamOids[i] = get_opclass_family(classObjectId[i]); @@ -1199,18 +1202,20 @@ 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; } + oldcontext = MemoryContextSwitchTo(ind_context); childidxs = RelationGetIndexList(childrel); attmap = build_attrmap_by_name(RelationGetDescr(childrel), parentDesc); + MemoryContextSwitchTo(oldcontext); foreach(cell, childidxs) { @@ -1336,10 +1341,17 @@ DefineIndex(Oid relationId, createdConstraintId, is_alter_table, check_rights, check_not_in_use, skip_build, quiet); + if (concurrent) + { + PushActiveSnapshot(GetTransactionSnapshot()); + invalidate_parent = true; + } } - pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITIONS_DONE, - i + 1); + /* For concurrent build, this is a catalog-only stage */ + if (!concurrent) + pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITIONS_DONE, + i + 1); free_attrmap(attmap); } @@ -1366,23 +1378,72 @@ DefineIndex(Oid relationId, table_close(pg_index, RowExclusiveLock); heap_freetuple(newtup); } + } else + table_close(rel, NoLock); + + if (concurrent) + { + List *childs; + ListCell *lc; + int npart = 0; + + /* Reindex invalid child indexes created earlier */ + MemoryContext ind_context = AllocSetContextCreate(PortalContext, "CREATE INDEX", + ALLOCSET_DEFAULT_SIZES); + MemoryContext oldcontext = MemoryContextSwitchTo(ind_context); + childs = find_inheritance_children(indexRelationId, NoLock); + MemoryContextSwitchTo(oldcontext); + + /* Make the catalog changes visible to get_partition_parent */ + CommandCounterIncrement(); + + foreach (lc, childs) + { + Oid indexrelid = lfirst_oid(lc); + Relation cldidx; + bool isvalid; + + if (!OidIsValid(parentIndexId)) + pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITIONS_DONE, + npart++); + + cldidx = index_open(indexrelid, ShareUpdateExclusiveLock); + isvalid = cldidx->rd_index->indisvalid; + index_close(cldidx, NoLock); + if (isvalid) + continue; + + /* This may be a partitioned index, which is fine too */ + ReindexRelationConcurrently(indexrelid, 0); + PushActiveSnapshot(GetTransactionSnapshot()); + } + + PopActiveSnapshot(); + CommitTransactionCommand(); + StartTransactionCommand(); + + /* + * CIC needs to mark a partitioned index as VALID, which itself + * requires setting READY, which is unset for CIC (even though + * it's meaningless for an index without storage). + */ + 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(); @@ -1390,10 +1451,9 @@ DefineIndex(Oid relationId, return address; } - /* save lockrelid and locktag for below, then close rel */ + /* save lockrelid and locktag for below */ 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 diff --git a/src/test/regress/expected/indexing.out b/src/test/regress/expected/indexing.out index f78865ef81..1ae58e110f 100644 --- a/src/test/regress/expected/indexing.out +++ b/src/test/regress/expected/indexing.out @@ -50,11 +50,60 @@ 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 +create table idxpart1 partition of idxpart for values from (0) to (10) partition by range(a); +create table idxpart11 partition of idxpart1 for values from (0) to (10) partition by range(a); +create table idxpart2 partition of idxpart for values from (10) to (20); +insert into idxpart2 values(10),(10); -- not unique +create index concurrently on idxpart (a); -- partitioned +create index concurrently on idxpart1 (a); -- partitioned and partition +create index concurrently on idxpart11 (a); -- partitioned and partition, with no leaves +create index concurrently on idxpart2 (a); -- leaf +create unique index concurrently on idxpart (a); -- partitioned, unique failure +ERROR: could not create unique index "idxpart2_a_idx2" +DETAIL: Key (a)=(10) is duplicated. +\d idxpart + Partitioned table "public.idxpart" + Column | Type | Collation | Nullable | Default +--------+---------+-----------+----------+--------- + a | integer | | | + b | integer | | | + c | text | | | +Partition key: RANGE (a) +Indexes: + "idxpart_a_idx" btree (a) + "idxpart_a_idx1" UNIQUE, btree (a) INVALID +Number of partitions: 2 (Use \d+ to list them.) + +\d idxpart1 + Partitioned table "public.idxpart1" + Column | Type | Collation | Nullable | Default +--------+---------+-----------+----------+--------- + a | integer | | | + b | integer | | | + c | text | | | +Partition of: idxpart FOR VALUES FROM (0) TO (10) +Partition key: RANGE (a) +Indexes: + "idxpart1_a_idx" btree (a) + "idxpart1_a_idx1" btree (a) + "idxpart1_a_idx2" UNIQUE, btree (a) +Number of partitions: 1 (Use \d+ to list them.) + +\d idxpart2 + Table "public.idxpart2" + Column | Type | Collation | Nullable | Default +--------+---------+-----------+----------+--------- + a | integer | | | + b | integer | | | + c | text | | | +Partition of: idxpart FOR VALUES FROM (10) TO (20) +Indexes: + "idxpart2_a_idx" btree (a) + "idxpart2_a_idx1" btree (a) + "idxpart2_a_idx2" UNIQUE, btree (a) INVALID + drop table idxpart; -- Verify bugfix with query on indexed partitioned table with no partitions -- https://postgr.es/m/[email protected] diff --git a/src/test/regress/sql/indexing.sql b/src/test/regress/sql/indexing.sql index 35d159f41b..d908ee6d17 100644 --- a/src/test/regress/sql/indexing.sql +++ b/src/test/regress/sql/indexing.sql @@ -29,10 +29,20 @@ 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); +create table idxpart1 partition of idxpart for values from (0) to (10) partition by range(a); +create table idxpart11 partition of idxpart1 for values from (0) to (10) partition by range(a); +create table idxpart2 partition of idxpart for values from (10) to (20); +insert into idxpart2 values(10),(10); -- not unique +create index concurrently on idxpart (a); -- partitioned +create index concurrently on idxpart1 (a); -- partitioned and partition +create index concurrently on idxpart11 (a); -- partitioned and partition, with no leaves +create index concurrently on idxpart2 (a); -- leaf +create unique index concurrently on idxpart (a); -- partitioned, unique failure +\d idxpart +\d idxpart1 +\d idxpart2 drop table idxpart; -- Verify bugfix with query on indexed partitioned table with no partitions -- 2.17.0 --hYooF8G/hrfVAmum Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v5-0003-Implement-CLUSTER-of-partitioned-table.patch" ^ permalink raw reply [nested|flat] 3+ messages in thread
* [PATCH] Teach planner to further optimize sort in distinct @ 2023-01-17 19:27 Ankit Kumar Pandey <[email protected]> 0 siblings, 1 reply; 3+ messages in thread From: Ankit Kumar Pandey @ 2023-01-17 19:27 UTC (permalink / raw) To: pghackers <[email protected]>; David Rowley <[email protected]> Hi, this is extension of `teach planner to evaluate multiple windows in the optimal order` work applied to distinct operation. Based on discussions before (https://www.postgresql.org/message-id/flat/CAApHDvr7rSCVXzGfVa1L9pLpkKj6-s8NynK8o%2B98X9sKjejnQQ%40m...) , > All I imagine you need to do for it > is to invent a function in pathkeys.c which is along the lines of what > pathkeys_count_contained_in() does, but returns a List of pathkeys > which are in keys1 but not in keys2 and NIL if keys2 has a pathkey > that does not exist as a pathkey in keys1. In > create_final_distinct_paths(), you can then perform an incremental > sort on any input_path which has a non-empty return list and in > create_incremental_sort_path(), you'll pass presorted_keys as the > number of pathkeys in the path, and the required pathkeys the > input_path->pathkeys + the pathkeys returned from the new function. There is bit confusion in wording here: "returns a List of pathkeys which are in keys1 but not in keys2 and NIL if keys2 has a pathkey that does not exist as a pathkey in keys1." You mean extract common keys without ordering right? Example: keys1 = (a,b,c), keys2 = (b,a) returns (a,b) and keys1 = (a,b,c), keys = (d) returns = () which translates to needed_pathkeys = (a,b,c) = key2 input_pathkeys = (b,a) key1 returns (b,a) = common_keys new needed_pathkeys = unique(common_keys + old needed_pathkeys) => new needed_pathkeys = (b,a,c) The new needed_pathkeys matches input_pathkeys. This is what I implemented in the patch. The patched version yields the following plans: set enable_hashagg=0; set enable_seqscan=0; explain (costs off) select distinct relname,relkind,count(*) over (partition by relkind) from pg_Class; QUERY PLAN --------------------------------------------------------- Unique -> Incremental Sort Sort Key: relkind, relname, (count(*) OVER (?)) Presorted Key: relkind -> WindowAgg -> Sort Sort Key: relkind -> Seq Scan on pg_class (8 rows) explain (costs off) select distinct a, b, count(*) over (partition by b, a) from abcd; QUERY PLAN -------------------------------------------------------- Unique -> Incremental Sort Sort Key: b, a, (count(*) OVER (?)) Presorted Key: b, a -> WindowAgg -> Incremental Sort Sort Key: b, a Presorted Key: b -> Index Scan using b_idx on abcd (9 rows) explain (costs off) select distinct a, b, count(*) over (partition by c, d) from abcd; QUERY PLAN -------------------------------------------------------- Unique -> Sort Sort Key: a, b, (count(*) OVER (?)) -> WindowAgg -> Incremental Sort Sort Key: c, d Presorted Key: c -> Index Scan using c_idx on abcd (8 rows) Issue with index path still remains as pathkeys get purged by truncate_useless_pathkeys and hence are not available in create_final_distinct_paths for the above optimizations. I have attached a patch for the reference. Thanks, Ankit Attachments: [text/x-patch] distinct_sort_optimizations.patch (2.7K, ../../[email protected]/2-distinct_sort_optimizations.patch) download | inline diff: diff --git a/src/backend/optimizer/path/pathkeys.c b/src/backend/optimizer/path/pathkeys.c index 609df93dc9..13f6006577 100644 --- a/src/backend/optimizer/path/pathkeys.c +++ b/src/backend/optimizer/path/pathkeys.c @@ -1968,3 +1968,32 @@ has_useful_pathkeys(PlannerInfo *root, RelOptInfo *rel) return true; /* might be able to use them for ordering */ return false; /* definitely useless */ } + +/* + * extract_common_pathkeys + * returns a List of pathkeys + * which are in keys1 but not in keys2 and NIL if keys2 has a pathkey + * that does not exist as a pathkey in keys1 + */ +List * +extract_common_pathkeys(List* keys1, List *keys2) +{ + List *new_pk = NIL; + ListCell *l1; + ListCell *l2; + foreach(l1, keys1) + { + PathKey *pathkey1 = (PathKey *) lfirst(l1); + foreach(l2, keys2) + { + PathKey *pathkey2 = (PathKey *) lfirst(l2); + if (pathkey1 == pathkey2) + { + new_pk = lappend(new_pk, pathkey1); + break; + } + } + } + return new_pk; + +} \ No newline at end of file diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c index 044fb24666..1802d28e75 100644 --- a/src/backend/optimizer/plan/planner.c +++ b/src/backend/optimizer/plan/planner.c @@ -4844,11 +4844,28 @@ create_final_distinct_paths(PlannerInfo *root, RelOptInfo *input_rel, Path *sorted_path; bool is_sorted; int presorted_keys; + List *common_keys; is_sorted = pathkeys_count_contained_in(needed_pathkeys, input_path->pathkeys, &presorted_keys); + /* + * Check if there are common pathkeys (regardless of ordering) + */ + common_keys = extract_common_pathkeys(input_path->pathkeys, needed_pathkeys); + + if (common_keys) + { + /* + * Now that we have common keys, we can add these to path + */ + needed_pathkeys = list_concat_unique(common_keys, needed_pathkeys); + is_sorted = pathkeys_count_contained_in(needed_pathkeys, + input_path->pathkeys, + &presorted_keys); + } + if (is_sorted) sorted_path = input_path; else diff --git a/src/include/optimizer/paths.h b/src/include/optimizer/paths.h index 65a3c35611..b1b700e067 100644 --- a/src/include/optimizer/paths.h +++ b/src/include/optimizer/paths.h @@ -248,6 +248,7 @@ extern List *truncate_useless_pathkeys(PlannerInfo *root, RelOptInfo *rel, List *pathkeys); extern bool has_useful_pathkeys(PlannerInfo *root, RelOptInfo *rel); +extern List *extract_common_pathkeys(List* keys1, List *keys2); extern List *append_pathkeys(List *target, List *source); extern PathKey *make_canonical_pathkey(PlannerInfo *root, EquivalenceClass *eclass, Oid opfamily, ^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: [PATCH] Teach planner to further optimize sort in distinct @ 2023-01-19 13:19 David Rowley <[email protected]> parent: Ankit Kumar Pandey <[email protected]> 0 siblings, 0 replies; 3+ messages in thread From: David Rowley @ 2023-01-19 13:19 UTC (permalink / raw) To: Ankit Kumar Pandey <[email protected]>; +Cc: pghackers <[email protected]> On Wed, 18 Jan 2023 at 08:27, Ankit Kumar Pandey <[email protected]> wrote: > There is bit confusion in wording here: > > "returns a List of pathkeys > which are in keys1 but not in keys2 and NIL if keys2 has a pathkey > that does not exist as a pathkey in keys1." > > You mean extract common keys without ordering right? I think you should write a function like: bool pathkeys_count_contained_in_unordered(List *keys1, List *keys2, List **reorderedkeys, int *n_common) which works very similarly to pathkeys_count_contained_in, but populates *reorderedkeys so it contains all of the keys in keys1, but put the matching ones in the same order as they are in keys2. If you find a keys2 that does not exist in keys1 then just add the additional unmatched keys1 keys to *reorderedkeys. Set *n_common to the number of common keys excluding any that come after a key2 key that does not exist as a key1 key. You can just switch to using that function in create_final_distinct_paths(). You'll need to consider if the query is a DISTINCT ON query and not try the unordered version of the function in that case. I also just noticed that in build_index_paths() we'll leave the index path's pathkeys empty if we deem the pathkeys as useless. I'm not sure what the repercussions of setting those to the return value of build_index_pathkeys() if useful_pathkeys is otherwise empty. It's possible that truncate_useless_pathkeys() needs to be modified to check if the pathkeys might be useful for DISTINCT, but now that I see we don't populate the IndexPath's pathkeys when we deem them not useful makes me wonder if this entire patch is a good idea. When I thought about it I assumed that we always set IndexPath's pathkeys to whatever (if any) sort order that the index provides. David ^ permalink raw reply [nested|flat] 3+ messages in thread
end of thread, other threads:[~2023-01-19 13:19 UTC | newest] Thread overview: 3+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-06-06 22:42 [PATCH v5 2/3] Allow CREATE INDEX CONCURRENTLY on partitioned table Justin Pryzby <[email protected]> 2023-01-17 19:27 [PATCH] Teach planner to further optimize sort in distinct Ankit Kumar Pandey <[email protected]> 2023-01-19 13:19 ` Re: [PATCH] Teach planner to further optimize sort in distinct David Rowley <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox