public inbox for [email protected]help / color / mirror / Atom feed
[PATCH] Allow CREATE INDEX CONCURRENTLY on partitioned table 7+ messages / 5 participants [nested] [flat]
* [PATCH] Allow CREATE INDEX CONCURRENTLY on partitioned table @ 2020-06-06 22:42 Justin Pryzby <[email protected]> 0 siblings, 0 replies; 7+ 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/ddl.sgml | 4 +- doc/src/sgml/ref/create_index.sgml | 9 -- src/backend/commands/indexcmds.c | 214 +++++++++++++++++++------ src/include/catalog/index.h | 1 + src/test/regress/expected/indexing.out | 136 +++++++++++++++- src/test/regress/sql/indexing.sql | 26 ++- 6 files changed, 320 insertions(+), 70 deletions(-) diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml index 03c01937094..fd56e21ef49 100644 --- a/doc/src/sgml/ddl.sgml +++ b/doc/src/sgml/ddl.sgml @@ -4131,9 +4131,7 @@ ALTER TABLE measurement ATTACH PARTITION measurement_y2008m02 so that they are applied automatically to the entire hierarchy. This is very convenient, as not only will the existing partitions become indexed, but - also any partitions that are created in the future will. One limitation is - that it's not possible to use the <literal>CONCURRENTLY</literal> - qualifier when creating such a partitioned index. To avoid long lock + also any partitions that are created in the future will. To avoid long lock times, it is possible to use <command>CREATE INDEX ON ONLY</command> the partitioned table; such an index is marked invalid, and the partitions do not get the index applied automatically. The indexes on partitions can diff --git a/doc/src/sgml/ref/create_index.sgml b/doc/src/sgml/ref/create_index.sgml index 40986aa502f..fc8cda655f0 100644 --- a/doc/src/sgml/ref/create_index.sgml +++ b/doc/src/sgml/ref/create_index.sgml @@ -692,15 +692,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 91cee27743d..bb98e745267 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -71,6 +71,7 @@ /* non-export function prototypes */ +static void reindex_invalid_child_indexes(Oid indexRelationId); static bool CompareOpclassOptions(Datum *opts1, Datum *opts2, int natts); static void CheckPredicate(Expr *predicate); static void ComputeIndexAttrs(IndexInfo *indexInfo, @@ -104,7 +105,9 @@ static void reindex_error_callback(void *arg); static void ReindexPartitions(Oid relid, ReindexParams *params, bool isTopLevel); static void ReindexMultipleInternal(List *relids, - ReindexParams *params); + ReindexParams *params, + Oid parent, + int npart); static bool ReindexRelationConcurrently(Oid relationOid, ReindexParams *params); static void update_relispartition(Oid relationId, bool newval); @@ -697,17 +700,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), @@ -1147,6 +1139,11 @@ DefineIndex(Oid relationId, if (pd->nparts != 0) flags |= INDEX_CREATE_INVALID; } + else if (concurrent && OidIsValid(parentIndexId)) + { + /* If concurrent, initially build index partitions as "invalid" */ + flags |= INDEX_CREATE_INVALID; + } if (stmt->deferrable) constr_flags |= INDEX_CONSTR_CREATE_DEFERRABLE; @@ -1212,17 +1209,30 @@ DefineIndex(Oid relationId, partdesc = RelationGetPartitionDesc(rel, true); if ((!stmt->relation || stmt->relation->inh) && partdesc->nparts > 0) { + /* + * 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); int nparts = partdesc->nparts; Oid *part_oids = palloc_array(Oid, nparts); bool invalidate_parent = false; Relation parentIndex; TupleDesc parentDesc; + char *relname; pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITIONS_TOTAL, nparts); /* Make a local copy of partdesc->oids[], just for safety */ memcpy(part_oids, partdesc->oids, sizeof(Oid) * nparts); + parentDesc = CreateTupleDescCopy(RelationGetDescr(rel)); + relname = pstrdup(RelationGetRelationName(rel)); + table_close(rel, NoLock); + MemoryContextSwitchTo(oldcontext); /* * We'll need an IndexInfo describing the parent index. The one @@ -1235,8 +1245,6 @@ DefineIndex(Oid relationId, parentIndex = index_open(indexRelationId, lockmode); indexInfo = BuildIndexInfo(parentIndex); - parentDesc = RelationGetDescr(rel); - /* * For each partition, scan all existing indexes; if one matches * our index definition and is not already attached to some other @@ -1276,9 +1284,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))); AtEOXact_GUC(false, child_save_nestlevel); SetUserIdAndSecContext(child_save_userid, @@ -1364,10 +1372,14 @@ DefineIndex(Oid relationId, */ if (!found) { - IndexStmt *childStmt = copyObject(stmt); + IndexStmt *childStmt; bool found_whole_row; ListCell *lc; + oldcontext = MemoryContextSwitchTo(ind_context); + childStmt = copyObject(stmt); + MemoryContextSwitchTo(oldcontext); + /* * We can't use the same index name for the child index, * so clear idxname to let the recursive invocation choose @@ -1426,14 +1438,24 @@ DefineIndex(Oid relationId, createdConstraintId, is_alter_table, check_rights, check_not_in_use, skip_build, quiet); + SetUserIdAndSecContext(child_save_userid, child_save_sec_context); + if (concurrent) + { + PopActiveSnapshot(); + 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); } + pfree(relname); index_close(parentIndex, lockmode); @@ -1443,46 +1465,40 @@ DefineIndex(Oid relationId, * invalid, this is incorrect, so update our row to invalid too. */ if (invalidate_parent) - { - Relation pg_index = table_open(IndexRelationId, RowExclusiveLock); - HeapTuple tup, - newtup; - - tup = SearchSysCache1(INDEXRELID, - ObjectIdGetDatum(indexRelationId)); - if (!HeapTupleIsValid(tup)) - elog(ERROR, "cache lookup failed for index %u", - indexRelationId); - newtup = heap_copytuple(tup); - ((Form_pg_index) GETSTRUCT(newtup))->indisvalid = false; - CatalogTupleUpdate(pg_index, &tup->t_self, newtup); - ReleaseSysCache(tup); - table_close(pg_index, RowExclusiveLock); - heap_freetuple(newtup); - } - } + index_set_state_flags(indexRelationId, INDEX_DROP_CLEAR_VALID); + } else + table_close(rel, NoLock); /* * Indexes on partitioned tables are not themselves built, so we're * done here. */ + AtEOXact_GUC(false, root_save_nestlevel); SetUserIdAndSecContext(root_save_userid, root_save_sec_context); - table_close(rel, NoLock); + if (!OidIsValid(parentIndexId)) + { + if (concurrent) + reindex_invalid_child_indexes(indexRelationId); + pgstat_progress_end_command(); + } + return address; } AtEOXact_GUC(false, root_save_nestlevel); SetUserIdAndSecContext(root_save_userid, root_save_sec_context); - if (!concurrent) + if (!concurrent || OidIsValid(parentIndexId)) { - /* Close the heap and we're done, in the non-concurrent case */ - table_close(rel, NoLock); + /* + * We're done if this is the top-level index, + * or the catalog-only phase of a partition built concurrently + */ - /* If this is the top-level index, we're done. */ + table_close(rel, NoLock); if (!OidIsValid(parentIndexId)) pgstat_progress_end_command(); @@ -1695,6 +1711,70 @@ DefineIndex(Oid relationId, return address; } +/* Reindex invalid child indexes created earlier */ +static void +reindex_invalid_child_indexes(Oid indexRelationId) +{ + ListCell *lc; + List *parentindexes = NIL; + int npart = 1; + ReindexParams params = { + .options = REINDEXOPT_CONCURRENTLY | REINDEXOPT_REPORT_CREATE_PART + }; + + MemoryContext ind_context = AllocSetContextCreate(PortalContext, "CREATE INDEX", + ALLOCSET_DEFAULT_SIZES); + MemoryContext oldcontext; + List *childs = find_all_inheritors(indexRelationId, ShareLock, NULL); + List *partitions = NIL; + + PreventInTransactionBlock(true, "REINDEX INDEX"); + + foreach (lc, childs) + { + Oid partoid = lfirst_oid(lc); + char partkind = get_rel_relkind(partoid); + + if (partkind == RELKIND_PARTITIONED_INDEX) + { + oldcontext = MemoryContextSwitchTo(ind_context); + parentindexes = lappend_oid(parentindexes, partoid); + MemoryContextSwitchTo(oldcontext); + } + + // npart++ ? + if (!RELKIND_HAS_STORAGE(partkind) || get_index_isvalid(partoid)) + continue; + + /* Save partition OID */ + oldcontext = MemoryContextSwitchTo(ind_context); + partitions = lappend_oid(partitions, partoid); + MemoryContextSwitchTo(oldcontext); + } + + /* + * Process each partition listed in a separate transaction. Note that + * this commits and then starts a new transaction immediately. + * XXX: since this is done in 2*N transactions, it could just as well + * call ReindexRelationConcurrently directly + */ + ReindexMultipleInternal(partitions, ¶ms, indexRelationId, npart); + + /* + * 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). This must be done only + * while holding a lock which precludes adding partitions. + * See also: validatePartitionedIndex(). + */ + foreach (lc, parentindexes) + { + Oid partoid = lfirst_oid(lc); + index_set_state_flags(partoid, INDEX_CREATE_SET_READY); + CommandCounterIncrement(); + index_set_state_flags(partoid, INDEX_CREATE_SET_VALID); + } +} /* * CheckMutability @@ -3084,7 +3164,7 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind, * Process each relation listed in a separate transaction. Note that this * commits and then starts a new transaction immediately. */ - ReindexMultipleInternal(relids, params); + ReindexMultipleInternal(relids, params, InvalidOid, 0); MemoryContextDelete(private_context); } @@ -3120,6 +3200,7 @@ ReindexPartitions(Oid relid, ReindexParams *params, bool isTopLevel) char relkind = get_rel_relkind(relid); char *relname = get_rel_name(relid); char *relnamespace = get_namespace_name(get_rel_namespace(relid)); + int npart = 1; MemoryContext reindex_context; List *inhoids; ListCell *lc; @@ -3190,7 +3271,7 @@ ReindexPartitions(Oid relid, ReindexParams *params, bool isTopLevel) * Process each partition listed in a separate transaction. Note that * this commits and then starts a new transaction immediately. */ - ReindexMultipleInternal(partitions, params); + ReindexMultipleInternal(partitions, params, relid, npart); /* * Clean up working storage --- note we must do this after @@ -3208,7 +3289,7 @@ ReindexPartitions(Oid relid, ReindexParams *params, bool isTopLevel) * and starts a new transaction when finished. */ static void -ReindexMultipleInternal(List *relids, ReindexParams *params) +ReindexMultipleInternal(List *relids, ReindexParams *params, Oid parent, int npart) { ListCell *l; @@ -3302,6 +3383,29 @@ ReindexMultipleInternal(List *relids, ReindexParams *params) } CommitTransactionCommand(); + + if (params->options & REINDEXOPT_REPORT_CREATE_PART) + { + /* + * Update pgstat progress report to indicate that create index on + * partition was finished. + */ + const int progress_cols[] = { + PROGRESS_CREATEIDX_COMMAND, + PROGRESS_CREATEIDX_INDEX_OID, + PROGRESS_CREATEIDX_PHASE, + PROGRESS_CREATEIDX_PARTITIONS_DONE + }; + const int64 progress_vals[] = { + PROGRESS_CREATEIDX_COMMAND_CREATE_CONCURRENTLY, + parent, + PROGRESS_CREATEIDX_PHASE_VALIDATE_IDXSCAN, + npart++ + }; + + pgstat_progress_update_multi_param(4, progress_cols, progress_vals); + } + } StartTransactionCommand(); @@ -3697,7 +3801,9 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params) if (indexRel->rd_rel->relpersistence == RELPERSISTENCE_TEMP) elog(ERROR, "cannot reindex a temporary table concurrently"); - pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX, + /* Don't overwrite CREATE INDEX command */ + if (!(params->options & REINDEXOPT_REPORT_CREATE_PART)) + pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX, idx->tableId); progress_vals[0] = PROGRESS_CREATEIDX_COMMAND_REINDEX_CONCURRENTLY; @@ -3857,9 +3963,11 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params) /* * Update progress for the index to build, with the correct parent - * table involved. + * table involved. Don't overwrite CREATE INDEX command. */ - pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX, newidx->tableId); + if (!(params->options & REINDEXOPT_REPORT_CREATE_PART)) + pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX, newidx->tableId); + progress_vals[0] = PROGRESS_CREATEIDX_COMMAND_REINDEX_CONCURRENTLY; progress_vals[1] = PROGRESS_CREATEIDX_PHASE_BUILD; progress_vals[2] = newidx->indexId; @@ -3921,10 +4029,12 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params) /* * Update progress for the index to build, with the correct parent - * table involved. + * table involved. Don't overwrite CREATE INDEX command. */ - pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX, + if (!(params->options & REINDEXOPT_REPORT_CREATE_PART)) + pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX, newidx->tableId); + progress_vals[0] = PROGRESS_CREATEIDX_COMMAND_REINDEX_CONCURRENTLY; progress_vals[1] = PROGRESS_CREATEIDX_PHASE_VALIDATE_IDXSCAN; progress_vals[2] = newidx->indexId; @@ -4159,7 +4269,9 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params) MemoryContextDelete(private_context); - pgstat_progress_end_command(); + /* Don't overwrite CREATE INDEX command. */ + if (!(params->options & REINDEXOPT_REPORT_CREATE_PART)) + pgstat_progress_end_command(); return true; } diff --git a/src/include/catalog/index.h b/src/include/catalog/index.h index 91c28868d47..4142894c230 100644 --- a/src/include/catalog/index.h +++ b/src/include/catalog/index.h @@ -42,6 +42,7 @@ typedef struct ReindexParams #define REINDEXOPT_REPORT_PROGRESS 0x02 /* report pgstat progress */ #define REINDEXOPT_MISSING_OK 0x04 /* skip missing relations */ #define REINDEXOPT_CONCURRENTLY 0x08 /* concurrent mode */ +#define REINDEXOPT_REPORT_CREATE_PART 0x10 /* report that index was created for partition */ /* state info for validate_index bulkdelete callback */ typedef struct ValidateIndexState diff --git a/src/test/regress/expected/indexing.out b/src/test/regress/expected/indexing.out index 1bdd430f063..6b2320244b8 100644 --- a/src/test/regress/expected/indexing.out +++ b/src/test/regress/expected/indexing.out @@ -50,11 +50,139 @@ 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 idxpart111 partition of idxpart11 default partition by range(a); +create table idxpart1111 partition of idxpart111 default partition by range(a); +create table idxpart2 partition of idxpart for values from (10) to (20); +create table idxpart3 partition of idxpart for values from (30) to (40) partition by range(a); +create table idxpart31 partition of idxpart3 default; +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_ccnew" +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: 3 (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) INVALID +Number of partitions: 1 (Use \d+ to list them.) + +\d idxpart11 + Partitioned table "public.idxpart11" + Column | Type | Collation | Nullable | Default +--------+---------+-----------+----------+--------- + a | integer | | | + b | integer | | | + c | text | | | +Partition of: idxpart1 FOR VALUES FROM (0) TO (10) +Partition key: RANGE (a) +Indexes: + "idxpart11_a_idx" btree (a) + "idxpart11_a_idx1" btree (a) + "idxpart11_a_idx2" btree (a) + "idxpart11_a_idx3" UNIQUE, btree (a) INVALID +Number of partitions: 1 (Use \d+ to list them.) + +\d idxpart111 + Partitioned table "public.idxpart111" + Column | Type | Collation | Nullable | Default +--------+---------+-----------+----------+--------- + a | integer | | | + b | integer | | | + c | text | | | +Partition of: idxpart11 DEFAULT +Partition key: RANGE (a) +Indexes: + "idxpart111_a_idx" btree (a) + "idxpart111_a_idx1" btree (a) + "idxpart111_a_idx2" btree (a) + "idxpart111_a_idx3" UNIQUE, btree (a) INVALID +Number of partitions: 1 (Use \d+ to list them.) + +\d idxpart1111 + Partitioned table "public.idxpart1111" + Column | Type | Collation | Nullable | Default +--------+---------+-----------+----------+--------- + a | integer | | | + b | integer | | | + c | text | | | +Partition of: idxpart111 DEFAULT +Partition key: RANGE (a) +Indexes: + "idxpart1111_a_idx" btree (a) + "idxpart1111_a_idx1" btree (a) + "idxpart1111_a_idx2" btree (a) + "idxpart1111_a_idx3" UNIQUE, btree (a) INVALID +Number of partitions: 0 + +\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 + "idxpart2_a_idx2_ccnew" UNIQUE, btree (a) INVALID + +\d idxpart3 + Partitioned table "public.idxpart3" + Column | Type | Collation | Nullable | Default +--------+---------+-----------+----------+--------- + a | integer | | | + b | integer | | | + c | text | | | +Partition of: idxpart FOR VALUES FROM (30) TO (40) +Partition key: RANGE (a) +Indexes: + "idxpart3_a_idx" btree (a) + "idxpart3_a_idx1" UNIQUE, btree (a) INVALID +Number of partitions: 1 (Use \d+ to list them.) + +\d idxpart31 + Table "public.idxpart31" + Column | Type | Collation | Nullable | Default +--------+---------+-----------+----------+--------- + a | integer | | | + b | integer | | | + c | text | | | +Partition of: idxpart3 DEFAULT +Indexes: + "idxpart31_a_idx" btree (a) + "idxpart31_a_idx1" 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 429120e7104..14e0513386c 100644 --- a/src/test/regress/sql/indexing.sql +++ b/src/test/regress/sql/indexing.sql @@ -29,10 +29,30 @@ 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 idxpart111 partition of idxpart11 default partition by range(a); +create table idxpart1111 partition of idxpart111 default partition by range(a); +create table idxpart2 partition of idxpart for values from (10) to (20); +create table idxpart3 partition of idxpart for values from (30) to (40) partition by range(a); +create table idxpart31 partition of idxpart3 default; + +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 idxpart11 +\d idxpart111 +\d idxpart1111 +\d idxpart2 +\d idxpart3 +\d idxpart31 drop table idxpart; -- Verify bugfix with query on indexed partitioned table with no partitions -- 2.25.1 --aPdhxNJGSeOG9wFI-- ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Improve error message for ICU libraries if pkg-config is absent @ 2024-08-09 08:59 Heikki Linnakangas <[email protected]> 0 siblings, 2 replies; 7+ messages in thread From: Heikki Linnakangas @ 2024-08-09 08:59 UTC (permalink / raw) To: Michael Banck <[email protected]>; pgsql-hackers; +Cc: Holger Jakobs <[email protected]> On 09/08/2024 11:16, Michael Banck wrote: > Hi, > > Holger Jacobs complained in pgsql-admin that in v17, if you have the ICU > development libraries installed but not pkg-config, you get a somewhat > unhelpful error message about ICU not being present: > > |checking for pkg-config... no > |checking whether to build with ICU support... yes > |checking for icu-uc icu-i18n... no > |configure: error: ICU library not found > |If you have ICU already installed, see config.log for details on the > |failure. It is possible the compiler isn't looking in the proper directory. > |Use --without-icu to disable ICU support. > > The attached patch improves things to that: > > |checking for pkg-config... no > |checking whether to build with ICU support... yes > |configure: error: ICU library not found > |The ICU library could not be found because pkg-config is not available, see > |config.log for details on the failure. If ICU is installed, the variables > |ICU_CFLAGS and ICU_LIBS can be set explicitly in this case, or use > |--without-icu to disable ICU support. Hmm, if that's a good change, shouldn't we do it for all libraries that we try to find with pkg-config? I'm surprised the pkg.m4 module doesn't provide a nice error message already if pkg-config is not found. I can see some messages like that in pkg.m4. Why are they not printed? > Also, this should be backpatched to v17 if accepted. Did something change here in v17? -- Heikki Linnakangas Neon (https://neon.tech) ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Improve error message for ICU libraries if pkg-config is absent @ 2024-08-09 09:44 Michael Banck <[email protected]> parent: Heikki Linnakangas <[email protected]> 1 sibling, 1 reply; 7+ messages in thread From: Michael Banck @ 2024-08-09 09:44 UTC (permalink / raw) To: Heikki Linnakangas <[email protected]>; +Cc: pgsql-hackers; Holger Jakobs <[email protected]>; Jeff Davis <[email protected]> Hi, adding Jeff to CC as he changed the way ICU configure detection was done in fcb21b3. On Fri, Aug 09, 2024 at 11:59:12AM +0300, Heikki Linnakangas wrote: > On 09/08/2024 11:16, Michael Banck wrote: > > Hi, > > > > Holger Jacobs complained in pgsql-admin that in v17, if you have the ICU > > development libraries installed but not pkg-config, you get a somewhat > > unhelpful error message about ICU not being present: > > > > |checking for pkg-config... no > > |checking whether to build with ICU support... yes > > |checking for icu-uc icu-i18n... no > > |configure: error: ICU library not found > > |If you have ICU already installed, see config.log for details on the > > |failure. It is possible the compiler isn't looking in the proper directory. > > |Use --without-icu to disable ICU support. > > > > The attached patch improves things to that: > > > > |checking for pkg-config... no > > |checking whether to build with ICU support... yes > > |configure: error: ICU library not found > > |The ICU library could not be found because pkg-config is not available, see > > |config.log for details on the failure. If ICU is installed, the variables > > |ICU_CFLAGS and ICU_LIBS can be set explicitly in this case, or use > > |--without-icu to disable ICU support. > > Hmm, if that's a good change, shouldn't we do it for all libraries that we > try to find with pkg-config? Hrm, probably. I think the main difference is that libicu is checked by default (actually since v16, see below), but the others are not, so maybe it is less of a problem there? So I had a further look and the only other pkg-config checks seem to be xml2, lz4 and zstd. From those, lz4 and zstd do not have a custom AC_MSG_ERROR so there you get something more helpful like this: |checking for pkg-config... no [...] |checking whether to build with LZ4 support... yes |checking for liblz4... no |configure: error: in `/home/mbanck/repos/postgres/postgresql/build': |configure: error: The pkg-config script could not be found or is too old. Make sure it |is in your PATH or set the PKG_CONFIG environment variable to the full |path to pkg-config. | |Alternatively, you may set the environment variables LZ4_CFLAGS |and LZ4_LIBS to avoid the need to call pkg-config. |See the pkg-config man page for more details. | |To get pkg-config, see <http://pkg-config.freedesktop.org/;. |See `config.log' for more details The XML check sets the error as no-op because there is xml2-config which is usually used: | if test -z "$XML2_CONFIG" -a -n "$PKG_CONFIG"; then | PKG_CHECK_MODULES(XML2, [libxml-2.0 >= 2.6.23], | [have_libxml2_pkg_config=yes], [# do nothing]) [...] |if test "$with_libxml" = yes ; then | AC_CHECK_LIB(xml2, xmlSaveToBuffer, [], [AC_MSG_ERROR([library 'xml2' (version >= 2.6.23) is required for XML support])]) |fi So if both pkg-config and libxml2-dev are missing this results in: |checking for pkg-config... no [...] |checking whether to build with XML support... yes |checking for xml2-config... no [...] |checking for xmlSaveToBuffer in -lxml2... no |configure: error: library 'xml2' (version >= 2.6.23) is required for XML support Whereas if only pkg-config is missing, configure goes through fine: |checking for pkg-config... no [...] |checking whether to build with XML support... yes |checking for xml2-config... /usr/bin/xml2-config [...] |checking for xmlSaveToBuffer in -lxml2... yes So to summarize, I think the others are fine. > I'm surprised the pkg.m4 module doesn't provide a nice error message already > if pkg-config is not found. I can see some messages like that in pkg.m4. Why > are they not printed? > > > Also, this should be backpatched to v17 if accepted. > Did something change here in v17? I was mistaken, things changed in v16 when ICU was checked for by default and the explicit error message was added. Before, ICU behaved like LZ4/ZST now, i.e. if you added --with-icu explicitly you would get the error about pkg-config not being there. So maybe the better change is to just remove the explicit error message again and depend on PKG_CHECK_MODULES erroring out helpfully? The downside would be that the hint about specifying --without-icu to get around this would disappear, but I think somebody building from source can figure that out more easily than the subtle issue that pkg-config is not installed. This would lead to this: |checking for pkg-config... no |checking whether to build with ICU support... yes |checking for icu-uc icu-i18n... no |configure: error: in `/home/mbanck/repos/postgres/postgresql/build': |configure: error: The pkg-config script could not be found or is too old. Make sure it |is in your PATH or set the PKG_CONFIG environment variable to the full |path to pkg-config. | |Alternatively, you may set the environment variables ICU_CFLAGS |and ICU_LIBS to avoid the need to call pkg-config. |See the pkg-config man page for more details. | |To get pkg-config, see <http://pkg-config.freedesktop.org/;. |See `config.log' for more details I have attached a new patch for that. Additionally, going forward, v18+ could just mandate pkg-config to be installed, but I would assume this is not something we would want to change in the back branches. (I also haven't looked how Meson is handling this) Michael Attachments: [text/x-diff] v2-0001-Improve-configure-error-for-ICU-libraries-if-pkg-.patch (3.2K, ../../[email protected]/2-v2-0001-Improve-configure-error-for-ICU-libraries-if-pkg-.patch) download | inline diff: From 457bf49bd6bcfb15b8552031ac06e87b5a8b89f7 Mon Sep 17 00:00:00 2001 From: Michael Banck <[email protected]> Date: Fri, 9 Aug 2024 10:13:27 +0200 Subject: [PATCH v2] Improve configure error for ICU libraries if pkg-config is absent. If pkg-config is not installed, the ICU libraries cannot be found, but the custom configure error message did not mention this. This might lead to confusion about the actual problem. To improve this, remove the explicit error message and rely on PKG_CHECK_MODULES' generic error message. Reported-by: Holger Jakobs Discussion: https://www.postgresql.org/message-id/ccd579ed-4949-d3de-ab13-9e6456fd2caf%40jakobs.com --- configure | 30 ++++++++++++++++++++++-------- configure.ac | 6 +----- 2 files changed, 23 insertions(+), 13 deletions(-) diff --git a/configure b/configure index 4f3aa44756..14b7ae27e6 100755 --- a/configure +++ b/configure @@ -8153,17 +8153,31 @@ fi # Put the nasty error message in config.log where it belongs echo "$ICU_PKG_ERRORS" >&5 - as_fn_error $? "ICU library not found -If you have ICU already installed, see config.log for details on the -failure. It is possible the compiler isn't looking in the proper directory. -Use --without-icu to disable ICU support." "$LINENO" 5 + as_fn_error $? "Package requirements (icu-uc icu-i18n) were not met: + +$ICU_PKG_ERRORS + +Consider adjusting the PKG_CONFIG_PATH environment variable if you +installed software in a non-standard prefix. + +Alternatively, you may set the environment variables ICU_CFLAGS +and ICU_LIBS to avoid the need to call pkg-config. +See the pkg-config man page for more details." "$LINENO" 5 elif test $pkg_failed = untried; then { $as_echo "$as_me:${as_lineno-$LINENO}: result: no" >&5 $as_echo "no" >&6; } - as_fn_error $? "ICU library not found -If you have ICU already installed, see config.log for details on the -failure. It is possible the compiler isn't looking in the proper directory. -Use --without-icu to disable ICU support." "$LINENO" 5 + { { $as_echo "$as_me:${as_lineno-$LINENO}: error: in \`$ac_pwd':" >&5 +$as_echo "$as_me: error: in \`$ac_pwd':" >&2;} +as_fn_error $? "The pkg-config script could not be found or is too old. Make sure it +is in your PATH or set the PKG_CONFIG environment variable to the full +path to pkg-config. + +Alternatively, you may set the environment variables ICU_CFLAGS +and ICU_LIBS to avoid the need to call pkg-config. +See the pkg-config man page for more details. + +To get pkg-config, see <http://pkg-config.freedesktop.org/>. +See \`config.log' for more details" "$LINENO" 5; } else ICU_CFLAGS=$pkg_cv_ICU_CFLAGS ICU_LIBS=$pkg_cv_ICU_LIBS diff --git a/configure.ac b/configure.ac index 049bc01491..94e35da4f2 100644 --- a/configure.ac +++ b/configure.ac @@ -829,11 +829,7 @@ AC_MSG_RESULT([$with_icu]) AC_SUBST(with_icu) if test "$with_icu" = yes; then - PKG_CHECK_MODULES(ICU, icu-uc icu-i18n, [], - [AC_MSG_ERROR([ICU library not found -If you have ICU already installed, see config.log for details on the -failure. It is possible the compiler isn't looking in the proper directory. -Use --without-icu to disable ICU support.])]) + PKG_CHECK_MODULES(ICU, icu-uc icu-i18n) fi # -- 2.39.2 ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Improve error message for ICU libraries if pkg-config is absent @ 2024-08-13 20:13 Peter Eisentraut <[email protected]> parent: Heikki Linnakangas <[email protected]> 1 sibling, 0 replies; 7+ messages in thread From: Peter Eisentraut @ 2024-08-13 20:13 UTC (permalink / raw) To: Heikki Linnakangas <[email protected]>; Michael Banck <[email protected]>; pgsql-hackers; +Cc: Holger Jakobs <[email protected]> On 09.08.24 10:59, Heikki Linnakangas wrote: > On 09/08/2024 11:16, Michael Banck wrote: >> Hi, >> >> Holger Jacobs complained in pgsql-admin that in v17, if you have the ICU >> development libraries installed but not pkg-config, you get a somewhat >> unhelpful error message about ICU not being present: >> >> |checking for pkg-config... no >> |checking whether to build with ICU support... yes >> |checking for icu-uc icu-i18n... no >> |configure: error: ICU library not found >> |If you have ICU already installed, see config.log for details on the >> |failure. It is possible the compiler isn't looking in the proper >> directory. >> |Use --without-icu to disable ICU support. >> >> The attached patch improves things to that: >> >> |checking for pkg-config... no >> |checking whether to build with ICU support... yes >> |configure: error: ICU library not found >> |The ICU library could not be found because pkg-config is not >> available, see >> |config.log for details on the failure. If ICU is installed, the >> variables >> |ICU_CFLAGS and ICU_LIBS can be set explicitly in this case, or use >> |--without-icu to disable ICU support. > > Hmm, if that's a good change, shouldn't we do it for all libraries that > we try to find with pkg-config? > > I'm surprised the pkg.m4 module doesn't provide a nice error message > already if pkg-config is not found. I can see some messages like that in > pkg.m4. Why are they not printed? Because we override it with our own message. If we don't supply our own message, we get the built-in ones. Might be worth trying whether the built-in ones are better? (But they won't know about higher-level options like "--without-icu", so they won't be able to give suggestions like that.) ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Improve error message for ICU libraries if pkg-config is absent @ 2024-08-15 01:05 Jeff Davis <[email protected]> parent: Michael Banck <[email protected]> 0 siblings, 1 reply; 7+ messages in thread From: Jeff Davis @ 2024-08-15 01:05 UTC (permalink / raw) To: Michael Banck <[email protected]>; Heikki Linnakangas <[email protected]>; +Cc: pgsql-hackers; Holger Jakobs <[email protected]>; Jeff Davis <[email protected]> On Fri, 2024-08-09 at 11:44 +0200, Michael Banck wrote: > So maybe the better change is to just remove the explicit error > message > again and depend on PKG_CHECK_MODULES erroring out helpfully? The > downside would be that the hint about specifying --without-icu to get > around this would disappear, but I think somebody building from > source > can figure that out more easily than the subtle issue that pkg-config > is > not installed. That looks good to me. Does anyone have a different opinion? If not, I'll go ahead and commit (but not backport) this change. Regards, Jeff Davis ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Improve error message for ICU libraries if pkg-config is absent @ 2024-08-15 07:20 Michael Banck <[email protected]> parent: Jeff Davis <[email protected]> 0 siblings, 1 reply; 7+ messages in thread From: Michael Banck @ 2024-08-15 07:20 UTC (permalink / raw) To: Jeff Davis <[email protected]>; +Cc: Heikki Linnakangas <[email protected]>; pgsql-hackers; Holger Jakobs <[email protected]>; Jeff Davis <[email protected]> Hi, On Wed, Aug 14, 2024 at 06:05:19PM -0700, Jeff Davis wrote: > That looks good to me. Does anyone have a different opinion? If not, > I'll go ahead and commit (but not backport) this change. What is the rationale not to backpatch this? The error message changes, but we do not translate configure output, so is this a problem/project policy to never change configure output in the back-branches? If the change is too invasive, would something like the initial patch I suggested (i.e., in the absense of pkg-config, add a more targetted error message) be acceptable for the back-branches? Michael ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Improve error message for ICU libraries if pkg-config is absent @ 2024-08-15 13:22 Peter Eisentraut <[email protected]> parent: Michael Banck <[email protected]> 0 siblings, 0 replies; 7+ messages in thread From: Peter Eisentraut @ 2024-08-15 13:22 UTC (permalink / raw) To: Michael Banck <[email protected]>; Jeff Davis <[email protected]>; +Cc: Heikki Linnakangas <[email protected]>; pgsql-hackers; Holger Jakobs <[email protected]>; Jeff Davis <[email protected]> On 15.08.24 09:20, Michael Banck wrote: > On Wed, Aug 14, 2024 at 06:05:19PM -0700, Jeff Davis wrote: >> That looks good to me. Does anyone have a different opinion? If not, >> I'll go ahead and commit (but not backport) this change. > > What is the rationale not to backpatch this? The error message changes, > but we do not translate configure output, so is this a problem/project > policy to never change configure output in the back-branches? > > If the change is too invasive, would something like the initial patch I > suggested (i.e., in the absense of pkg-config, add a more targetted > error message) be acceptable for the back-branches? But it's not just changing an error message, it's changing the logic that leads to the error message. Have we really thought through all the combinations here? I don't know. So committing for master and then seeing if there is any further feedback seems most prudent. (I'm not endorsing either patch version here, just commenting on the process.) ^ permalink raw reply [nested|flat] 7+ messages in thread
end of thread, other threads:[~2024-08-15 13:22 UTC | newest] Thread overview: 7+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-06-06 22:42 [PATCH] Allow CREATE INDEX CONCURRENTLY on partitioned table Justin Pryzby <[email protected]> 2024-08-09 08:59 Re: Improve error message for ICU libraries if pkg-config is absent Heikki Linnakangas <[email protected]> 2024-08-09 09:44 ` Re: Improve error message for ICU libraries if pkg-config is absent Michael Banck <[email protected]> 2024-08-15 01:05 ` Re: Improve error message for ICU libraries if pkg-config is absent Jeff Davis <[email protected]> 2024-08-15 07:20 ` Re: Improve error message for ICU libraries if pkg-config is absent Michael Banck <[email protected]> 2024-08-15 13:22 ` Re: Improve error message for ICU libraries if pkg-config is absent Peter Eisentraut <[email protected]> 2024-08-13 20:13 ` Re: Improve error message for ICU libraries if pkg-config is absent Peter Eisentraut <[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