public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v5 2/3] Allow CREATE INDEX CONCURRENTLY on partitioned table 2+ messages / 2 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; 2+ 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] 2+ messages in thread
* Re: Sort optimizations: Making in-memory sort cache-aware @ 2023-02-11 20:29 Andres Freund <[email protected]> 0 siblings, 0 replies; 2+ messages in thread From: Andres Freund @ 2023-02-11 20:29 UTC (permalink / raw) To: Ankit Kumar Pandey <[email protected]>; +Cc: pghackers <[email protected]>; David Rowley <[email protected]> Hi, On 2023-02-11 17:49:02 +0530, Ankit Kumar Pandey wrote: > 2. Frequent cache misses > > Issue #1 is being looked in separate patch. I am currently looking at #2. > > Possible solution was to batch tuples into groups (which can fit into L3 > cache) before pushing them to sort function. > > After looking at different papers on this (multi-Quicksort, memory-tuned > quicksort, Samplesort and various distributed sorts), although they look > promising (especially samplesort), I would like to get more inputs as > changes look bit too steep and may or may not be in of scope of solving > actual problem in hand. > > Please let me know your opinions, do we really need to re-look at quicksort > for this use-case or we can perform optimization without major change in > core sorting algorithm? Are we are open for trying new algorithms for sort? I think it'll require some experimentation to know what we can and should do. Clearly we're not going to do anything fundamental if the gains are a few percent. But it's not hard to imagine that the gains will be substantially larger. I believe that a significant part of the reason we have low cache hit ratios once the input gets larger, is that we kind of break the fundamental benefit of qsort: The reason quicksort is a good sorting algorithm, despite plenty downsides, is that it has pretty decent locality, due to its divide and conquer approach. However, tuplesort.c completely breaks that for > 1 column sorts. While spatial locality for accesses to the ->memtuples array is decent during sorting, due to qsort's subdividing of the problem, the locality for access to the tuples is *awful*. The memtuples array is reordered while sorting, but the tuples themselves aren't. Unless the input data is vaguely presorted, the access pattern for the tuples has practically zero locality. The only reason that doesn't completely kill us is that SortTuple contains datum1 inline and that abbreviated keys reduce the cost of by-reference datums in the first column. There are things we could do to improve upon this that don't require swapping out our sorting implementation wholesale. One idea is to keep track of the distinctness of the first column sorted and to behave differently if it's significantly lower than the number of to be sorted tuples. E.g. by doing a first sort solely on the first column, then reorder the MinimalTuples in memory, and then continue normally. There's two main problems with that idea: 1) It's hard to re-order the tuples in memory, without needing substantial amounts of additional memory 2) If the second column also is not very distinct, it doesn't buy you much, if anything. But it might provide sufficient benefits regardless. And a naive version, requiring additional memory, should be quick to hack up. I have *not* looked at a whole lot of papers of cache optimized sorts, and the little I did was not recently. Partially because I am not sure that they are that applicable to our scenarios: Most sorting papers don't discuss variable-width data, nor a substantial amount of cache-polluting work while gathering the data that needs to be sorted. I think: > Possible solution was to batch tuples into groups (which can fit into L3 > cache) before pushing them to sort function. is the most general solution to the issue outlined above. I wouldn't try to implement this via a full new sorting algorithm though. My suggestion would be to collect a roughly ~L3 sized amount of tuples, sort just those using the existing code, allocate new memory for all the corresponding MinimalTuples in one allocation, and copy the MinimalTuples into that, obviously in ->memtuples order. Even if we just use the existing code for the overall sort after that, I'd expect that to yield noticable benefits. It's very likely we can do better than just doing a plain sort of everything after that. You effectively end up with a bounded number of pre-sorted blocks, so the most obvious thing to try is to build a heap of those blocks and effectively do a heapsort between the presorted blocks. A related, but separate, improvement is to reduce / remove the memory allocation overhead. The switch to GenerationContext helped some, but still leaves a bunch of overhead. And it's not used for bounded sorts right now. We don't palloc/pfree individual tuples during a normal sorts, but we do have some, for bounded sorts. I think with a reasonable amount of work we could avoid that for all tuples in ->tuplecontext. And switch to a trivial bump allocator, getting rid of all allocator overhead. The biggest source of individual pfree()s in the bounded case is that we unconditionally copy the tuple into base->tuplecontext during puttuple. Even though quite likely we'll immediately free it in the "case TSS_BOUNDED" block. We could instead pre-check that the tuple won't immediately be discarded, before copying it into tuplecontext. Only in the TSS_BOUNDED, case, of course. I think we also can replace the individual freeing of tuples in tuplesort_heap_replace_top(), by allowing a number of dead tuples to accumulate (up to work_mem/2 maybe), and then copying the still living tuples into new memory context, freeing the old one. While that doesn't sound cheap, in bounded sorts the number of tuples commonly is quite limited, the pre-check before copying the tuple will prevent this from occurring too often, the copy will result in higher locality, and, most importantly, the reduced palloc() overhead (~25% or so with aset.c) will result in a considerably higher cache hit ratio / lower memory usage. Greetings, Andres Freund ^ permalink raw reply [nested|flat] 2+ messages in thread
end of thread, other threads:[~2023-02-11 20:29 UTC | newest] Thread overview: 2+ 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-02-11 20:29 Re: Sort optimizations: Making in-memory sort cache-aware Andres Freund <[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