public inbox for [email protected]help / color / mirror / Atom feed
[PATCH] Avoid touching user indexes while they are being (re)built. 31+ messages / 9 participants [nested] [flat]
* [PATCH] Avoid touching user indexes while they are being (re)built. @ 2019-09-12 14:35 Arseny Sher <[email protected]> 0 siblings, 0 replies; 31+ messages in thread From: Arseny Sher @ 2019-09-12 14:35 UTC (permalink / raw) Existing ReindexIsProcessingIndex check is consulted only in genam.c and thus enforced only for system catalogs. Check it also in the planner, so that indexes which are currently being rebuilt are never used. Also cock SetReindexProcessing in index_create to defend from index self usage during its creation. Without this, VACUUM FULL or just CREATE INDEX might fail with something like ERROR: could not read block 3534 in file "base/41366676/56697497": read only 0 of 8192 bytes if there are indexes which usage can be considered during these very indexes (re)building, i.e. index expression scans indexed table. --- src/backend/catalog/index.c | 22 ++++++++++++++++++++-- src/backend/optimizer/util/plancat.c | 5 +++++ src/test/regress/expected/create_index.out | 12 ++++++++++++ src/test/regress/sql/create_index.sql | 13 +++++++++++++ 4 files changed, 50 insertions(+), 2 deletions(-) diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c index 3e1d40662d..5bc764ce46 100644 --- a/src/backend/catalog/index.c +++ b/src/backend/catalog/index.c @@ -1174,7 +1174,22 @@ index_create(Relation heapRelation, } else { - index_build(heapRelation, indexRelation, indexInfo, false, true); + /* ensure SetReindexProcessing state isn't leaked */ + PG_TRY(); + { + /* Suppress use of the target index while building it */ + SetReindexProcessing(heapRelationId, indexRelationId); + + index_build(heapRelation, indexRelation, indexInfo, false, true); + } + PG_CATCH(); + { + /* Make sure flag gets cleared on error exit */ + ResetReindexProcessing(); + PG_RE_THROW(); + } + PG_END_TRY(); + ResetReindexProcessing(); } /* @@ -1379,7 +1394,10 @@ index_concurrently_build(Oid heapRelationId, indexInfo->ii_Concurrent = true; indexInfo->ii_BrokenHotChain = false; - /* Now build the index */ + /* + * Now build the index + * SetReindexProcessing is not required since indisvalid is false anyway + */ index_build(heapRel, indexRelation, indexInfo, false, true); /* Close both the relations, but keep the locks */ diff --git a/src/backend/optimizer/util/plancat.c b/src/backend/optimizer/util/plancat.c index cf1761401d..9d58cd2574 100644 --- a/src/backend/optimizer/util/plancat.c +++ b/src/backend/optimizer/util/plancat.c @@ -27,6 +27,7 @@ #include "access/xlog.h" #include "catalog/catalog.h" #include "catalog/dependency.h" +#include "catalog/index.h" #include "catalog/heap.h" #include "catalog/pg_am.h" #include "catalog/pg_proc.h" @@ -193,6 +194,10 @@ get_relation_info(PlannerInfo *root, Oid relationObjectId, bool inhparent, nkeycolumns; int i; + /* Don't chase own tail */ + if (ReindexIsProcessingIndex(indexoid)) + continue; + /* * Extract info from the relation descriptor for the index. */ diff --git a/src/test/regress/expected/create_index.out b/src/test/regress/expected/create_index.out index 324db1b6ae..1706964277 100644 --- a/src/test/regress/expected/create_index.out +++ b/src/test/regress/expected/create_index.out @@ -1323,6 +1323,18 @@ create unique index hash_f8_index_1 on hash_f8_heap(abs(random)); create unique index hash_f8_index_2 on hash_f8_heap((seqno + 1), random); create unique index hash_f8_index_3 on hash_f8_heap(random) where seqno > 1000; -- +-- Create an index which might consider using this very index during the build. +-- +-- primary key ensures relhasindex is set +CREATE TABLE pears(f1 int primary key, f2 int); +INSERT INTO pears SELECT i, i+1 FROM generate_series(1, 100) i; +CREATE FUNCTION pears_f(i int) RETURNS int LANGUAGE SQL IMMUTABLE AS $$ + SELECT f1 FROM pears WHERE pears.f2 = 42 +$$; +CREATE index ON pears ((pears_f(f1))); +DROP TABLE pears; +DROP FUNCTION pears_f; +-- -- Try some concurrent index builds -- -- Unfortunately this only tests about half the code paths because there are diff --git a/src/test/regress/sql/create_index.sql b/src/test/regress/sql/create_index.sql index f96bebf410..76a781f6b0 100644 --- a/src/test/regress/sql/create_index.sql +++ b/src/test/regress/sql/create_index.sql @@ -446,6 +446,19 @@ create unique index hash_f8_index_2 on hash_f8_heap((seqno + 1), random); create unique index hash_f8_index_3 on hash_f8_heap(random) where seqno > 1000; -- +-- Create an index which might consider using this very index during the build. +-- +-- primary key ensures relhasindex is set +CREATE TABLE pears(f1 int primary key, f2 int); +INSERT INTO pears SELECT i, i+1 FROM generate_series(1, 100) i; +CREATE FUNCTION pears_f(i int) RETURNS int LANGUAGE SQL IMMUTABLE AS $$ + SELECT f1 FROM pears WHERE pears.f2 = 42 +$$; +CREATE index ON pears ((pears_f(f1))); +DROP TABLE pears; +DROP FUNCTION pears_f; + +-- -- Try some concurrent index builds -- -- Unfortunately this only tests about half the code paths because there are -- 2.11.0 --=-=-= Content-Type: text/plain -- Arseny Sher Postgres Professional: http://www.postgrespro.com The Russian Postgres Company --=-=-=-- ^ permalink raw reply [nested|flat] 31+ messages in thread
* [PATCH v3 07/17] Execute freezing in heap_page_prune() @ 2024-03-08 21:45 Melanie Plageman <[email protected]> 0 siblings, 0 replies; 31+ messages in thread From: Melanie Plageman @ 2024-03-08 21:45 UTC (permalink / raw) As a step toward combining the prune and freeze WAL records, execute freezing in heap_page_prune(). The logic to determine whether or not to execute freeze plans was moved from lazy_scan_prune() over to heap_page_prune() with little modification. --- src/backend/access/heap/heapam_handler.c | 2 +- src/backend/access/heap/pruneheap.c | 151 +++++++++++++++++------ src/backend/access/heap/vacuumlazy.c | 129 ++++++------------- src/backend/storage/ipc/procarray.c | 6 +- src/include/access/heapam.h | 41 +++--- src/tools/pgindent/typedefs.list | 2 +- 6 files changed, 180 insertions(+), 151 deletions(-) diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c index 680a50bf8b1..5e522f5b0ba 100644 --- a/src/backend/access/heap/heapam_handler.c +++ b/src/backend/access/heap/heapam_handler.c @@ -1046,7 +1046,7 @@ heapam_scan_analyze_next_tuple(TableScanDesc scan, TransactionId OldestXmin, * We ignore unused and redirect line pointers. DEAD line pointers * should be counted as dead, because we need vacuum to run to get rid * of them. Note that this rule agrees with the way that - * heap_page_prune() counts things. + * heap_page_prune_and_freeze() counts things. */ if (!ItemIdIsNormal(itemid)) { diff --git a/src/backend/access/heap/pruneheap.c b/src/backend/access/heap/pruneheap.c index 6bd8400b33b..abf6bdb2d99 100644 --- a/src/backend/access/heap/pruneheap.c +++ b/src/backend/access/heap/pruneheap.c @@ -17,16 +17,18 @@ #include "access/heapam.h" #include "access/heapam_xlog.h" #include "access/htup_details.h" +#include "access/multixact.h" #include "access/transam.h" #include "access/xlog.h" #include "access/xloginsert.h" +#include "executor/instrument.h" #include "miscadmin.h" #include "pgstat.h" #include "storage/bufmgr.h" #include "utils/rel.h" #include "utils/snapmgr.h" -/* Working data for heap_page_prune and subroutines */ +/* Working data for heap_page_prune_and_freeze() and subroutines */ typedef struct { Relation rel; @@ -61,17 +63,18 @@ static HTSV_Result heap_prune_satisfies_vacuum(PruneState *prstate, Buffer buffer); static int heap_prune_chain(Buffer buffer, OffsetNumber rootoffnum, - PruneState *prstate, PruneResult *presult); + PruneState *prstate, PruneFreezeResult *presult); static void prune_prepare_freeze_tuple(Page page, OffsetNumber offnum, - HeapPageFreeze *pagefrz, PruneResult *presult); + HeapPageFreeze *pagefrz, HeapTupleFreeze *frozen, + PruneFreezeResult *presult); static void heap_prune_record_prunable(PruneState *prstate, TransactionId xid); static void heap_prune_record_redirect(PruneState *prstate, OffsetNumber offnum, OffsetNumber rdoffnum); static void heap_prune_record_dead(PruneState *prstate, OffsetNumber offnum, - PruneResult *presult); + PruneFreezeResult *presult); static void heap_prune_record_dead_or_unused(PruneState *prstate, OffsetNumber offnum, - PruneResult *presult); + PruneFreezeResult *presult); static void heap_prune_record_unused(PruneState *prstate, OffsetNumber offnum); static void page_verify_redirects(Page page); @@ -151,15 +154,15 @@ heap_page_prune_opt(Relation relation, Buffer buffer) */ if (PageIsFull(page) || PageGetHeapFreeSpace(page) < minfree) { - PruneResult presult; + PruneFreezeResult presult; /* * For now, pass mark_unused_now as false regardless of whether or * not the relation has indexes, since we cannot safely determine * that during on-access pruning with the current implementation. */ - heap_page_prune(relation, buffer, vistest, false, NULL, - &presult, NULL); + heap_page_prune_and_freeze(relation, buffer, vistest, false, NULL, + &presult, NULL); /* * Report the number of tuples reclaimed to pgstats. This is @@ -207,7 +210,12 @@ prune_freeze_xmin_is_removable(GlobalVisState *visstate, TransactionId xmin) } /* - * Prune and repair fragmentation in the specified page. + * Prune and repair fragmentation and potentially freeze tuples on the + * specified page. + * + * If the page can be marked all-frozen in the visibility map, we may + * opportunistically freeze tuples on the page if either its tuples are old + * enough or freezing will be cheap enough. * * Caller must have pin and buffer cleanup lock on the page. Note that we * don't update the FSM information for page on caller's behalf. Caller might @@ -221,23 +229,24 @@ prune_freeze_xmin_is_removable(GlobalVisState *visstate, TransactionId xmin) * mark_unused_now indicates whether or not dead items can be set LP_UNUSED during * pruning. * - * pagefrz contains both input and output parameters used if the caller is - * interested in potentially freezing tuples on the page. + * pagefrz is an input parameter containing visibility cutoff information and + * the current relfrozenxid and relminmxids used if the caller is interested in + * freezing tuples on the page. * * off_loc is the offset location required by the caller to use in error * callback. * * presult contains output parameters needed by callers such as the number of * tuples removed and the number of line pointers newly marked LP_DEAD. - * heap_page_prune() is responsible for initializing it. + * heap_page_prune_and_freeze() is responsible for initializing it. */ void -heap_page_prune(Relation relation, Buffer buffer, - GlobalVisState *vistest, - bool mark_unused_now, - HeapPageFreeze *pagefrz, - PruneResult *presult, - OffsetNumber *off_loc) +heap_page_prune_and_freeze(Relation relation, Buffer buffer, + GlobalVisState *vistest, + bool mark_unused_now, + HeapPageFreeze *pagefrz, + PruneFreezeResult *presult, + OffsetNumber *off_loc) { Page page = BufferGetPage(buffer); BlockNumber blockno = BufferGetBlockNumber(buffer); @@ -245,6 +254,14 @@ heap_page_prune(Relation relation, Buffer buffer, maxoff; PruneState prstate; HeapTupleData tup; + bool do_freeze; + int64 fpi_before = pgWalUsage.wal_fpi; + TransactionId frz_conflict_horizon = InvalidTransactionId; + + /* + * One entry for every tuple that we may freeze. + */ + HeapTupleFreeze frozen[MaxHeapTuplesPerPage]; /* * Our strategy is to scan the page and make lists of items to change, @@ -281,6 +298,10 @@ heap_page_prune(Relation relation, Buffer buffer, /* for recovery conflicts */ presult->frz_conflict_horizon = InvalidTransactionId; + /* For advancing relfrozenxid and relminmxid */ + presult->new_relfrozenxid = InvalidTransactionId; + presult->new_relminmxid = InvalidMultiXactId; + maxoff = PageGetMaxOffsetNumber(page); tup.t_tableOid = RelationGetRelid(prstate.rel); @@ -440,7 +461,7 @@ heap_page_prune(Relation relation, Buffer buffer, if (pagefrz) prune_prepare_freeze_tuple(page, offnum, - pagefrz, presult); + pagefrz, frozen, presult); /* Ignore items already processed as part of an earlier chain */ if (prstate.marked[offnum]) @@ -555,6 +576,61 @@ heap_page_prune(Relation relation, Buffer buffer, /* Record number of newly-set-LP_DEAD items for caller */ presult->nnewlpdead = prstate.ndead; + + /* + * Freeze the page when heap_prepare_freeze_tuple indicates that at least + * one XID/MXID from before FreezeLimit/MultiXactCutoff is present. Also + * freeze when pruning generated an FPI, if doing so means that we set the + * page all-frozen afterwards (might not happen until final heap pass). + */ + if (pagefrz) + do_freeze = pagefrz->freeze_required || + (presult->all_visible_except_removable && presult->all_frozen && + presult->nfrozen > 0 && + fpi_before != pgWalUsage.wal_fpi); + else + do_freeze = false; + + if (do_freeze) + { + frz_conflict_horizon = heap_frz_conflict_horizon(presult, pagefrz); + + /* Execute all freeze plans for page as a single atomic action */ + heap_freeze_execute_prepared(relation, buffer, + frz_conflict_horizon, + frozen, presult->nfrozen); + } + else if (!pagefrz || !presult->all_frozen || presult->nfrozen > 0) + { + /* + * If we will neither freeze tuples on the page nor set the page all + * frozen in the visibility map, the page is not all frozen and there + * will be no newly frozen tuples. + */ + presult->all_frozen = false; + presult->nfrozen = 0; /* avoid miscounts in instrumentation */ + } + + /* Caller won't update new_relfrozenxid and new_relminmxid */ + if (!pagefrz) + return; + + /* + * If we will freeze tuples on the page or, even if we don't freeze tuples + * on the page, if we will set the page all-frozen in the visibility map, + * we can advance relfrozenxid and relminmxid to the values in + * pagefrz->FreezePageRelfrozenXid and pagefrz->FreezePageRelminMxid. + */ + if (presult->all_frozen || presult->nfrozen > 0) + { + presult->new_relfrozenxid = pagefrz->FreezePageRelfrozenXid; + presult->new_relminmxid = pagefrz->FreezePageRelminMxid; + } + else + { + presult->new_relfrozenxid = pagefrz->NoFreezePageRelfrozenXid; + presult->new_relminmxid = pagefrz->NoFreezePageRelminMxid; + } } @@ -612,7 +688,7 @@ heap_prune_satisfies_vacuum(PruneState *prstate, HeapTuple tup, Buffer buffer) */ static int heap_prune_chain(Buffer buffer, OffsetNumber rootoffnum, - PruneState *prstate, PruneResult *presult) + PruneState *prstate, PruneFreezeResult *presult) { int ndeleted = 0; Page dp = (Page) BufferGetPage(buffer); @@ -877,10 +953,10 @@ heap_prune_chain(Buffer buffer, OffsetNumber rootoffnum, { /* * We found a redirect item that doesn't point to a valid follow-on - * item. This can happen if the loop in heap_page_prune caused us to - * visit the dead successor of a redirect item before visiting the - * redirect item. We can clean up by setting the redirect item to - * DEAD state or LP_UNUSED if the caller indicated. + * item. This can happen if the loop in heap_page_prune_and_freeze() + * caused us to visit the dead successor of a redirect item before + * visiting the redirect item. We can clean up by setting the + * redirect item to DEAD state or LP_UNUSED if the caller indicated. */ heap_prune_record_dead_or_unused(prstate, rootoffnum, presult); } @@ -897,7 +973,8 @@ heap_prune_chain(Buffer buffer, OffsetNumber rootoffnum, static void prune_prepare_freeze_tuple(Page page, OffsetNumber offnum, HeapPageFreeze *pagefrz, - PruneResult *presult) + HeapTupleFreeze *frozen, + PruneFreezeResult *presult) { bool totally_frozen; HeapTupleHeader htup; @@ -919,11 +996,11 @@ prune_prepare_freeze_tuple(Page page, OffsetNumber offnum, /* Tuple with storage -- consider need to freeze */ if ((heap_prepare_freeze_tuple(htup, pagefrz, - &presult->frozen[presult->nfrozen], + &frozen[presult->nfrozen], &totally_frozen))) { /* Save prepared freeze plan for later */ - presult->frozen[presult->nfrozen++].offset = offnum; + frozen[presult->nfrozen++].offset = offnum; } /* @@ -967,7 +1044,7 @@ heap_prune_record_redirect(PruneState *prstate, /* Record line pointer to be marked dead */ static void heap_prune_record_dead(PruneState *prstate, OffsetNumber offnum, - PruneResult *presult) + PruneFreezeResult *presult) { Assert(prstate->ndead < MaxHeapTuplesPerPage); prstate->nowdead[prstate->ndead] = offnum; @@ -990,7 +1067,7 @@ heap_prune_record_dead(PruneState *prstate, OffsetNumber offnum, */ static void heap_prune_record_dead_or_unused(PruneState *prstate, OffsetNumber offnum, - PruneResult *presult) + PruneFreezeResult *presult) { /* * If the caller set mark_unused_now to true, we can remove dead tuples @@ -1017,9 +1094,9 @@ heap_prune_record_unused(PruneState *prstate, OffsetNumber offnum) /* - * Perform the actual page changes needed by heap_page_prune. - * It is expected that the caller has a full cleanup lock on the - * buffer. + * Perform the actual page pruning modifications needed by + * heap_page_prune_and_freeze(). It is expected that the caller has a full + * cleanup lock on the buffer. */ void heap_page_prune_execute(Buffer buffer, @@ -1133,11 +1210,11 @@ heap_page_prune_execute(Buffer buffer, #ifdef USE_ASSERT_CHECKING /* - * When heap_page_prune() was called, mark_unused_now may have been - * passed as true, which allows would-be LP_DEAD items to be made - * LP_UNUSED instead. This is only possible if the relation has no - * indexes. If there are any dead items, then mark_unused_now was not - * true and every item being marked LP_UNUSED must refer to a + * When heap_page_prune_and_freeze() was called, mark_unused_now may + * have been passed as true, which allows would-be LP_DEAD items to be + * made LP_UNUSED instead. This is only possible if the relation has + * no indexes. If there are any dead items, then mark_unused_now was + * not true and every item being marked LP_UNUSED must refer to a * heap-only tuple. */ if (ndead > 0) diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c index abbb7ab3ada..6dd8d457c9c 100644 --- a/src/backend/access/heap/vacuumlazy.c +++ b/src/backend/access/heap/vacuumlazy.c @@ -269,9 +269,6 @@ static void update_vacuum_error_info(LVRelState *vacrel, static void restore_vacuum_error_info(LVRelState *vacrel, const LVSavedErrInfo *saved_vacrel); -static TransactionId heap_frz_conflict_horizon(PruneResult *presult, - HeapPageFreeze *pagefrz); - /* * heap_vacuum_rel() -- perform VACUUM for one heap relation * @@ -432,12 +429,13 @@ heap_vacuum_rel(Relation rel, VacuumParams *params, * as an upper bound on the XIDs stored in the pages we'll actually scan * (NewRelfrozenXid tracking must never be allowed to miss unfrozen XIDs). * - * Next acquire vistest, a related cutoff that's used in heap_page_prune. - * We expect vistest will always make heap_page_prune remove any deleted - * tuple whose xmax is < OldestXmin. lazy_scan_prune must never become - * confused about whether a tuple should be frozen or removed. (In the - * future we might want to teach lazy_scan_prune to recompute vistest from - * time to time, to increase the number of dead tuples it can prune away.) + * Next acquire vistest, a related cutoff that's used in + * heap_page_prune_and_freeze(). We expect vistest will always make + * heap_page_prune_and_freeze() remove any deleted tuple whose xmax is < + * OldestXmin. lazy_scan_prune must never become confused about whether a + * tuple should be frozen or removed. (In the future we might want to + * teach lazy_scan_prune to recompute vistest from time to time, to + * increase the number of dead tuples it can prune away.) */ vacrel->aggressive = vacuum_get_cutoffs(rel, params, &vacrel->cutoffs); vacrel->rel_pages = orig_rel_pages = RelationGetNumberOfBlocks(rel); @@ -1379,8 +1377,8 @@ lazy_scan_new_or_empty(LVRelState *vacrel, Buffer buf, BlockNumber blkno, * Determine the snapshotConflictHorizon for freezing. Must only be called * after pruning and determining if the page is freezable. */ -static TransactionId -heap_frz_conflict_horizon(PruneResult *presult, HeapPageFreeze *pagefrz) +TransactionId +heap_frz_conflict_horizon(PruneFreezeResult *presult, HeapPageFreeze *pagefrz) { TransactionId result; @@ -1407,21 +1405,21 @@ heap_frz_conflict_horizon(PruneResult *presult, HeapPageFreeze *pagefrz) * * Caller must hold pin and buffer cleanup lock on the buffer. * - * Prior to PostgreSQL 14 there were very rare cases where heap_page_prune() - * was allowed to disagree with our HeapTupleSatisfiesVacuum() call about - * whether or not a tuple should be considered DEAD. This happened when an - * inserting transaction concurrently aborted (after our heap_page_prune() - * call, before our HeapTupleSatisfiesVacuum() call). There was rather a lot - * of complexity just so we could deal with tuples that were DEAD to VACUUM, - * but nevertheless were left with storage after pruning. + * Prior to PostgreSQL 14 there were very rare cases where + * heap_page_prune_and_freeze() was allowed to disagree with our + * HeapTupleSatisfiesVacuum() call about whether or not a tuple should be + * considered DEAD. This happened when an inserting transaction concurrently + * aborted (after our heap_page_prune_and_freeze() call, before our + * HeapTupleSatisfiesVacuum() call). There was rather a lot of complexity just + * so we could deal with tuples that were DEAD to VACUUM, but nevertheless were + * left with storage after pruning. * * As of Postgres 17, we circumvent this problem altogether by reusing the - * result of heap_page_prune()'s visibility check. Without the second call to - * HeapTupleSatisfiesVacuum(), there is no new HTSV_Result and there can be no - * disagreement. We'll just handle such tuples as if they had become fully dead - * right after this operation completes instead of in the middle of it. Note that - * any tuple that becomes dead after the call to heap_page_prune() can't need to - * be frozen, because it was visible to another session when vacuum started. + * result of heap_page_prune_and_freeze()'s visibility check. Without the + * second call to HeapTupleSatisfiesVacuum(), there is no new HTSV_Result and + * there can be no disagreement. We'll just handle such tuples as if they had + * become fully dead right after this operation completes instead of in the + * middle of it. * * vmbuffer is the buffer containing the VM block with visibility information * for the heap block, blkno. all_visible_according_to_vm is the saved @@ -1444,26 +1442,24 @@ lazy_scan_prune(LVRelState *vacrel, OffsetNumber offnum, maxoff; ItemId itemid; - PruneResult presult; + PruneFreezeResult presult; int lpdead_items, live_tuples, recently_dead_tuples; HeapPageFreeze pagefrz; bool hastup = false; - bool do_freeze; - int64 fpi_before = pgWalUsage.wal_fpi; OffsetNumber deadoffsets[MaxHeapTuplesPerPage]; Assert(BufferGetBlockNumber(buf) == blkno); /* * maxoff might be reduced following line pointer array truncation in - * heap_page_prune. That's safe for us to ignore, since the reclaimed - * space will continue to look like LP_UNUSED items below. + * heap_page_prune_and_freeze(). That's safe for us to ignore, since the + * reclaimed space will continue to look like LP_UNUSED items below. */ maxoff = PageGetMaxOffsetNumber(page); - /* Initialize (or reset) page-level state */ + /* Initialize pagefrz */ pagefrz.freeze_required = false; pagefrz.FreezePageRelfrozenXid = vacrel->NewRelfrozenXid; pagefrz.FreezePageRelminMxid = vacrel->NewRelminMxid; @@ -1475,7 +1471,7 @@ lazy_scan_prune(LVRelState *vacrel, recently_dead_tuples = 0; /* - * Prune all HOT-update chains in this page. + * Prune all HOT-update chains and potentially freeze tuples on this page. * * We count the number of tuples removed from the page by the pruning step * in presult.ndeleted. It should not be confused with lpdead_items; @@ -1486,8 +1482,8 @@ lazy_scan_prune(LVRelState *vacrel, * items LP_UNUSED, so mark_unused_now should be true if no indexes and * false otherwise. */ - heap_page_prune(rel, buf, vacrel->vistest, vacrel->nindexes == 0, - &pagefrz, &presult, &vacrel->offnum); + heap_page_prune_and_freeze(rel, buf, vacrel->vistest, vacrel->nindexes == 0, + &pagefrz, &presult, &vacrel->offnum); /* * Now scan the page to collect LP_DEAD items and check for tuples @@ -1604,72 +1600,23 @@ lazy_scan_prune(LVRelState *vacrel, vacrel->offnum = InvalidOffsetNumber; - /* - * Freeze the page when heap_prepare_freeze_tuple indicates that at least - * one XID/MXID from before FreezeLimit/MultiXactCutoff is present. Also - * freeze when pruning generated an FPI, if doing so means that we set the - * page all-frozen afterwards (might not happen until final heap pass). - */ - do_freeze = pagefrz.freeze_required || - (presult.all_visible_except_removable && presult.all_frozen && - presult.nfrozen > 0 && - fpi_before != pgWalUsage.wal_fpi); + Assert(MultiXactIdIsValid(presult.new_relminmxid)); + vacrel->NewRelfrozenXid = presult.new_relfrozenxid; + Assert(TransactionIdIsValid(presult.new_relfrozenxid)); + vacrel->NewRelminMxid = presult.new_relminmxid; - if (do_freeze) + if (presult.nfrozen > 0) { - TransactionId snapshotConflictHorizon; - /* - * We're freezing the page. Our final NewRelfrozenXid doesn't need to - * be affected by the XIDs that are just about to be frozen anyway. + * We never increment the frozen_pages instrumentation counter when + * nfrozen == 0, since it only counts pages with newly frozen tuples + * (don't confuse that with pages newly set all-frozen in VM). */ - vacrel->NewRelfrozenXid = pagefrz.FreezePageRelfrozenXid; - vacrel->NewRelminMxid = pagefrz.FreezePageRelminMxid; - vacrel->frozen_pages++; - snapshotConflictHorizon = heap_frz_conflict_horizon(&presult, &pagefrz); - /* Using same cutoff when setting VM is now unnecessary */ - if (presult.all_visible_except_removable && presult.all_frozen) + if (presult.all_frozen) presult.frz_conflict_horizon = InvalidTransactionId; - - /* Execute all freeze plans for page as a single atomic action */ - heap_freeze_execute_prepared(vacrel->rel, buf, - snapshotConflictHorizon, - presult.frozen, presult.nfrozen); - } - else if (presult.all_frozen && presult.nfrozen == 0) - { - /* Page should be all visible except to-be-removed tuples */ - Assert(presult.all_visible_except_removable); - - /* - * We have no freeze plans to execute, so there's no added cost from - * following the freeze path. That's why it was chosen. This is - * important in the case where the page only contains totally frozen - * tuples at this point (perhaps only following pruning). Such pages - * can be marked all-frozen in the VM by our caller, even though none - * of its tuples were newly frozen here (note that the "no freeze" - * path never sets pages all-frozen). - * - * We never increment the frozen_pages instrumentation counter here, - * since it only counts pages with newly frozen tuples (don't confuse - * that with pages newly set all-frozen in VM). - */ - vacrel->NewRelfrozenXid = pagefrz.FreezePageRelfrozenXid; - vacrel->NewRelminMxid = pagefrz.FreezePageRelminMxid; - } - else - { - /* - * Page requires "no freeze" processing. It might be set all-visible - * in the visibility map, but it can never be set all-frozen. - */ - vacrel->NewRelfrozenXid = pagefrz.NoFreezePageRelfrozenXid; - vacrel->NewRelminMxid = pagefrz.NoFreezePageRelminMxid; - presult.all_frozen = false; - presult.nfrozen = 0; /* avoid miscounts in instrumentation */ } /* diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c index b3cd248fb64..88a6d504dff 100644 --- a/src/backend/storage/ipc/procarray.c +++ b/src/backend/storage/ipc/procarray.c @@ -1715,9 +1715,9 @@ TransactionIdIsActive(TransactionId xid) * Note: the approximate horizons (see definition of GlobalVisState) are * updated by the computations done here. That's currently required for * correctness and a small optimization. Without doing so it's possible that - * heap vacuum's call to heap_page_prune() uses a more conservative horizon - * than later when deciding which tuples can be removed - which the code - * doesn't expect (breaking HOT). + * heap vacuum's call to heap_page_prune_and_freeze() uses a more conservative + * horizon than later when deciding which tuples can be removed - which the + * code doesn't expect (breaking HOT). */ static void ComputeXidHorizons(ComputeXidHorizonsResult *h) diff --git a/src/include/access/heapam.h b/src/include/access/heapam.h index 2339abfd28a..45c4ae22e6a 100644 --- a/src/include/access/heapam.h +++ b/src/include/access/heapam.h @@ -195,7 +195,7 @@ typedef struct HeapPageFreeze /* * Per-page state returned from pruning */ -typedef struct PruneResult +typedef struct PruneFreezeResult { int ndeleted; /* Number of tuples deleted from the page */ int nnewlpdead; /* Number of newly LP_DEAD items */ @@ -210,9 +210,10 @@ typedef struct PruneResult /* * Tuple visibility is only computed once for each tuple, for correctness - * and efficiency reasons; see comment in heap_page_prune() for details. - * This is of type int8[], instead of HTSV_Result[], so we can use -1 to - * indicate no visibility has been computed, e.g. for LP_DEAD items. + * and efficiency reasons; see comment in heap_page_prune_and_freeze() for + * details. This is of type int8[], instead of HTSV_Result[], so we can + * use -1 to indicate no visibility has been computed, e.g. for LP_DEAD + * items. * * This needs to be MaxHeapTuplesPerPage + 1 long as FirstOffsetNumber is * 1. Otherwise every access would need to subtract 1. @@ -220,17 +221,18 @@ typedef struct PruneResult int8 htsv[MaxHeapTuplesPerPage + 1]; - /* - * One entry for every tuple that we may freeze. - */ - HeapTupleFreeze frozen[MaxHeapTuplesPerPage]; -} PruneResult; + /* New value of relfrozenxid found by heap_page_prune_and_freeze() */ + TransactionId new_relfrozenxid; + + /* New value of relminmxid found by heap_page_prune_and_freeze() */ + MultiXactId new_relminmxid; +} PruneFreezeResult; /* * Pruning calculates tuple visibility once and saves the results in an array - * of int8. See PruneResult.htsv for details. This helper function is meant to - * guard against examining visibility status array members which have not yet - * been computed. + * of int8. See PruneFreezeResult.htsv for details. This helper function is + * meant to guard against examining visibility status array members which have + * not yet been computed. */ static inline HTSV_Result htsv_get_valid_status(int status) @@ -306,6 +308,9 @@ extern TM_Result heap_lock_tuple(Relation relation, HeapTuple tuple, Buffer *buffer, struct TM_FailureData *tmfd); extern void heap_inplace_update(Relation relation, HeapTuple tuple); + +extern TransactionId heap_frz_conflict_horizon(PruneFreezeResult *presult, + HeapPageFreeze *pagefrz); extern bool heap_prepare_freeze_tuple(HeapTupleHeader tuple, HeapPageFreeze *pagefrz, HeapTupleFreeze *frz, bool *totally_frozen); @@ -332,12 +337,12 @@ extern TransactionId heap_index_delete_tuples(Relation rel, /* in heap/pruneheap.c */ struct GlobalVisState; extern void heap_page_prune_opt(Relation relation, Buffer buffer); -extern void heap_page_prune(Relation relation, Buffer buffer, - struct GlobalVisState *vistest, - bool mark_unused_now, - HeapPageFreeze *pagefrz, - PruneResult *presult, - OffsetNumber *off_loc); +extern void heap_page_prune_and_freeze(Relation relation, Buffer buffer, + struct GlobalVisState *vistest, + bool mark_unused_now, + HeapPageFreeze *pagefrz, + PruneFreezeResult *presult, + OffsetNumber *off_loc); extern void heap_page_prune_execute(Buffer buffer, OffsetNumber *redirected, int nredirected, OffsetNumber *nowdead, int ndead, diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list index aa7a25b8f8c..1c1a4d305d6 100644 --- a/src/tools/pgindent/typedefs.list +++ b/src/tools/pgindent/typedefs.list @@ -2175,7 +2175,7 @@ ProjectionPath PromptInterruptContext ProtocolVersion PrsStorage -PruneResult +PruneFreezeResult PruneState PruneStepResult PsqlScanCallbacks -- 2.40.1 --racicctn4wry6xe5 Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v3-0008-Make-opp-freeze-heuristic-compatible-with-prune-f.patch" ^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: RFC: Additional Directory for Extensions @ 2025-03-10 20:25 Matheus Alcantara <[email protected]> 0 siblings, 1 reply; 31+ messages in thread From: Matheus Alcantara @ 2025-03-10 20:25 UTC (permalink / raw) To: Peter Eisentraut <[email protected]>; +Cc: Andrew Dunstan <[email protected]>; Gabriele Bartolini <[email protected]>; Craig Ringer <[email protected]>; David E. Wheeler <[email protected]>; Robert Haas <[email protected]>; Alvaro Herrera <[email protected]>; pgsql-hackers Hi, Thanks for reviewing and suggestions! On Thu, Mar 6, 2025 at 10:46 AM Peter Eisentraut <[email protected]> wrote: > This looks very good to me. I have one issue to point out: The logic > in get_extension_control_directories() needs to be a little bit more > careful to align with the rules in find_in_path(). For example, it > should use first_path_var_separator() to get the platform-specific path > separator, and probably also substitute_path_macro() and > canonicalize_path() etc., to keep everything consistent. > I fixed this hardcoded path separator issue on the TAP test and forgot to fix it also on code, sorry, fixed on this new version. I also spent some time investigating why the tests on Windows were still passing even using a wrong path separator. Consider extension_control_path = '$system;C:\custom\path' When running on Windows, the get_extension_control_directories was returning [$system;C:, \custom\path] and for somehow the \custom\path was successfully being read and since the tests was only referencing the extension on this custom path everything was passing, but querying for an extension that is only on $system was resulting in an empty query result. In the attached patch I also included a new test case to query on pg_available_extensions for an extension that is installed on the $system, so we can ensure that extensions in both paths can be used correctly. > (Maybe it would be ok to move the function to dfmgr.c to avoid having > to export too many things from there.) > I've exported substitute_path_macro because adding a new function on dfmgr would require #include nodes/pg_list.h and I'm not sure what approach would be better, please let me know what you think. -- Matheus Alcantara Attachments: [application/octet-stream] v6-0001-extension_control_path.patch (35.1K, ../../CAFY6G8cGeUV0f5K8v-Du0ts3iZyRE6Q5dNtYjQq8cjS4epLX5A@mail.gmail.com/2-v6-0001-extension_control_path.patch) download | inline diff: From f0ed47907ab40d3201dfaad6d29f57155337dc2c Mon Sep 17 00:00:00 2001 From: Peter Eisentraut <[email protected]> Date: Thu, 5 Dec 2024 11:49:05 +0100 Subject: [PATCH v6] extension_control_path The new GUC extension_control_path specifies a path to look for extension control files. The default value is $system, which looks in the compiled-in location, as before. The path search uses the same code and works in the same way as dynamic_library_path. Discussion: https://www.postgresql.org/message-id/flat/[email protected] --- doc/src/sgml/config.sgml | 68 ++++ doc/src/sgml/extend.sgml | 19 +- doc/src/sgml/ref/create_extension.sgml | 6 +- src/Makefile.global.in | 19 +- src/backend/commands/extension.c | 356 +++++++++++------- src/backend/utils/fmgr/dfmgr.c | 77 ++-- src/backend/utils/misc/guc_tables.c | 13 + src/backend/utils/misc/postgresql.conf.sample | 1 + src/include/commands/extension.h | 2 + src/include/fmgr.h | 5 + src/test/modules/test_extensions/Makefile | 1 + src/test/modules/test_extensions/meson.build | 5 + .../t/001_extension_control_path.pl | 77 ++++ 13 files changed, 475 insertions(+), 174 deletions(-) create mode 100644 src/test/modules/test_extensions/t/001_extension_control_path.pl diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index d2fa5f7d1a9..9fec78db6f7 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -10725,6 +10725,74 @@ dynamic_library_path = 'C:\tools\postgresql;H:\my_project\lib;$libdir' </listitem> </varlistentry> + <varlistentry id="guc-extension-control-path" xreflabel="extension_control_path"> + <term><varname>extension_control_path</varname> (<type>string</type>) + <indexterm> + <primary><varname>extension_control_path</varname> configuration parameter</primary> + </indexterm> + </term> + <listitem> + <para> + A path to search for extensions, specifically extension control files + (<filename><replaceable>name</replaceable>.control</filename>). The + remaining extension script and secondary control files are then loaded + from the same directory where the primary control file was found. + See <xref linkend="extend-extensions-files"/> for details. + </para> + + <para> + The value for <varname>extension_control_path</varname> must be a + list of absolute directory paths separated by colons (or semi-colons + on Windows). If a list element starts + with the special string <literal>$system</literal>, the + compiled-in <productname>PostgreSQL</productname> extension + directory is substituted for <literal>$system</literal>; this + is where the extensions provided by the standard + <productname>PostgreSQL</productname> distribution are installed. + (Use <literal>pg_config --sharedir</literal> to find out the name of + this directory.) For example: +<programlisting> +extension_control_path = '/usr/local/share/postgresql/extension:/home/my_project/share/extension:$system' +</programlisting> + or, in a Windows environment: +<programlisting> +extension_control_path = 'C:\tools\postgresql\extension;H:\my_project\share\extension;$system' +</programlisting> + Note that the path elements should typically end in + <literal>extension</literal> if the normal installation layouts are + followed. (The value for <literal>$system</literal> already includes + the <literal>extension</literal> suffix.) + </para> + + <para> + The default value for this parameter is + <literal>'$system'</literal>. If the value is set to an empty + string, the default <literal>'$system'</literal> is also assumed. + </para> + + <para> + This parameter can be changed at run time by superusers and users + with the appropriate <literal>SET</literal> privilege, but a + setting done that way will only persist until the end of the + client connection, so this method should be reserved for + development purposes. The recommended way to set this parameter + is in the <filename>postgresql.conf</filename> configuration + file. + </para> + + <para> + Note that if you set this parameter to be able to load extensions from + nonstandard locations, you will most likely also need to set <xref + linkend="guc-dynamic-library-path"/> to a correspondent location, for + example, +<programlisting> +extension_control_path = '/usr/local/share/postgresql/extension:$system' +dynamic_library_path = '/usr/local/lib/postgresql:$libdir' +</programlisting> + </para> + </listitem> + </varlistentry> + <varlistentry id="guc-gin-fuzzy-search-limit" xreflabel="gin_fuzzy_search_limit"> <term><varname>gin_fuzzy_search_limit</varname> (<type>integer</type>) <indexterm> diff --git a/doc/src/sgml/extend.sgml b/doc/src/sgml/extend.sgml index ba492ca27c0..64f8e133cae 100644 --- a/doc/src/sgml/extend.sgml +++ b/doc/src/sgml/extend.sgml @@ -649,6 +649,11 @@ RETURNS anycompatible AS ... control file can specify a different directory for the script file(s). </para> + <para> + Additional locations for extension control files can be configured using + the parameter <xref linkend="guc-extension-control-path"/>. + </para> + <para> The file format for an extension control file is the same as for the <filename>postgresql.conf</filename> file, namely a list of @@ -669,9 +674,9 @@ RETURNS anycompatible AS ... <para> The directory containing the extension's <acronym>SQL</acronym> script file(s). Unless an absolute path is given, the name is relative to - the installation's <literal>SHAREDIR</literal> directory. The - default behavior is equivalent to specifying - <literal>directory = 'extension'</literal>. + the installation's <literal>SHAREDIR</literal> directory. By default, + the script files are looked for in the same directory where the + control file was found. </para> </listitem> </varlistentry> @@ -719,8 +724,8 @@ RETURNS anycompatible AS ... <para> The value of this parameter will be substituted for each occurrence of <literal>MODULE_PATHNAME</literal> in the script file(s). If it is not - set, no substitution is made. Typically, this is set to - <literal>$libdir/<replaceable>shared_library_name</replaceable></literal> and + set, no substitution is made. Typically, this is set to just + <literal><replaceable>shared_library_name</replaceable></literal> and then <literal>MODULE_PATHNAME</literal> is used in <command>CREATE FUNCTION</command> commands for C-language functions, so that the script files do not need to hard-wire the name of the shared library. @@ -1804,6 +1809,10 @@ include $(PGXS) setting <varname>PG_CONFIG</varname> to point to its <command>pg_config</command> program, either within the makefile or on the <literal>make</literal> command line. + You can also select a separate installation directory for your extension + by setting the <literal>make</literal> variable <varname>prefix</varname> + on the <literal>make</literal> command line. (But this will then require + additional setup to get the server to find the extension there.) </para> <para> diff --git a/doc/src/sgml/ref/create_extension.sgml b/doc/src/sgml/ref/create_extension.sgml index ca2b80d669c..713abd9c494 100644 --- a/doc/src/sgml/ref/create_extension.sgml +++ b/doc/src/sgml/ref/create_extension.sgml @@ -90,8 +90,10 @@ CREATE EXTENSION [ IF NOT EXISTS ] <replaceable class="parameter">extension_name <para> The name of the extension to be installed. <productname>PostgreSQL</productname> will create the - extension using details from the file - <literal>SHAREDIR/extension/</literal><replaceable class="parameter">extension_name</replaceable><literal>.control</literal>. + extension using details from the file <filename><replaceable + class="parameter">extension_name</replaceable>.control</filename>, + found via the server's extension control path (set by <xref + linkend="guc-extension-control-path"/>.) </para> </listitem> </varlistentry> diff --git a/src/Makefile.global.in b/src/Makefile.global.in index 3b620bac5ac..8fe9d61e82a 100644 --- a/src/Makefile.global.in +++ b/src/Makefile.global.in @@ -87,9 +87,19 @@ endif # not PGXS # # In a PGXS build, we cannot use the values inserted into Makefile.global # by configure, since the installation tree may have been relocated. -# Instead get the path values from pg_config. +# Instead get the path values from pg_config. But users can specify +# prefix explicitly, if they want to select their own installation +# location. -ifndef PGXS +ifdef PGXS +# Extension makefiles should set PG_CONFIG, but older ones might not +ifndef PG_CONFIG +PG_CONFIG = pg_config +endif +endif + +# This means: if ((not PGXS) or prefix) +ifneq (,$(if $(PGXS),,1)$(prefix)) # Note that prefix, exec_prefix, and datarootdir aren't defined in a PGXS build; # makefiles may only use the derived variables such as bindir. @@ -147,11 +157,6 @@ localedir := @localedir@ else # PGXS case -# Extension makefiles should set PG_CONFIG, but older ones might not -ifndef PG_CONFIG -PG_CONFIG = pg_config -endif - bindir := $(shell $(PG_CONFIG) --bindir) datadir := $(shell $(PG_CONFIG) --sharedir) sysconfdir := $(shell $(PG_CONFIG) --sysconfdir) diff --git a/src/backend/commands/extension.c b/src/backend/commands/extension.c index d9bb4ce5f1e..a45389807de 100644 --- a/src/backend/commands/extension.c +++ b/src/backend/commands/extension.c @@ -54,6 +54,7 @@ #include "funcapi.h" #include "mb/pg_wchar.h" #include "miscadmin.h" +#include "nodes/pg_list.h" #include "nodes/queryjumble.h" #include "storage/fd.h" #include "tcop/utility.h" @@ -69,6 +70,9 @@ #include "utils/varlena.h" +/* GUC */ +char *Extension_control_path; + /* Globally visible state variables */ bool creating_extension = false; Oid CurrentExtensionObject = InvalidOid; @@ -79,6 +83,7 @@ Oid CurrentExtensionObject = InvalidOid; typedef struct ExtensionControlFile { char *name; /* name of the extension */ + char *control_dir; /* directory where control file was found */ char *directory; /* directory for script files */ char *default_version; /* default install target version, if any */ char *module_pathname; /* string to substitute for @@ -328,29 +333,106 @@ is_extension_script_filename(const char *filename) return (extension != NULL) && (strcmp(extension, ".sql") == 0); } -static char * -get_extension_control_directory(void) +/* + * Return a list of directories declared on extension_control_path GUC. + */ +static List * +get_extension_control_directories(void) { char sharepath[MAXPGPATH]; - char *result; + char *system_dir; + char *ecp; + List *paths = NIL; get_share_path(my_exec_path, sharepath); - result = (char *) palloc(MAXPGPATH); - snprintf(result, MAXPGPATH, "%s/extension", sharepath); - return result; + system_dir = psprintf("%s/extension", sharepath); + + if (strlen(Extension_control_path) == 0) + { + paths = lappend(paths, system_dir); + } + else + { + /* Duplicate the string so we can modify it */ + ecp = pstrdup(Extension_control_path); + + { + for (;;) + { + int len; + char *mangled; + char *piece = first_path_var_separator(ecp); + + /* Get the length of the next path on ecp */ + if (piece == NULL) + len = strlen(ecp); + else + len = piece - ecp; + + /* Copy the next path found on ecp */ + piece = palloc(len + 1); + strlcpy(piece, ecp, len + 1); + + /* Substitute the path macro if needed */ + mangled = substitute_path_macro(piece, "$system", system_dir); + pfree(piece); + + /* Canonicalize the path based on the OS and add to the list */ + canonicalize_path(mangled); + paths = lappend(paths, mangled); + + /* Break if ecp is empty or move to the next path on ecp */ + if (ecp[len] == '\0') + break; + else + ecp += len + 1; + } + } + } + + return paths; } +/* + * Find control file for extension with name in control->name, looking in the + * path. Return the full file name, or NULL if not found. If found, the + * directory is recorded in control->control_dir. + */ static char * -get_extension_control_filename(const char *extname) +find_extension_control_filename(ExtensionControlFile *control) { char sharepath[MAXPGPATH]; + char *system_dir; + char *basename; + char *ecp; char *result; + Assert(control->name); + get_share_path(my_exec_path, sharepath); - result = (char *) palloc(MAXPGPATH); - snprintf(result, MAXPGPATH, "%s/extension/%s.control", - sharepath, extname); + system_dir = psprintf("%s/extension", sharepath); + + basename = psprintf("%s.control", control->name); + + /* + * find_in_path() does nothing if the path value is empty. This is the + * historical behavior for dynamic_library_path, but it makes no sense for + * extensions. So in that case, substitute a default value. + */ + ecp = Extension_control_path; + if (strlen(ecp) == 0) + ecp = "$system"; + result = find_in_path(basename, Extension_control_path, "extension_control_path", "$system", system_dir); + + if (result) + { + const char *p; + + p = strrchr(result, '/'); + Assert(p); + control->control_dir = pnstrdup(result, p - result); + } return result; } @@ -366,7 +448,7 @@ get_extension_script_directory(ExtensionControlFile *control) * installation's share directory. */ if (!control->directory) - return get_extension_control_directory(); + return pstrdup(control->control_dir); if (is_absolute_path(control->directory)) return pstrdup(control->directory); @@ -444,27 +526,25 @@ parse_extension_control_file(ExtensionControlFile *control, if (version) filename = get_extension_aux_control_filename(control, version); else - filename = get_extension_control_filename(control->name); + filename = find_extension_control_filename(control); + + if (!filename) + { + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("extension \"%s\" is not available", control->name), + errhint("The extension must first be installed on the system where PostgreSQL is running."))); + } if ((file = AllocateFile(filename, "r")) == NULL) { - if (errno == ENOENT) + /* no complaint for missing auxiliary file */ + if (errno == ENOENT && version) { - /* no complaint for missing auxiliary file */ - if (version) - { - pfree(filename); - return; - } - - /* missing control file indicates extension is not installed */ - ereport(ERROR, - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), - errmsg("extension \"%s\" is not available", control->name), - errdetail("Could not open extension control file \"%s\": %m.", - filename), - errhint("The extension must first be installed on the system where PostgreSQL is running."))); + pfree(filename); + return; } + ereport(ERROR, (errcode_for_file_access(), errmsg("could not open extension control file \"%s\": %m", @@ -2121,68 +2201,72 @@ Datum pg_available_extensions(PG_FUNCTION_ARGS) { ReturnSetInfo *rsinfo = (ReturnSetInfo *) fcinfo->resultinfo; - char *location; + List *locations; DIR *dir; struct dirent *de; /* Build tuplestore to hold the result rows */ InitMaterializedSRF(fcinfo, 0); - location = get_extension_control_directory(); - dir = AllocateDir(location); + locations = get_extension_control_directories(); - /* - * If the control directory doesn't exist, we want to silently return an - * empty set. Any other error will be reported by ReadDir. - */ - if (dir == NULL && errno == ENOENT) - { - /* do nothing */ - } - else + foreach_ptr(char, location, locations) { - while ((de = ReadDir(dir, location)) != NULL) + dir = AllocateDir(location); + + /* + * If the control directory doesn't exist, we want to silently return + * an empty set. Any other error will be reported by ReadDir. + */ + if (dir == NULL && errno == ENOENT) { - ExtensionControlFile *control; - char *extname; - Datum values[3]; - bool nulls[3]; + /* do nothing */ + } + else + { + while ((de = ReadDir(dir, location)) != NULL) + { + ExtensionControlFile *control; + char *extname; + Datum values[3]; + bool nulls[3]; - if (!is_extension_control_filename(de->d_name)) - continue; + if (!is_extension_control_filename(de->d_name)) + continue; - /* extract extension name from 'name.control' filename */ - extname = pstrdup(de->d_name); - *strrchr(extname, '.') = '\0'; + /* extract extension name from 'name.control' filename */ + extname = pstrdup(de->d_name); + *strrchr(extname, '.') = '\0'; - /* ignore it if it's an auxiliary control file */ - if (strstr(extname, "--")) - continue; + /* ignore it if it's an auxiliary control file */ + if (strstr(extname, "--")) + continue; - control = read_extension_control_file(extname); + control = read_extension_control_file(extname); - memset(values, 0, sizeof(values)); - memset(nulls, 0, sizeof(nulls)); + memset(values, 0, sizeof(values)); + memset(nulls, 0, sizeof(nulls)); - /* name */ - values[0] = DirectFunctionCall1(namein, - CStringGetDatum(control->name)); - /* default_version */ - if (control->default_version == NULL) - nulls[1] = true; - else - values[1] = CStringGetTextDatum(control->default_version); - /* comment */ - if (control->comment == NULL) - nulls[2] = true; - else - values[2] = CStringGetTextDatum(control->comment); + /* name */ + values[0] = DirectFunctionCall1(namein, + CStringGetDatum(control->name)); + /* default_version */ + if (control->default_version == NULL) + nulls[1] = true; + else + values[1] = CStringGetTextDatum(control->default_version); + /* comment */ + if (control->comment == NULL) + nulls[2] = true; + else + values[2] = CStringGetTextDatum(control->comment); - tuplestore_putvalues(rsinfo->setResult, rsinfo->setDesc, - values, nulls); - } + tuplestore_putvalues(rsinfo->setResult, rsinfo->setDesc, + values, nulls); + } - FreeDir(dir); + FreeDir(dir); + } } return (Datum) 0; @@ -2201,51 +2285,55 @@ Datum pg_available_extension_versions(PG_FUNCTION_ARGS) { ReturnSetInfo *rsinfo = (ReturnSetInfo *) fcinfo->resultinfo; - char *location; + List *locations; DIR *dir; struct dirent *de; /* Build tuplestore to hold the result rows */ InitMaterializedSRF(fcinfo, 0); - location = get_extension_control_directory(); - dir = AllocateDir(location); + locations = get_extension_control_directories(); - /* - * If the control directory doesn't exist, we want to silently return an - * empty set. Any other error will be reported by ReadDir. - */ - if (dir == NULL && errno == ENOENT) - { - /* do nothing */ - } - else + foreach_ptr(char, location, locations) { - while ((de = ReadDir(dir, location)) != NULL) + dir = AllocateDir(location); + + /* + * If the control directory doesn't exist, we want to silently return + * an empty set. Any other error will be reported by ReadDir. + */ + if (dir == NULL && errno == ENOENT) { - ExtensionControlFile *control; - char *extname; + /* do nothing */ + } + else + { + while ((de = ReadDir(dir, location)) != NULL) + { + ExtensionControlFile *control; + char *extname; - if (!is_extension_control_filename(de->d_name)) - continue; + if (!is_extension_control_filename(de->d_name)) + continue; - /* extract extension name from 'name.control' filename */ - extname = pstrdup(de->d_name); - *strrchr(extname, '.') = '\0'; + /* extract extension name from 'name.control' filename */ + extname = pstrdup(de->d_name); + *strrchr(extname, '.') = '\0'; - /* ignore it if it's an auxiliary control file */ - if (strstr(extname, "--")) - continue; + /* ignore it if it's an auxiliary control file */ + if (strstr(extname, "--")) + continue; - /* read the control file */ - control = read_extension_control_file(extname); + /* read the control file */ + control = read_extension_control_file(extname); - /* scan extension's script directory for install scripts */ - get_available_versions_for_extension(control, rsinfo->setResult, - rsinfo->setDesc); - } + /* scan extension's script directory for install scripts */ + get_available_versions_for_extension(control, rsinfo->setResult, + rsinfo->setDesc); + } - FreeDir(dir); + FreeDir(dir); + } } return (Datum) 0; @@ -2373,47 +2461,53 @@ bool extension_file_exists(const char *extensionName) { bool result = false; - char *location; + List *locations; DIR *dir; struct dirent *de; - location = get_extension_control_directory(); - dir = AllocateDir(location); + locations = get_extension_control_directories(); - /* - * If the control directory doesn't exist, we want to silently return - * false. Any other error will be reported by ReadDir. - */ - if (dir == NULL && errno == ENOENT) - { - /* do nothing */ - } - else + foreach_ptr(char, location, locations) { - while ((de = ReadDir(dir, location)) != NULL) + dir = AllocateDir(location); + + /* + * If the control directory doesn't exist, we want to silently return + * false. Any other error will be reported by ReadDir. + */ + if (dir == NULL && errno == ENOENT) + { + /* do nothing */ + } + else { - char *extname; + while ((de = ReadDir(dir, location)) != NULL) + { + char *extname; - if (!is_extension_control_filename(de->d_name)) - continue; + if (!is_extension_control_filename(de->d_name)) + continue; - /* extract extension name from 'name.control' filename */ - extname = pstrdup(de->d_name); - *strrchr(extname, '.') = '\0'; + /* extract extension name from 'name.control' filename */ + extname = pstrdup(de->d_name); + *strrchr(extname, '.') = '\0'; - /* ignore it if it's an auxiliary control file */ - if (strstr(extname, "--")) - continue; + /* ignore it if it's an auxiliary control file */ + if (strstr(extname, "--")) + continue; - /* done if it matches request */ - if (strcmp(extname, extensionName) == 0) - { - result = true; - break; + /* done if it matches request */ + if (strcmp(extname, extensionName) == 0) + { + result = true; + break; + } } - } - FreeDir(dir); + FreeDir(dir); + } + if (result) + break; } return result; diff --git a/src/backend/utils/fmgr/dfmgr.c b/src/backend/utils/fmgr/dfmgr.c index 87b233cb887..ca12e954ea2 100644 --- a/src/backend/utils/fmgr/dfmgr.c +++ b/src/backend/utils/fmgr/dfmgr.c @@ -71,8 +71,6 @@ static void incompatible_module_error(const char *libname, const Pg_magic_struct *module_magic_data) pg_attribute_noreturn(); static char *expand_dynamic_library_name(const char *name); static void check_restricted_library_name(const char *name); -static char *substitute_libpath_macro(const char *name); -static char *find_in_dynamic_libpath(const char *basename); /* Magic structure that module needs to match to be accepted */ static const Pg_magic_struct magic_data = PG_MODULE_MAGIC_DATA; @@ -398,7 +396,7 @@ incompatible_module_error(const char *libname, /* * If name contains a slash, check if the file exists, if so return * the name. Else (no slash) try to expand using search path (see - * find_in_dynamic_libpath below); if that works, return the fully + * find_in_path below); if that works, return the fully * expanded file name. If the previous failed, append DLSUFFIX and * try again. If all fails, just return the original name. * @@ -413,17 +411,25 @@ expand_dynamic_library_name(const char *name) Assert(name); + /* + * If the value starts with "$libdir/", strip that. This is because many + * extensions have hardcoded '$libdir/foo' as their library name, which + * prevents using the path. + */ + if (strncmp(name, "$libdir/", 8) == 0) + name += 8; + have_slash = (first_dir_separator(name) != NULL); if (!have_slash) { - full = find_in_dynamic_libpath(name); + full = find_in_path(name, Dynamic_library_path, "dynamic_library_path", "$libdir", pkglib_path); if (full) return full; } else { - full = substitute_libpath_macro(name); + full = substitute_path_macro(name, "$libdir", pkglib_path); if (pg_file_exists(full)) return full; pfree(full); @@ -433,14 +439,14 @@ expand_dynamic_library_name(const char *name) if (!have_slash) { - full = find_in_dynamic_libpath(new); + full = find_in_path(new, Dynamic_library_path, "dynamic_library_path", "$libdir", pkglib_path); pfree(new); if (full) return full; } else { - full = substitute_libpath_macro(new); + full = substitute_path_macro(new, "$libdir", pkglib_path); pfree(new); if (pg_file_exists(full)) return full; @@ -474,48 +480,61 @@ check_restricted_library_name(const char *name) * Substitute for any macros appearing in the given string. * Result is always freshly palloc'd. */ -static char * -substitute_libpath_macro(const char *name) +char * +substitute_path_macro(const char *str, const char *macro, const char *value) { const char *sep_ptr; - Assert(name != NULL); + Assert(str != NULL); + Assert(macro[0] == '$'); - /* Currently, we only recognize $libdir at the start of the string */ - if (name[0] != '$') - return pstrdup(name); + /* Currently, we only recognize $macro at the start of the string */ + if (str[0] != '$') + return pstrdup(str); - if ((sep_ptr = first_dir_separator(name)) == NULL) - sep_ptr = name + strlen(name); + if ((sep_ptr = first_dir_separator(str)) == NULL) + sep_ptr = str + strlen(str); - if (strlen("$libdir") != sep_ptr - name || - strncmp(name, "$libdir", strlen("$libdir")) != 0) + if (strlen(macro) != sep_ptr - str || + strncmp(str, macro, strlen(macro)) != 0) ereport(ERROR, (errcode(ERRCODE_INVALID_NAME), - errmsg("invalid macro name in dynamic library path: %s", - name))); + errmsg("invalid macro name in path: %s", + str))); - return psprintf("%s%s", pkglib_path, sep_ptr); + return psprintf("%s%s", value, sep_ptr); } /* * Search for a file called 'basename' in the colon-separated search - * path Dynamic_library_path. If the file is found, the full file name + * path given. If the file is found, the full file name * is returned in freshly palloc'd memory. If the file is not found, * return NULL. + * + * path_param is the name of the parameter that path came from, for error + * messages. + * + * macro and macro_val allow substituting a macro; see + * substitute_path_macro(). */ -static char * -find_in_dynamic_libpath(const char *basename) +char * +find_in_path(const char *basename, const char *path, const char *path_param, + const char *macro, const char *macro_val) { const char *p; size_t baselen; Assert(basename != NULL); Assert(first_dir_separator(basename) == NULL); - Assert(Dynamic_library_path != NULL); + Assert(path != NULL); + Assert(path_param != NULL); + + p = path; - p = Dynamic_library_path; + /* + * If the path variable is empty, don't do a path search. + */ if (strlen(p) == 0) return NULL; @@ -532,7 +551,7 @@ find_in_dynamic_libpath(const char *basename) if (piece == p) ereport(ERROR, (errcode(ERRCODE_INVALID_NAME), - errmsg("zero-length component in parameter \"dynamic_library_path\""))); + errmsg("zero-length component in parameter \"%s\"", path_param))); if (piece == NULL) len = strlen(p); @@ -542,7 +561,7 @@ find_in_dynamic_libpath(const char *basename) piece = palloc(len + 1); strlcpy(piece, p, len + 1); - mangled = substitute_libpath_macro(piece); + mangled = substitute_path_macro(piece, macro, macro_val); pfree(piece); canonicalize_path(mangled); @@ -551,13 +570,13 @@ find_in_dynamic_libpath(const char *basename) if (!is_absolute_path(mangled)) ereport(ERROR, (errcode(ERRCODE_INVALID_NAME), - errmsg("component in parameter \"dynamic_library_path\" is not an absolute path"))); + errmsg("component in parameter \"%s\" is not an absolute path", path_param))); full = palloc(strlen(mangled) + 1 + baselen + 1); sprintf(full, "%s/%s", mangled, basename); pfree(mangled); - elog(DEBUG3, "find_in_dynamic_libpath: trying \"%s\"", full); + elog(DEBUG3, "%s: trying \"%s\"", __func__, full); if (pg_file_exists(full)) return full; diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c index ad25cbb39c5..c357a5304ae 100644 --- a/src/backend/utils/misc/guc_tables.c +++ b/src/backend/utils/misc/guc_tables.c @@ -39,6 +39,7 @@ #include "catalog/namespace.h" #include "catalog/storage.h" #include "commands/async.h" +#include "commands/extension.h" #include "commands/event_trigger.h" #include "commands/tablespace.h" #include "commands/trigger.h" @@ -4314,6 +4315,18 @@ struct config_string ConfigureNamesString[] = NULL, NULL, NULL }, + { + {"extension_control_path", PGC_SUSET, CLIENT_CONN_OTHER, + gettext_noop("Sets the path for extension control files."), + gettext_noop("The remaining extension script and secondary control files are then loaded " + "from the same directory where the primary control file was found."), + GUC_SUPERUSER_ONLY + }, + &Extension_control_path, + "$system", + NULL, NULL, NULL + }, + { {"krb_server_keyfile", PGC_SIGHUP, CONN_AUTH_AUTH, gettext_noop("Sets the location of the Kerberos server key file."), diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample index 2d1de9c37bd..d22ef6ef47b 100644 --- a/src/backend/utils/misc/postgresql.conf.sample +++ b/src/backend/utils/misc/postgresql.conf.sample @@ -791,6 +791,7 @@ autovacuum_worker_slots = 16 # autovacuum worker slots to allocate # - Other Defaults - #dynamic_library_path = '$libdir' +#extension_control_path = '$system' #gin_fuzzy_search_limit = 0 diff --git a/src/include/commands/extension.h b/src/include/commands/extension.h index 0b636405120..24419bfb5c9 100644 --- a/src/include/commands/extension.h +++ b/src/include/commands/extension.h @@ -17,6 +17,8 @@ #include "catalog/objectaddress.h" #include "parser/parse_node.h" +/* GUC */ +extern PGDLLIMPORT char *Extension_control_path; /* * creating_extension is only true while running a CREATE EXTENSION or ALTER diff --git a/src/include/fmgr.h b/src/include/fmgr.h index e609ea875a7..442c50d6b90 100644 --- a/src/include/fmgr.h +++ b/src/include/fmgr.h @@ -740,6 +740,8 @@ extern bool CheckFunctionValidatorAccess(Oid validatorOid, Oid functionOid); */ extern PGDLLIMPORT char *Dynamic_library_path; +extern char *find_in_path(const char *basename, const char *path, const char *path_param, + const char *macro, const char *macro_val); extern void *load_external_function(const char *filename, const char *funcname, bool signalNotFound, void **filehandle); extern void *lookup_external_function(void *filehandle, const char *funcname); @@ -749,6 +751,9 @@ extern Size EstimateLibraryStateSpace(void); extern void SerializeLibraryState(Size maxsize, char *start_address); extern void RestoreLibraryState(char *start_address); +extern char * +substitute_path_macro(const char *str, const char *macro, const char *value); + /* * Support for aggregate functions * diff --git a/src/test/modules/test_extensions/Makefile b/src/test/modules/test_extensions/Makefile index 1dbec14cba3..a3591bf3d2f 100644 --- a/src/test/modules/test_extensions/Makefile +++ b/src/test/modules/test_extensions/Makefile @@ -28,6 +28,7 @@ DATA = test_ext1--1.0.sql test_ext2--1.0.sql test_ext3--1.0.sql \ test_ext_req_schema3--1.0.sql REGRESS = test_extensions test_extdepend +TAP_TESTS = 1 # force C locale for output stability NO_LOCALE = 1 diff --git a/src/test/modules/test_extensions/meson.build b/src/test/modules/test_extensions/meson.build index dd7ec0ce56b..3c7e378bf35 100644 --- a/src/test/modules/test_extensions/meson.build +++ b/src/test/modules/test_extensions/meson.build @@ -57,4 +57,9 @@ tests += { ], 'regress_args': ['--no-locale'], }, + 'tap': { + 'tests': [ + 't/001_extension_control_path.pl', + ], + }, } diff --git a/src/test/modules/test_extensions/t/001_extension_control_path.pl b/src/test/modules/test_extensions/t/001_extension_control_path.pl new file mode 100644 index 00000000000..19c9ed9748b --- /dev/null +++ b/src/test/modules/test_extensions/t/001_extension_control_path.pl @@ -0,0 +1,77 @@ +# Copyright (c) 2024-2025, PostgreSQL Global Development Group + +use strict; +use warnings FATAL => 'all'; +use PostgreSQL::Test::Utils; +use PostgreSQL::Test::Cluster; +use Test::More; + +my $node = PostgreSQL::Test::Cluster->new('node'); + +$node->init; + +# Create a temporary directory for the extension control file +my $ext_dir = PostgreSQL::Test::Utils::tempdir(); +my $ext_name = "test_custom_ext_paths"; +my $control_file = "$ext_dir/$ext_name.control"; +my $sql_file = "$ext_dir/$ext_name--1.0.sql"; + +# Create .control .sql file +open my $cf, '>', $control_file or die "Could not create control file: $!"; +print $cf "comment = 'Test extension_control_path'\n"; +print $cf "default_version = '1.0'\n"; +print $cf "relocatable = true\n"; +close $cf; + +# Create --1.0.sql file +open my $sqlf, '>', $sql_file or die "Could not create sql file: $!"; +print $sqlf "/* $sql_file */\n"; +print $sqlf "-- complain if script is sourced in psql, rather than via CREATE EXTENSION\n"; +print $sqlf qq'\\echo Use "CREATE EXTENSION $ext_name" to load this file. \\quit\n'; +close $sqlf; + +# Use the correct separator and escape \ when running on Windows. +my $sep = $windows_os ? ";" : ":"; +$node->append_conf( + 'postgresql.conf', qq{ +extension_control_path = '\$system$sep@{[ $windows_os ? ($ext_dir =~ s/\\/\\\\/gr) : $ext_dir ]}' +}); + +# Start node +$node->start; + +my $ecp = $node->safe_psql('postgres', 'show extension_control_path;'); + +is($ecp, "\$system$sep$ext_dir", "Custom extension control directory path configured"); + +$node->safe_psql( + 'postgres', + "CREATE EXTENSION $ext_name"); + +my $ret = $node->safe_psql( + 'postgres', + "select * from pg_available_extensions where name = '$ext_name'"); +is( + $ret, + "test_custom_ext_paths|1.0|1.0|Test extension_control_path", + "Extension is installed correctly on pg_available_extensions"); + +my $ret2 = $node->safe_psql( + 'postgres', + "select * from pg_available_extension_versions where name = '$ext_name'"); +is( + $ret2, + "test_custom_ext_paths|1.0|t|t|f|t|||Test extension_control_path", + "Extension is installed correctly on pg_available_extension_versions"); + +# Ensure that extensions installed on $system is still visible when using with +# custom extension control path. +my $ret3 = $node->safe_psql( + 'postgres', + "select count(*) > 0 as ok from pg_available_extensions where name = 'amcheck'"); +is( + $ret3, + "t", + "\$system extension is installed correctly on pg_available_extensions"); + +done_testing(); -- 2.39.5 (Apple Git-154) ^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: RFC: Additional Directory for Extensions @ 2025-03-11 15:58 Peter Eisentraut <[email protected]> parent: Matheus Alcantara <[email protected]> 0 siblings, 1 reply; 31+ messages in thread From: Peter Eisentraut @ 2025-03-11 15:58 UTC (permalink / raw) To: Matheus Alcantara <[email protected]>; +Cc: Andrew Dunstan <[email protected]>; Gabriele Bartolini <[email protected]>; Craig Ringer <[email protected]>; David E. Wheeler <[email protected]>; Robert Haas <[email protected]>; Alvaro Herrera <[email protected]>; pgsql-hackers On 10.03.25 21:25, Matheus Alcantara wrote: > On Thu, Mar 6, 2025 at 10:46 AM Peter Eisentraut <[email protected]> wrote: >> This looks very good to me. I have one issue to point out: The logic >> in get_extension_control_directories() needs to be a little bit more >> careful to align with the rules in find_in_path(). For example, it >> should use first_path_var_separator() to get the platform-specific path >> separator, and probably also substitute_path_macro() and >> canonicalize_path() etc., to keep everything consistent. >> > I fixed this hardcoded path separator issue on the TAP test and forgot > to fix it also on code, sorry, fixed on this new version. >> (Maybe it would be ok to move the function to dfmgr.c to avoid having >> to export too many things from there.) >> > I've exported substitute_path_macro because adding a new function on > dfmgr would require #include nodes/pg_list.h and I'm not sure what > approach would be better, please let me know what you think. Yes, that structure looks ok. But you can remove one level of block in get_extension_control_directories(). I found a bug that was already present in my earlier patch versions: @@ -423,7 +424,7 @@ find_extension_control_filename(ExtensionControlFile *control) ecp = Extension_control_path; if (strlen(ecp) == 0) ecp = "$system"; - result = find_in_path(basename, Extension_control_path, "extension_control_path", "$system", system_dir); + result = find_in_path(basename, ecp, "extension_control_path", "$system", system_dir); Without this, it won't work if you set extension_control_path empty. (Maybe add a test for that?) I think this all works now, but I think the way pg_available_extensions() works is a bit strange and inefficient. After it finds a candidate control file, it calls read_extension_control_file() with the extension name, that calls parse_extension_control_file(), that calls find_extension_control_filename(), and that calls find_in_path(), which searches that path again! There should be a simpler way into this. Maybe pg_available_extensions() should fill out the ExtensionControlFile structure itself, set ->control_dir with where it found it, then call directly to parse_extension_control_file(), and that should skip the finding if the directory is already set. Or something similar. ^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: RFC: Additional Directory for Extensions @ 2025-03-12 13:17 Matheus Alcantara <[email protected]> parent: Peter Eisentraut <[email protected]> 0 siblings, 2 replies; 31+ messages in thread From: Matheus Alcantara @ 2025-03-12 13:17 UTC (permalink / raw) To: Peter Eisentraut <[email protected]>; +Cc: Andrew Dunstan <[email protected]>; Gabriele Bartolini <[email protected]>; Craig Ringer <[email protected]>; David E. Wheeler <[email protected]>; Robert Haas <[email protected]>; Alvaro Herrera <[email protected]>; pgsql-hackers On Tue, Mar 11, 2025 at 12:59 PM Peter Eisentraut <[email protected]> wrote: > Yes, that structure looks ok. But you can remove one level of block in > get_extension_control_directories(). > Sorry, missed during debugging. Fixed > I found a bug that was already present in my earlier patch versions: > > @@ -423,7 +424,7 @@ find_extension_control_filename(ExtensionControlFile > *control) > ecp = Extension_control_path; > if (strlen(ecp) == 0) > ecp = "$system"; > - result = find_in_path(basename, Extension_control_path, > "extension_control_path", "$system", system_dir); > + result = find_in_path(basename, ecp, "extension_control_path", > "$system", system_dir); > > Without this, it won't work if you set extension_control_path empty. > (Maybe add a test for that?) > Fixed, and also added a new test case for this. > I think this all works now, but I think the way > pg_available_extensions() works is a bit strange and inefficient. After > it finds a candidate control file, it calls > read_extension_control_file() with the extension name, that calls > parse_extension_control_file(), that calls > find_extension_control_filename(), and that calls find_in_path(), which > searches that path again! > > There should be a simpler way into this. Maybe > pg_available_extensions() should fill out the ExtensionControlFile > structure itself, set ->control_dir with where it found it, then call > directly to parse_extension_control_file(), and that should skip the > finding if the directory is already set. Or something similar. > Good catch. I fixed this by creating a new function to construct the ExtensionControlFile and changed the pg_available_extensions to set the control_dir. The read_extension_control_file was also changed to just call this new function constructor. I implemented the logic to check if the control_dir is already set on parse_extension_control_file because it seems to me that make more sense to not call find_extension_control_filename instead of putting this logic there since we already set the control_dir when we find the control file, and having the logic to set the control_dir or skip the find_in_path seems more confusing on this function instead of on parse_extension_control_file. Please let me know what you think. -- Matheus Alcantara Attachments: [application/octet-stream] v7-0001-extension_control_path.patch (38.3K, ../../CAFY6G8d56BK5TZ7K+uw4DHLZ=9Th3p+E1x4pp26nJ_K5k4EUwA@mail.gmail.com/2-v7-0001-extension_control_path.patch) download | inline diff: From a0e8ae7af182cf5f37442b85c42b9c40d84419bb Mon Sep 17 00:00:00 2001 From: Peter Eisentraut <[email protected]> Date: Thu, 5 Dec 2024 11:49:05 +0100 Subject: [PATCH v7] extension_control_path The new GUC extension_control_path specifies a path to look for extension control files. The default value is $system, which looks in the compiled-in location, as before. The path search uses the same code and works in the same way as dynamic_library_path. Discussion: https://www.postgresql.org/message-id/flat/[email protected] --- doc/src/sgml/config.sgml | 68 +++ doc/src/sgml/extend.sgml | 19 +- doc/src/sgml/ref/create_extension.sgml | 6 +- src/Makefile.global.in | 19 +- src/backend/commands/extension.c | 416 ++++++++++++------ src/backend/utils/fmgr/dfmgr.c | 77 ++-- src/backend/utils/misc/guc_tables.c | 13 + src/backend/utils/misc/postgresql.conf.sample | 1 + src/include/commands/extension.h | 2 + src/include/fmgr.h | 5 + src/test/modules/test_extensions/Makefile | 1 + src/test/modules/test_extensions/meson.build | 5 + .../t/001_extension_control_path.pl | 86 ++++ 13 files changed, 535 insertions(+), 183 deletions(-) create mode 100644 src/test/modules/test_extensions/t/001_extension_control_path.pl diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index d2fa5f7d1a9..9fec78db6f7 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -10725,6 +10725,74 @@ dynamic_library_path = 'C:\tools\postgresql;H:\my_project\lib;$libdir' </listitem> </varlistentry> + <varlistentry id="guc-extension-control-path" xreflabel="extension_control_path"> + <term><varname>extension_control_path</varname> (<type>string</type>) + <indexterm> + <primary><varname>extension_control_path</varname> configuration parameter</primary> + </indexterm> + </term> + <listitem> + <para> + A path to search for extensions, specifically extension control files + (<filename><replaceable>name</replaceable>.control</filename>). The + remaining extension script and secondary control files are then loaded + from the same directory where the primary control file was found. + See <xref linkend="extend-extensions-files"/> for details. + </para> + + <para> + The value for <varname>extension_control_path</varname> must be a + list of absolute directory paths separated by colons (or semi-colons + on Windows). If a list element starts + with the special string <literal>$system</literal>, the + compiled-in <productname>PostgreSQL</productname> extension + directory is substituted for <literal>$system</literal>; this + is where the extensions provided by the standard + <productname>PostgreSQL</productname> distribution are installed. + (Use <literal>pg_config --sharedir</literal> to find out the name of + this directory.) For example: +<programlisting> +extension_control_path = '/usr/local/share/postgresql/extension:/home/my_project/share/extension:$system' +</programlisting> + or, in a Windows environment: +<programlisting> +extension_control_path = 'C:\tools\postgresql\extension;H:\my_project\share\extension;$system' +</programlisting> + Note that the path elements should typically end in + <literal>extension</literal> if the normal installation layouts are + followed. (The value for <literal>$system</literal> already includes + the <literal>extension</literal> suffix.) + </para> + + <para> + The default value for this parameter is + <literal>'$system'</literal>. If the value is set to an empty + string, the default <literal>'$system'</literal> is also assumed. + </para> + + <para> + This parameter can be changed at run time by superusers and users + with the appropriate <literal>SET</literal> privilege, but a + setting done that way will only persist until the end of the + client connection, so this method should be reserved for + development purposes. The recommended way to set this parameter + is in the <filename>postgresql.conf</filename> configuration + file. + </para> + + <para> + Note that if you set this parameter to be able to load extensions from + nonstandard locations, you will most likely also need to set <xref + linkend="guc-dynamic-library-path"/> to a correspondent location, for + example, +<programlisting> +extension_control_path = '/usr/local/share/postgresql/extension:$system' +dynamic_library_path = '/usr/local/lib/postgresql:$libdir' +</programlisting> + </para> + </listitem> + </varlistentry> + <varlistentry id="guc-gin-fuzzy-search-limit" xreflabel="gin_fuzzy_search_limit"> <term><varname>gin_fuzzy_search_limit</varname> (<type>integer</type>) <indexterm> diff --git a/doc/src/sgml/extend.sgml b/doc/src/sgml/extend.sgml index ba492ca27c0..64f8e133cae 100644 --- a/doc/src/sgml/extend.sgml +++ b/doc/src/sgml/extend.sgml @@ -649,6 +649,11 @@ RETURNS anycompatible AS ... control file can specify a different directory for the script file(s). </para> + <para> + Additional locations for extension control files can be configured using + the parameter <xref linkend="guc-extension-control-path"/>. + </para> + <para> The file format for an extension control file is the same as for the <filename>postgresql.conf</filename> file, namely a list of @@ -669,9 +674,9 @@ RETURNS anycompatible AS ... <para> The directory containing the extension's <acronym>SQL</acronym> script file(s). Unless an absolute path is given, the name is relative to - the installation's <literal>SHAREDIR</literal> directory. The - default behavior is equivalent to specifying - <literal>directory = 'extension'</literal>. + the installation's <literal>SHAREDIR</literal> directory. By default, + the script files are looked for in the same directory where the + control file was found. </para> </listitem> </varlistentry> @@ -719,8 +724,8 @@ RETURNS anycompatible AS ... <para> The value of this parameter will be substituted for each occurrence of <literal>MODULE_PATHNAME</literal> in the script file(s). If it is not - set, no substitution is made. Typically, this is set to - <literal>$libdir/<replaceable>shared_library_name</replaceable></literal> and + set, no substitution is made. Typically, this is set to just + <literal><replaceable>shared_library_name</replaceable></literal> and then <literal>MODULE_PATHNAME</literal> is used in <command>CREATE FUNCTION</command> commands for C-language functions, so that the script files do not need to hard-wire the name of the shared library. @@ -1804,6 +1809,10 @@ include $(PGXS) setting <varname>PG_CONFIG</varname> to point to its <command>pg_config</command> program, either within the makefile or on the <literal>make</literal> command line. + You can also select a separate installation directory for your extension + by setting the <literal>make</literal> variable <varname>prefix</varname> + on the <literal>make</literal> command line. (But this will then require + additional setup to get the server to find the extension there.) </para> <para> diff --git a/doc/src/sgml/ref/create_extension.sgml b/doc/src/sgml/ref/create_extension.sgml index ca2b80d669c..713abd9c494 100644 --- a/doc/src/sgml/ref/create_extension.sgml +++ b/doc/src/sgml/ref/create_extension.sgml @@ -90,8 +90,10 @@ CREATE EXTENSION [ IF NOT EXISTS ] <replaceable class="parameter">extension_name <para> The name of the extension to be installed. <productname>PostgreSQL</productname> will create the - extension using details from the file - <literal>SHAREDIR/extension/</literal><replaceable class="parameter">extension_name</replaceable><literal>.control</literal>. + extension using details from the file <filename><replaceable + class="parameter">extension_name</replaceable>.control</filename>, + found via the server's extension control path (set by <xref + linkend="guc-extension-control-path"/>.) </para> </listitem> </varlistentry> diff --git a/src/Makefile.global.in b/src/Makefile.global.in index 3b620bac5ac..8fe9d61e82a 100644 --- a/src/Makefile.global.in +++ b/src/Makefile.global.in @@ -87,9 +87,19 @@ endif # not PGXS # # In a PGXS build, we cannot use the values inserted into Makefile.global # by configure, since the installation tree may have been relocated. -# Instead get the path values from pg_config. +# Instead get the path values from pg_config. But users can specify +# prefix explicitly, if they want to select their own installation +# location. -ifndef PGXS +ifdef PGXS +# Extension makefiles should set PG_CONFIG, but older ones might not +ifndef PG_CONFIG +PG_CONFIG = pg_config +endif +endif + +# This means: if ((not PGXS) or prefix) +ifneq (,$(if $(PGXS),,1)$(prefix)) # Note that prefix, exec_prefix, and datarootdir aren't defined in a PGXS build; # makefiles may only use the derived variables such as bindir. @@ -147,11 +157,6 @@ localedir := @localedir@ else # PGXS case -# Extension makefiles should set PG_CONFIG, but older ones might not -ifndef PG_CONFIG -PG_CONFIG = pg_config -endif - bindir := $(shell $(PG_CONFIG) --bindir) datadir := $(shell $(PG_CONFIG) --sharedir) sysconfdir := $(shell $(PG_CONFIG) --sysconfdir) diff --git a/src/backend/commands/extension.c b/src/backend/commands/extension.c index d9bb4ce5f1e..aa45f4810b8 100644 --- a/src/backend/commands/extension.c +++ b/src/backend/commands/extension.c @@ -54,6 +54,7 @@ #include "funcapi.h" #include "mb/pg_wchar.h" #include "miscadmin.h" +#include "nodes/pg_list.h" #include "nodes/queryjumble.h" #include "storage/fd.h" #include "tcop/utility.h" @@ -69,6 +70,9 @@ #include "utils/varlena.h" +/* GUC */ +char *Extension_control_path; + /* Globally visible state variables */ bool creating_extension = false; Oid CurrentExtensionObject = InvalidOid; @@ -79,6 +83,7 @@ Oid CurrentExtensionObject = InvalidOid; typedef struct ExtensionControlFile { char *name; /* name of the extension */ + char *control_dir; /* directory where control file was found */ char *directory; /* directory for script files */ char *default_version; /* default install target version, if any */ char *module_pathname; /* string to substitute for @@ -147,6 +152,8 @@ static void ExecAlterExtensionContentsRecurse(AlterExtensionContentsStmt *stmt, ObjectAddress object); static char *read_whole_file(const char *filename, int *length); +static ExtensionControlFile *new_ExtensionControlFile(const char *extname); + /* * get_extension_oid - given an extension name, look up the OID @@ -328,29 +335,104 @@ is_extension_script_filename(const char *filename) return (extension != NULL) && (strcmp(extension, ".sql") == 0); } -static char * -get_extension_control_directory(void) +/* + * Return a list of directories declared on extension_control_path GUC. + */ +static List * +get_extension_control_directories(void) { char sharepath[MAXPGPATH]; - char *result; + char *system_dir; + char *ecp; + List *paths = NIL; get_share_path(my_exec_path, sharepath); - result = (char *) palloc(MAXPGPATH); - snprintf(result, MAXPGPATH, "%s/extension", sharepath); - return result; + system_dir = psprintf("%s/extension", sharepath); + + if (strlen(Extension_control_path) == 0) + { + paths = lappend(paths, system_dir); + } + else + { + /* Duplicate the string so we can modify it */ + ecp = pstrdup(Extension_control_path); + + for (;;) + { + int len; + char *mangled; + char *piece = first_path_var_separator(ecp); + + /* Get the length of the next path on ecp */ + if (piece == NULL) + len = strlen(ecp); + else + len = piece - ecp; + + /* Copy the next path found on ecp */ + piece = palloc(len + 1); + strlcpy(piece, ecp, len + 1); + + /* Substitute the path macro if needed */ + mangled = substitute_path_macro(piece, "$system", system_dir); + pfree(piece); + + /* Canonicalize the path based on the OS and add to the list */ + canonicalize_path(mangled); + paths = lappend(paths, mangled); + + /* Break if ecp is empty or move to the next path on ecp */ + if (ecp[len] == '\0') + break; + else + ecp += len + 1; + } + } + + return paths; } +/* + * Find control file for extension with name in control->name, looking in the + * path. Return the full file name, or NULL if not found. If found, the + * directory is recorded in control->control_dir. + */ static char * -get_extension_control_filename(const char *extname) +find_extension_control_filename(ExtensionControlFile *control) { char sharepath[MAXPGPATH]; + char *system_dir; + char *basename; + char *ecp; char *result; + Assert(control->name); + get_share_path(my_exec_path, sharepath); - result = (char *) palloc(MAXPGPATH); - snprintf(result, MAXPGPATH, "%s/extension/%s.control", - sharepath, extname); + system_dir = psprintf("%s/extension", sharepath); + + basename = psprintf("%s.control", control->name); + + /* + * find_in_path() does nothing if the path value is empty. This is the + * historical behavior for dynamic_library_path, but it makes no sense for + * extensions. So in that case, substitute a default value. + */ + ecp = Extension_control_path; + if (strlen(ecp) == 0) + ecp = "$system"; + result = find_in_path(basename, ecp, "extension_control_path", "$system", system_dir); + + if (result) + { + const char *p; + + p = strrchr(result, '/'); + Assert(p); + control->control_dir = pnstrdup(result, p - result); + } return result; } @@ -366,7 +448,7 @@ get_extension_script_directory(ExtensionControlFile *control) * installation's share directory. */ if (!control->directory) - return get_extension_control_directory(); + return pstrdup(control->control_dir); if (is_absolute_path(control->directory)) return pstrdup(control->directory); @@ -424,6 +506,11 @@ get_extension_script_filename(ExtensionControlFile *control, * fields of *control. We parse primary file if version == NULL, * else the optional auxiliary file for that version. * + * The control file will be search on Extension_control_path paths if + * control->control_dir is NULL, otherwise it will use the value of control_dir + * to read and parse the .control file, so it assume that the control_dir is a + * valid path for the control file being parsed. + * * Control files are supposed to be very short, half a dozen lines, * so we don't worry about memory allocation risks here. Also we don't * worry about what encoding it's in; all values are expected to be ASCII. @@ -444,27 +531,52 @@ parse_extension_control_file(ExtensionControlFile *control, if (version) filename = get_extension_aux_control_filename(control, version); else - filename = get_extension_control_filename(control->name); - - if ((file = AllocateFile(filename, "r")) == NULL) { - if (errno == ENOENT) + /* + * Skip searching if control_dir is already set. We assume that + * control_dir is set correctly to find the .control file, otherwise + * ereport extension not available error. + */ + if (control->control_dir != NULL) { - /* no complaint for missing auxiliary file */ - if (version) + /* + * Don't forget to consider path separator, .control suffix and + * null terminator. + */ + filename = palloc(strlen(control->control_dir) + 1 + strlen(control->name) + 8 + 1); + sprintf(filename, "%s/%s.control", control->control_dir, control->name); + + if (!pg_file_exists(filename)) { + /* + * Extension is not available. Free the memory and set to NULL + * for ereporting. + */ pfree(filename); - return; + filename = NULL; } + } + else + filename = find_extension_control_filename(control); + } - /* missing control file indicates extension is not installed */ - ereport(ERROR, - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), - errmsg("extension \"%s\" is not available", control->name), - errdetail("Could not open extension control file \"%s\": %m.", - filename), - errhint("The extension must first be installed on the system where PostgreSQL is running."))); + if (!filename) + { + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("extension \"%s\" is not available", control->name), + errhint("The extension must first be installed on the system where PostgreSQL is running."))); + } + + if ((file = AllocateFile(filename, "r")) == NULL) + { + /* no complaint for missing auxiliary file */ + if (errno == ENOENT && version) + { + pfree(filename); + return; } + ereport(ERROR, (errcode_for_file_access(), errmsg("could not open extension control file \"%s\": %m", @@ -603,17 +715,7 @@ parse_extension_control_file(ExtensionControlFile *control, static ExtensionControlFile * read_extension_control_file(const char *extname) { - ExtensionControlFile *control; - - /* - * Set up default values. Pointer fields are initially null. - */ - control = (ExtensionControlFile *) palloc0(sizeof(ExtensionControlFile)); - control->name = pstrdup(extname); - control->relocatable = false; - control->superuser = true; - control->trusted = false; - control->encoding = -1; + ExtensionControlFile *control = new_ExtensionControlFile(extname); /* * Parse the primary control file. @@ -2121,68 +2223,75 @@ Datum pg_available_extensions(PG_FUNCTION_ARGS) { ReturnSetInfo *rsinfo = (ReturnSetInfo *) fcinfo->resultinfo; - char *location; + List *locations; DIR *dir; struct dirent *de; /* Build tuplestore to hold the result rows */ InitMaterializedSRF(fcinfo, 0); - location = get_extension_control_directory(); - dir = AllocateDir(location); + locations = get_extension_control_directories(); - /* - * If the control directory doesn't exist, we want to silently return an - * empty set. Any other error will be reported by ReadDir. - */ - if (dir == NULL && errno == ENOENT) - { - /* do nothing */ - } - else + foreach_ptr(char, location, locations) { - while ((de = ReadDir(dir, location)) != NULL) + dir = AllocateDir(location); + + /* + * If the control directory doesn't exist, we want to silently return + * an empty set. Any other error will be reported by ReadDir. + */ + if (dir == NULL && errno == ENOENT) { - ExtensionControlFile *control; - char *extname; - Datum values[3]; - bool nulls[3]; + /* do nothing */ + } + else + { + while ((de = ReadDir(dir, location)) != NULL) + { + ExtensionControlFile *control; + char *extname; + Datum values[3]; + bool nulls[3]; - if (!is_extension_control_filename(de->d_name)) - continue; + if (!is_extension_control_filename(de->d_name)) + continue; - /* extract extension name from 'name.control' filename */ - extname = pstrdup(de->d_name); - *strrchr(extname, '.') = '\0'; + /* extract extension name from 'name.control' filename */ + extname = pstrdup(de->d_name); + *strrchr(extname, '.') = '\0'; - /* ignore it if it's an auxiliary control file */ - if (strstr(extname, "--")) - continue; + /* ignore it if it's an auxiliary control file */ + if (strstr(extname, "--")) + continue; - control = read_extension_control_file(extname); + control = new_ExtensionControlFile(extname); + control->control_dir = pstrdup(location); - memset(values, 0, sizeof(values)); - memset(nulls, 0, sizeof(nulls)); + parse_extension_control_file(control, NULL); - /* name */ - values[0] = DirectFunctionCall1(namein, - CStringGetDatum(control->name)); - /* default_version */ - if (control->default_version == NULL) - nulls[1] = true; - else - values[1] = CStringGetTextDatum(control->default_version); - /* comment */ - if (control->comment == NULL) - nulls[2] = true; - else - values[2] = CStringGetTextDatum(control->comment); + memset(values, 0, sizeof(values)); + memset(nulls, 0, sizeof(nulls)); - tuplestore_putvalues(rsinfo->setResult, rsinfo->setDesc, - values, nulls); - } + /* name */ + values[0] = DirectFunctionCall1(namein, + CStringGetDatum(control->name)); + /* default_version */ + if (control->default_version == NULL) + nulls[1] = true; + else + values[1] = CStringGetTextDatum(control->default_version); + /* comment */ + if (control->comment == NULL) + nulls[2] = true; + else + values[2] = CStringGetTextDatum(control->comment); + + tuplestore_putvalues(rsinfo->setResult, rsinfo->setDesc, + values, nulls); + } - FreeDir(dir); + FreeDir(dir); + } } return (Datum) 0; @@ -2201,51 +2310,55 @@ Datum pg_available_extension_versions(PG_FUNCTION_ARGS) { ReturnSetInfo *rsinfo = (ReturnSetInfo *) fcinfo->resultinfo; - char *location; + List *locations; DIR *dir; struct dirent *de; /* Build tuplestore to hold the result rows */ InitMaterializedSRF(fcinfo, 0); - location = get_extension_control_directory(); - dir = AllocateDir(location); + locations = get_extension_control_directories(); - /* - * If the control directory doesn't exist, we want to silently return an - * empty set. Any other error will be reported by ReadDir. - */ - if (dir == NULL && errno == ENOENT) + foreach_ptr(char, location, locations) { - /* do nothing */ - } - else - { - while ((de = ReadDir(dir, location)) != NULL) + dir = AllocateDir(location); + + /* + * If the control directory doesn't exist, we want to silently return + * an empty set. Any other error will be reported by ReadDir. + */ + if (dir == NULL && errno == ENOENT) { - ExtensionControlFile *control; - char *extname; + /* do nothing */ + } + else + { + while ((de = ReadDir(dir, location)) != NULL) + { + ExtensionControlFile *control; + char *extname; - if (!is_extension_control_filename(de->d_name)) - continue; + if (!is_extension_control_filename(de->d_name)) + continue; - /* extract extension name from 'name.control' filename */ - extname = pstrdup(de->d_name); - *strrchr(extname, '.') = '\0'; + /* extract extension name from 'name.control' filename */ + extname = pstrdup(de->d_name); + *strrchr(extname, '.') = '\0'; - /* ignore it if it's an auxiliary control file */ - if (strstr(extname, "--")) - continue; + /* ignore it if it's an auxiliary control file */ + if (strstr(extname, "--")) + continue; - /* read the control file */ - control = read_extension_control_file(extname); + /* read the control file */ + control = read_extension_control_file(extname); - /* scan extension's script directory for install scripts */ - get_available_versions_for_extension(control, rsinfo->setResult, - rsinfo->setDesc); - } + /* scan extension's script directory for install scripts */ + get_available_versions_for_extension(control, rsinfo->setResult, + rsinfo->setDesc); + } - FreeDir(dir); + FreeDir(dir); + } } return (Datum) 0; @@ -2373,47 +2486,53 @@ bool extension_file_exists(const char *extensionName) { bool result = false; - char *location; + List *locations; DIR *dir; struct dirent *de; - location = get_extension_control_directory(); - dir = AllocateDir(location); + locations = get_extension_control_directories(); - /* - * If the control directory doesn't exist, we want to silently return - * false. Any other error will be reported by ReadDir. - */ - if (dir == NULL && errno == ENOENT) + foreach_ptr(char, location, locations) { - /* do nothing */ - } - else - { - while ((de = ReadDir(dir, location)) != NULL) + dir = AllocateDir(location); + + /* + * If the control directory doesn't exist, we want to silently return + * false. Any other error will be reported by ReadDir. + */ + if (dir == NULL && errno == ENOENT) { - char *extname; + /* do nothing */ + } + else + { + while ((de = ReadDir(dir, location)) != NULL) + { + char *extname; - if (!is_extension_control_filename(de->d_name)) - continue; + if (!is_extension_control_filename(de->d_name)) + continue; - /* extract extension name from 'name.control' filename */ - extname = pstrdup(de->d_name); - *strrchr(extname, '.') = '\0'; + /* extract extension name from 'name.control' filename */ + extname = pstrdup(de->d_name); + *strrchr(extname, '.') = '\0'; - /* ignore it if it's an auxiliary control file */ - if (strstr(extname, "--")) - continue; + /* ignore it if it's an auxiliary control file */ + if (strstr(extname, "--")) + continue; - /* done if it matches request */ - if (strcmp(extname, extensionName) == 0) - { - result = true; - break; + /* done if it matches request */ + if (strcmp(extname, extensionName) == 0) + { + result = true; + break; + } } - } - FreeDir(dir); + FreeDir(dir); + } + if (result) + break; } return result; @@ -3691,3 +3810,20 @@ read_whole_file(const char *filename, int *length) *length = bytes_to_read; return buf; } + +static ExtensionControlFile * +new_ExtensionControlFile(const char *extname) +{ + /* + * Set up default values. Pointer fields are initially null. + */ + ExtensionControlFile *control = (ExtensionControlFile *) palloc0(sizeof(ExtensionControlFile)); + + control->name = pstrdup(extname); + control->relocatable = false; + control->superuser = true; + control->trusted = false; + control->encoding = -1; + + return control; +} diff --git a/src/backend/utils/fmgr/dfmgr.c b/src/backend/utils/fmgr/dfmgr.c index 87b233cb887..ca12e954ea2 100644 --- a/src/backend/utils/fmgr/dfmgr.c +++ b/src/backend/utils/fmgr/dfmgr.c @@ -71,8 +71,6 @@ static void incompatible_module_error(const char *libname, const Pg_magic_struct *module_magic_data) pg_attribute_noreturn(); static char *expand_dynamic_library_name(const char *name); static void check_restricted_library_name(const char *name); -static char *substitute_libpath_macro(const char *name); -static char *find_in_dynamic_libpath(const char *basename); /* Magic structure that module needs to match to be accepted */ static const Pg_magic_struct magic_data = PG_MODULE_MAGIC_DATA; @@ -398,7 +396,7 @@ incompatible_module_error(const char *libname, /* * If name contains a slash, check if the file exists, if so return * the name. Else (no slash) try to expand using search path (see - * find_in_dynamic_libpath below); if that works, return the fully + * find_in_path below); if that works, return the fully * expanded file name. If the previous failed, append DLSUFFIX and * try again. If all fails, just return the original name. * @@ -413,17 +411,25 @@ expand_dynamic_library_name(const char *name) Assert(name); + /* + * If the value starts with "$libdir/", strip that. This is because many + * extensions have hardcoded '$libdir/foo' as their library name, which + * prevents using the path. + */ + if (strncmp(name, "$libdir/", 8) == 0) + name += 8; + have_slash = (first_dir_separator(name) != NULL); if (!have_slash) { - full = find_in_dynamic_libpath(name); + full = find_in_path(name, Dynamic_library_path, "dynamic_library_path", "$libdir", pkglib_path); if (full) return full; } else { - full = substitute_libpath_macro(name); + full = substitute_path_macro(name, "$libdir", pkglib_path); if (pg_file_exists(full)) return full; pfree(full); @@ -433,14 +439,14 @@ expand_dynamic_library_name(const char *name) if (!have_slash) { - full = find_in_dynamic_libpath(new); + full = find_in_path(new, Dynamic_library_path, "dynamic_library_path", "$libdir", pkglib_path); pfree(new); if (full) return full; } else { - full = substitute_libpath_macro(new); + full = substitute_path_macro(new, "$libdir", pkglib_path); pfree(new); if (pg_file_exists(full)) return full; @@ -474,48 +480,61 @@ check_restricted_library_name(const char *name) * Substitute for any macros appearing in the given string. * Result is always freshly palloc'd. */ -static char * -substitute_libpath_macro(const char *name) +char * +substitute_path_macro(const char *str, const char *macro, const char *value) { const char *sep_ptr; - Assert(name != NULL); + Assert(str != NULL); + Assert(macro[0] == '$'); - /* Currently, we only recognize $libdir at the start of the string */ - if (name[0] != '$') - return pstrdup(name); + /* Currently, we only recognize $macro at the start of the string */ + if (str[0] != '$') + return pstrdup(str); - if ((sep_ptr = first_dir_separator(name)) == NULL) - sep_ptr = name + strlen(name); + if ((sep_ptr = first_dir_separator(str)) == NULL) + sep_ptr = str + strlen(str); - if (strlen("$libdir") != sep_ptr - name || - strncmp(name, "$libdir", strlen("$libdir")) != 0) + if (strlen(macro) != sep_ptr - str || + strncmp(str, macro, strlen(macro)) != 0) ereport(ERROR, (errcode(ERRCODE_INVALID_NAME), - errmsg("invalid macro name in dynamic library path: %s", - name))); + errmsg("invalid macro name in path: %s", + str))); - return psprintf("%s%s", pkglib_path, sep_ptr); + return psprintf("%s%s", value, sep_ptr); } /* * Search for a file called 'basename' in the colon-separated search - * path Dynamic_library_path. If the file is found, the full file name + * path given. If the file is found, the full file name * is returned in freshly palloc'd memory. If the file is not found, * return NULL. + * + * path_param is the name of the parameter that path came from, for error + * messages. + * + * macro and macro_val allow substituting a macro; see + * substitute_path_macro(). */ -static char * -find_in_dynamic_libpath(const char *basename) +char * +find_in_path(const char *basename, const char *path, const char *path_param, + const char *macro, const char *macro_val) { const char *p; size_t baselen; Assert(basename != NULL); Assert(first_dir_separator(basename) == NULL); - Assert(Dynamic_library_path != NULL); + Assert(path != NULL); + Assert(path_param != NULL); + + p = path; - p = Dynamic_library_path; + /* + * If the path variable is empty, don't do a path search. + */ if (strlen(p) == 0) return NULL; @@ -532,7 +551,7 @@ find_in_dynamic_libpath(const char *basename) if (piece == p) ereport(ERROR, (errcode(ERRCODE_INVALID_NAME), - errmsg("zero-length component in parameter \"dynamic_library_path\""))); + errmsg("zero-length component in parameter \"%s\"", path_param))); if (piece == NULL) len = strlen(p); @@ -542,7 +561,7 @@ find_in_dynamic_libpath(const char *basename) piece = palloc(len + 1); strlcpy(piece, p, len + 1); - mangled = substitute_libpath_macro(piece); + mangled = substitute_path_macro(piece, macro, macro_val); pfree(piece); canonicalize_path(mangled); @@ -551,13 +570,13 @@ find_in_dynamic_libpath(const char *basename) if (!is_absolute_path(mangled)) ereport(ERROR, (errcode(ERRCODE_INVALID_NAME), - errmsg("component in parameter \"dynamic_library_path\" is not an absolute path"))); + errmsg("component in parameter \"%s\" is not an absolute path", path_param))); full = palloc(strlen(mangled) + 1 + baselen + 1); sprintf(full, "%s/%s", mangled, basename); pfree(mangled); - elog(DEBUG3, "find_in_dynamic_libpath: trying \"%s\"", full); + elog(DEBUG3, "%s: trying \"%s\"", __func__, full); if (pg_file_exists(full)) return full; diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c index ad25cbb39c5..c357a5304ae 100644 --- a/src/backend/utils/misc/guc_tables.c +++ b/src/backend/utils/misc/guc_tables.c @@ -39,6 +39,7 @@ #include "catalog/namespace.h" #include "catalog/storage.h" #include "commands/async.h" +#include "commands/extension.h" #include "commands/event_trigger.h" #include "commands/tablespace.h" #include "commands/trigger.h" @@ -4314,6 +4315,18 @@ struct config_string ConfigureNamesString[] = NULL, NULL, NULL }, + { + {"extension_control_path", PGC_SUSET, CLIENT_CONN_OTHER, + gettext_noop("Sets the path for extension control files."), + gettext_noop("The remaining extension script and secondary control files are then loaded " + "from the same directory where the primary control file was found."), + GUC_SUPERUSER_ONLY + }, + &Extension_control_path, + "$system", + NULL, NULL, NULL + }, + { {"krb_server_keyfile", PGC_SIGHUP, CONN_AUTH_AUTH, gettext_noop("Sets the location of the Kerberos server key file."), diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample index 2d1de9c37bd..d22ef6ef47b 100644 --- a/src/backend/utils/misc/postgresql.conf.sample +++ b/src/backend/utils/misc/postgresql.conf.sample @@ -791,6 +791,7 @@ autovacuum_worker_slots = 16 # autovacuum worker slots to allocate # - Other Defaults - #dynamic_library_path = '$libdir' +#extension_control_path = '$system' #gin_fuzzy_search_limit = 0 diff --git a/src/include/commands/extension.h b/src/include/commands/extension.h index 0b636405120..24419bfb5c9 100644 --- a/src/include/commands/extension.h +++ b/src/include/commands/extension.h @@ -17,6 +17,8 @@ #include "catalog/objectaddress.h" #include "parser/parse_node.h" +/* GUC */ +extern PGDLLIMPORT char *Extension_control_path; /* * creating_extension is only true while running a CREATE EXTENSION or ALTER diff --git a/src/include/fmgr.h b/src/include/fmgr.h index e609ea875a7..442c50d6b90 100644 --- a/src/include/fmgr.h +++ b/src/include/fmgr.h @@ -740,6 +740,8 @@ extern bool CheckFunctionValidatorAccess(Oid validatorOid, Oid functionOid); */ extern PGDLLIMPORT char *Dynamic_library_path; +extern char *find_in_path(const char *basename, const char *path, const char *path_param, + const char *macro, const char *macro_val); extern void *load_external_function(const char *filename, const char *funcname, bool signalNotFound, void **filehandle); extern void *lookup_external_function(void *filehandle, const char *funcname); @@ -749,6 +751,9 @@ extern Size EstimateLibraryStateSpace(void); extern void SerializeLibraryState(Size maxsize, char *start_address); extern void RestoreLibraryState(char *start_address); +extern char * +substitute_path_macro(const char *str, const char *macro, const char *value); + /* * Support for aggregate functions * diff --git a/src/test/modules/test_extensions/Makefile b/src/test/modules/test_extensions/Makefile index 1dbec14cba3..a3591bf3d2f 100644 --- a/src/test/modules/test_extensions/Makefile +++ b/src/test/modules/test_extensions/Makefile @@ -28,6 +28,7 @@ DATA = test_ext1--1.0.sql test_ext2--1.0.sql test_ext3--1.0.sql \ test_ext_req_schema3--1.0.sql REGRESS = test_extensions test_extdepend +TAP_TESTS = 1 # force C locale for output stability NO_LOCALE = 1 diff --git a/src/test/modules/test_extensions/meson.build b/src/test/modules/test_extensions/meson.build index dd7ec0ce56b..3c7e378bf35 100644 --- a/src/test/modules/test_extensions/meson.build +++ b/src/test/modules/test_extensions/meson.build @@ -57,4 +57,9 @@ tests += { ], 'regress_args': ['--no-locale'], }, + 'tap': { + 'tests': [ + 't/001_extension_control_path.pl', + ], + }, } diff --git a/src/test/modules/test_extensions/t/001_extension_control_path.pl b/src/test/modules/test_extensions/t/001_extension_control_path.pl new file mode 100644 index 00000000000..1cf01fca57d --- /dev/null +++ b/src/test/modules/test_extensions/t/001_extension_control_path.pl @@ -0,0 +1,86 @@ +# Copyright (c) 2024-2025, PostgreSQL Global Development Group + +use strict; +use warnings FATAL => 'all'; +use PostgreSQL::Test::Utils; +use PostgreSQL::Test::Cluster; +use Test::More; + +my $node = PostgreSQL::Test::Cluster->new('node'); + +$node->init; + +# Create a temporary directory for the extension control file +my $ext_dir = PostgreSQL::Test::Utils::tempdir(); +my $ext_name = "test_custom_ext_paths"; +my $control_file = "$ext_dir/$ext_name.control"; +my $sql_file = "$ext_dir/$ext_name--1.0.sql"; + +# Create .control .sql file +open my $cf, '>', $control_file or die "Could not create control file: $!"; +print $cf "comment = 'Test extension_control_path'\n"; +print $cf "default_version = '1.0'\n"; +print $cf "relocatable = true\n"; +close $cf; + +# Create --1.0.sql file +open my $sqlf, '>', $sql_file or die "Could not create sql file: $!"; +print $sqlf "/* $sql_file */\n"; +print $sqlf "-- complain if script is sourced in psql, rather than via CREATE EXTENSION\n"; +print $sqlf qq'\\echo Use "CREATE EXTENSION $ext_name" to load this file. \\quit\n'; +close $sqlf; + +# Use the correct separator and escape \ when running on Windows. +my $sep = $windows_os ? ";" : ":"; +$node->append_conf( + 'postgresql.conf', qq{ +extension_control_path = '\$system$sep@{[ $windows_os ? ($ext_dir =~ s/\\/\\\\/gr) : $ext_dir ]}' +}); + +# Start node +$node->start; + +my $ecp = $node->safe_psql('postgres', 'show extension_control_path;'); + +is($ecp, "\$system$sep$ext_dir", "Custom extension control directory path configured"); + +$node->safe_psql( + 'postgres', + "CREATE EXTENSION $ext_name"); + +my $ret = $node->safe_psql( + 'postgres', + "select * from pg_available_extensions where name = '$ext_name'"); +is( + $ret, + "test_custom_ext_paths|1.0|1.0|Test extension_control_path", + "Extension is installed correctly on pg_available_extensions"); + +my $ret2 = $node->safe_psql( + 'postgres', + "select * from pg_available_extension_versions where name = '$ext_name'"); +is( + $ret2, + "test_custom_ext_paths|1.0|t|t|f|t|||Test extension_control_path", + "Extension is installed correctly on pg_available_extension_versions"); + +# Ensure that extensions installed on $system is still visible when using with +# custom extension control path. +my $ret3 = $node->safe_psql( + 'postgres', + "select count(*) > 0 as ok from pg_available_extensions where name = 'amcheck'"); +is( + $ret3, + "t", + "\$system extension is installed correctly on pg_available_extensions"); + + +my $ret4 = $node->safe_psql( + 'postgres', + "set extension_control_path = ''; select count(*) > 0 as ok from pg_available_extensions where name = 'amcheck'"); +is( + $ret4, + "t", + "\$system extension is installed correctly on pg_available_extensions with empty extension_control_path"); + +done_testing(); -- 2.39.5 (Apple Git-154) ^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: RFC: Additional Directory for Extensions @ 2025-03-19 06:42 Peter Eisentraut <[email protected]> parent: Matheus Alcantara <[email protected]> 1 sibling, 3 replies; 31+ messages in thread From: Peter Eisentraut @ 2025-03-19 06:42 UTC (permalink / raw) To: Matheus Alcantara <[email protected]>; +Cc: Andrew Dunstan <[email protected]>; Gabriele Bartolini <[email protected]>; Craig Ringer <[email protected]>; David E. Wheeler <[email protected]>; Robert Haas <[email protected]>; Alvaro Herrera <[email protected]>; pgsql-hackers On 12.03.25 14:17, Matheus Alcantara wrote: >> There should be a simpler way into this. Maybe >> pg_available_extensions() should fill out the ExtensionControlFile >> structure itself, set ->control_dir with where it found it, then call >> directly to parse_extension_control_file(), and that should skip the >> finding if the directory is already set. Or something similar. >> > Good catch. I fixed this by creating a new function to construct the > ExtensionControlFile and changed the pg_available_extensions to set the > control_dir. The read_extension_control_file was also changed to just > call this new function constructor. I implemented the logic to check if > the control_dir is already set on parse_extension_control_file because > it seems to me that make more sense to not call > find_extension_control_filename instead of putting this logic there > since we already set the control_dir when we find the control file, and > having the logic to set the control_dir or skip the find_in_path seems > more confusing on this function instead of on > parse_extension_control_file. Please let me know what you think. Committed that, thanks. A small tweak I made was to replace palloc+snprintf by psprintf. Maybe you were not aware that that function exists. I also simplified the error handling in parse_extension_control_file() a bit. If we pass in a control directory (which is the new code we're adding), then we can assume that we already found the file earlier, and then if we now don't find it, then we should just report the file system error instead of the "you should install this extension first" error. It's kind of a "can't happen" error anyway, so the different is small. ^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: RFC: Additional Directory for Extensions @ 2025-03-19 07:03 Gabriele Bartolini <[email protected]> parent: Peter Eisentraut <[email protected]> 2 siblings, 0 replies; 31+ messages in thread From: Gabriele Bartolini @ 2025-03-19 07:03 UTC (permalink / raw) To: Peter Eisentraut <[email protected]>; +Cc: Matheus Alcantara <[email protected]>; Andrew Dunstan <[email protected]>; Craig Ringer <[email protected]>; David E. Wheeler <[email protected]>; Robert Haas <[email protected]>; Alvaro Herrera <[email protected]>; pgsql-hackers Thanks everyone for making this happen. Ciao, Gabriele On Wed, 19 Mar 2025 at 07:42, Peter Eisentraut <[email protected]> wrote: > On 12.03.25 14:17, Matheus Alcantara wrote: > >> There should be a simpler way into this. Maybe > >> pg_available_extensions() should fill out the ExtensionControlFile > >> structure itself, set ->control_dir with where it found it, then call > >> directly to parse_extension_control_file(), and that should skip the > >> finding if the directory is already set. Or something similar. > >> > > Good catch. I fixed this by creating a new function to construct the > > ExtensionControlFile and changed the pg_available_extensions to set the > > control_dir. The read_extension_control_file was also changed to just > > call this new function constructor. I implemented the logic to check if > > the control_dir is already set on parse_extension_control_file because > > it seems to me that make more sense to not call > > find_extension_control_filename instead of putting this logic there > > since we already set the control_dir when we find the control file, and > > having the logic to set the control_dir or skip the find_in_path seems > > more confusing on this function instead of on > > parse_extension_control_file. Please let me know what you think. > > Committed that, thanks. > > A small tweak I made was to replace palloc+snprintf by psprintf. Maybe > you were not aware that that function exists. > > I also simplified the error handling in parse_extension_control_file() a > bit. If we pass in a control directory (which is the new code we're > adding), then we can assume that we already found the file earlier, and > then if we now don't find it, then we should just report the file system > error instead of the "you should install this extension first" error. > It's kind of a "can't happen" error anyway, so the different is small. > > -- Gabriele Bartolini VP, Chief Architect, Kubernetes enterprisedb.com ^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: RFC: Additional Directory for Extensions @ 2025-03-19 14:58 Christoph Berg <[email protected]> parent: Peter Eisentraut <[email protected]> 2 siblings, 0 replies; 31+ messages in thread From: Christoph Berg @ 2025-03-19 14:58 UTC (permalink / raw) To: Peter Eisentraut <[email protected]>; +Cc: Matheus Alcantara <[email protected]>; Andrew Dunstan <[email protected]>; Gabriele Bartolini <[email protected]>; Craig Ringer <[email protected]>; David E. Wheeler <[email protected]>; Robert Haas <[email protected]>; Alvaro Herrera <[email protected]>; pgsql-hackers Re: Peter Eisentraut > Committed that, thanks. Awesome, thanks! It works perfectly for the Debian "test extension packages at build time" use case, replacing our old extension_destdir patch. PKGARGS="--pgoption extension_control_path=$PWD/debian/$PACKAGE/usr/share/postgresql/$v/extension:\$system --pgoption dynamic_library_path=$PWD/debian/$PACKAGE/usr/lib/postgresql/$v/lib:/usr/lib/postgresql/$v/lib" https://salsa.debian.org/postgresql/postgresql-common/-/commit/3792eea42e4dcef39b5c8d99f63deb8091ef9... Christoph ^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: RFC: Additional Directory for Extensions @ 2025-03-19 17:29 David E. Wheeler <[email protected]> parent: Peter Eisentraut <[email protected]> 2 siblings, 2 replies; 31+ messages in thread From: David E. Wheeler @ 2025-03-19 17:29 UTC (permalink / raw) To: Peter Eisentraut <[email protected]>; +Cc: Matheus Alcantara <[email protected]>; Andrew Dunstan <[email protected]>; Gabriele Bartolini <[email protected]>; Craig Ringer <[email protected]>; Robert Haas <[email protected]>; Alvaro Herrera <[email protected]>; pgsql-hackers On Mar 19, 2025, at 02:42, Peter Eisentraut <[email protected]> wrote: > Committed that, thanks. 🎉 I’ve been meaning to test the patch again, so here goes. First thing I notice is that prefix= uses the magic to insert “postgresql” into the path if it’s not already there: ``` console ❯ make PG_CONFIG=~/dev/c/postgres/pgsql-devel/bin/pg_config prefix=/Users/david/Downloads install /opt/homebrew/bin/gmkdir -p '/Users/david/Downloads/share/postgresql/extension' /opt/homebrew/bin/gmkdir -p '/Users/david/Downloads/share/postgresql/extension' /opt/homebrew/bin/gmkdir -p '/Users/david/Downloads/share/doc//postgresql/extension' /opt/homebrew/bin/ginstall -c -m 644 .//pair.control '/Users/david/Downloads/share/postgresql/extension/' /opt/homebrew/bin/ginstall -c -m 644 .//sql/pair--0.1.2.sql .//sql/pair--unpackaged--0.1.2.sql '/Users/david/Downloads/share/postgresql/extension/' /opt/homebrew/bin/ginstall -c -m 644 .//doc/pair.md '/Users/david/Downloads/share/doc//postgresql/extension/‘ ``` I think this should at least be documented, but generally feels unexpected to me. I’ve attached a patch that fleshes out the docs, along with an example of setting `extension_control_path` and `dynamic_library_path` to use the locations. It might not have the information right about the need for “postgresql” or “pgsql” in the path. Back in 2003[1] it was just “postgres”, but I couldn’t find the logic for it just now. Everything else works very nicely except for extensions that use the Makefile `MODULEDIR` variable to install all of the share files except the control file into a particular directory, and the `directory` in the control file so that the files can be found. Here’s semver[2], which has both: ```console ❯ make PG_CONFIG=~/dev/c/postgres/pgsql-devel/bin/pg_config prefix=/Users/david/Downloads/postgresql install /opt/homebrew/bin/gmkdir -p '/Users/david/Downloads/postgresql/share/extension' /opt/homebrew/bin/gmkdir -p '/Users/david/Downloads/postgresql/share/semver' /opt/homebrew/bin/gmkdir -p '/Users/david/Downloads/postgresql/lib' /opt/homebrew/bin/gmkdir -p '/Users/david/Downloads/postgresql/share/doc//semver' /opt/homebrew/bin/ginstall -c -m 644 .//semver.control '/Users/david/Downloads/postgresql/share/extension/' /opt/homebrew/bin/ginstall -c -m 644 .//sql/semver--0.10.0--0.11.0.sql .//sql/semver--0.11.0--0.12.0.sql .//sql/semver--0.12.0--0.13.0.sql .//sql/semver--0.13.0--0.15.0.sql .//sql/semver--0.15.0--0.16.0.sql .//sql/semver--0.16.0--0.17.0.sql .//sql/semver--0.17.0--0.20.0.sql .//sql/semver--0.2.1--0.2.4.sql .//sql/semver--0.2.4--0.3.0.sql .//sql/semver--0.20.0--0.21.0.sql .//sql/semver--0.21.0--0.22.0.sql .//sql/semver--0.22.0--0.30.0.sql .//sql/semver--0.3.0--0.4.0.sql .//sql/semver--0.30.0--0.31.0.sql .//sql/semver--0.31.0--0.31.1.sql .//sql/semver--0.31.1--0.31.2.sql .//sql/semver--0.31.2--0.32.0.sql .//sql/semver--0.32.0--0.32.1.sql .//sql/semver--0.32.1--0.40.0.sql .//sql/semver--0.32.1.sql .//sql/semver--0.40.0.sql .//sql/semver--0.5.0--0.10.0.sql .//sql/semver--unpackaged--0.2.1.sql .//sql/semver.sql '/Users/david/Downloads/postgresql/share/semver/' /opt/homebrew/bin/ginstall -c -m 755 src/semver.dylib '/Users/david/Downloads/postgresql/lib/' /opt/homebrew/bin/gmkdir -p '/Users/david/Downloads/postgresql/lib/bitcode/src/semver' /opt/homebrew/bin/gmkdir -p '/Users/david/Downloads/postgresql/lib/bitcode'/src/semver/src/ /opt/homebrew/bin/ginstall -c -m 644 src/semver.bc '/Users/david/Downloads/postgresql/lib/bitcode'/src/semver/src/ cd '/Users/david/Downloads/postgresql/lib/bitcode' && /opt/homebrew/Cellar/llvm/19.1.7_1/bin/llvm-lto -thinlto -thinlto-action=thinlink -o src/semver.index.bc src/semver/src/semver.bc /opt/homebrew/bin/ginstall -c -m 644 .//doc/semver.mmd '/Users/david/Downloads/postgresql/share/doc//semver/‘ ``` Following `MODULEDIR=semver`, it puts the SQL files into `share/semver/` instead of `share/extension/`, as expected, but then, even though the control file has `directory=semver`, it can’t load them: ```pgsql david=# create extension semver; ERROR: could not open directory "/Users/david/dev/c/postgres/pgsql-devel/share/semver": No such file or directory ``` Looks like it’s only looking in the `semver` subdirectory under $libdir and not the whole path. But given that the `directory` variable in the control file can be a full path, I don’t see that there’s much of a way to generalize a solution. I guess there are three options: 1. If directory is a full path, try to load the files there. It probably already works that way, though I haven’t tired it. 2. If the directory is not a full path, check for it under each directory in `extension_control_path`? But no, that points to `share/extension`, not `share`, so it can’t really searched unless it also lops off `extension` from the end of each path. 3. Drop support for MODULEDIR and directory. I think I’d opt for #3, personally, just to simplify things. Anyway, I then built envvar, a C extension with no `directory` configuration, and it worked perfectly. I will say, though, that I will kind of miss being able to run `make install` without first running `make`, as the `prefix` variable does not work with `make`. Best, David [1]: https://postgr.es/m/[email protected] [2]: https://github.com/theory/pg-semver/ [3]: https://github.com/theory/pg-envvar Attachments: [application/octet-stream] v1-0001-Flesh-out-docs-for-the-prefix-make-variable.patch (2.6K, ../../[email protected]/2-v1-0001-Flesh-out-docs-for-the-prefix-make-variable.patch) download | inline diff: From e0ffe63f621463662d13bf21e9431a78a3391349 Mon Sep 17 00:00:00 2001 From: "David E. Wheeler" <[email protected]> Date: Wed, 19 Mar 2025 13:18:33 -0400 Subject: [PATCH v1] Flesh out docs for the `prefix` make variable The variable is a bit magical in how it requires "postgresql" or "pgsql" to be part of the path, and files end up in its `share` and `lib` subdirectories. So mention all that and show an example of setting `extension_control_path` and `dynamic_library_path` to use those locations. --- doc/src/sgml/extend.sgml | 33 +++++++++++++++++++++++++++++---- 1 file changed, 29 insertions(+), 4 deletions(-) diff --git a/doc/src/sgml/extend.sgml b/doc/src/sgml/extend.sgml index 64f8e133cae..4e75a01fae4 100644 --- a/doc/src/sgml/extend.sgml +++ b/doc/src/sgml/extend.sgml @@ -1809,10 +1809,35 @@ include $(PGXS) setting <varname>PG_CONFIG</varname> to point to its <command>pg_config</command> program, either within the makefile or on the <literal>make</literal> command line. - You can also select a separate installation directory for your extension - by setting the <literal>make</literal> variable <varname>prefix</varname> - on the <literal>make</literal> command line. (But this will then require - additional setup to get the server to find the extension there.) + </para> + + <para> + You can also select a separate directory prefix in which to install your + extension's files by setting the <literal>make</literal> variable + <varname>prefix</varname> when executing <literal>make install</literal> + like so: +<programlisting> +make install prefix=/etc/postgresql +</programlisting> + This will install the control SQL files into + <literal>/etc/postgresql/share</literal> and shared modules into + <literal>/etc/postgresql/lib</literal>. If the prefix does not + include the strings <literal>postgresql</literal> or + <literal>pgsql</literal>, such as: +<programlisting> +make install prefix=/etc/extras +</programlisting> + Then the <literal>postgresql</literal> directory will be appended io the + prefix, installing the control SQL files into + <literal>/etc/extras/postgresql/share</literal> and shared modules into + <literal>/etc/extras/postgresql/lib</literal>. Either way, you'll need to + set <xref linkend="guc-extension-control-path"/> and <xref + linkend="guc-dynamic-library-path"/> to allow + <productname>PostgreSQL</productname> to find the files: +</programlisting> +extension_control_path = '/etc/extras/postgresql/share/extension:$system' +dynamic_library_path = '/etc/extras/postgresql/lib:$libdir' + </programlisting> </para> <para> -- 2.48.1 [application/pgp-signature] signature.asc (833B, ../../[email protected]/4-signature.asc) download ^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: RFC: Additional Directory for Extensions @ 2025-03-19 18:55 Tom Lane <[email protected]> parent: Matheus Alcantara <[email protected]> 1 sibling, 2 replies; 31+ messages in thread From: Tom Lane @ 2025-03-19 18:55 UTC (permalink / raw) To: Peter Eisentraut <[email protected]>; +Cc: Matheus Alcantara <[email protected]>; Andrew Dunstan <[email protected]>; Gabriele Bartolini <[email protected]>; Craig Ringer <[email protected]>; David E. Wheeler <[email protected]>; Robert Haas <[email protected]>; Alvaro Herrera <[email protected]>; pgsql-hackers Peter Eisentraut <[email protected]> writes: > Committed that, thanks. Buildfarm member snakefly doesn't like this too much. Since no other animals have failed, I guess it must be about local conditions on that machine, but the report is pretty opaque: # +++ tap check in src/test/modules/test_extensions +++ # Failed test '$system extension is installed correctly on pg_available_extensions' # at t/001_extension_control_path.pl line 69. # got: 'f' # expected: 't' # Failed test '$system extension is installed correctly on pg_available_extensions with empty extension_control_path' # at t/001_extension_control_path.pl line 76. # got: 'f' # expected: 't' # Looks like you failed 2 tests of 5. [06:43:53] t/001_extension_control_path.pl .. Dubious, test returned 2 (wstat 512, 0x200) Failed 2/5 subtests Looking at the test, it presupposes that "amcheck" must be an available extension. I do not see anything that guarantees that that's so, though. It'd fail if contrib hasn't been installed. Is there a reason to use "amcheck" rather than something more certainly available, like "plpgsql"? regards, tom lane ^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: RFC: Additional Directory for Extensions @ 2025-03-19 19:25 Matheus Alcantara <[email protected]> parent: Tom Lane <[email protected]> 1 sibling, 3 replies; 31+ messages in thread From: Matheus Alcantara @ 2025-03-19 19:25 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; Andrew Dunstan <[email protected]>; Gabriele Bartolini <[email protected]>; Craig Ringer <[email protected]>; David E. Wheeler <[email protected]>; Robert Haas <[email protected]>; Alvaro Herrera <[email protected]>; pgsql-hackers On Wed, Mar 19, 2025 at 3:56 PM Tom Lane <[email protected]> wrote: > > Peter Eisentraut <[email protected]> writes: > > Committed that, thanks. > > Buildfarm member snakefly doesn't like this too much. Since no other > animals have failed, I guess it must be about local conditions on > that machine, but the report is pretty opaque: > > # +++ tap check in src/test/modules/test_extensions +++ > > # Failed test '$system extension is installed correctly on pg_available_extensions' > # at t/001_extension_control_path.pl line 69. > # got: 'f' > # expected: 't' > > # Failed test '$system extension is installed correctly on pg_available_extensions with empty extension_control_path' > # at t/001_extension_control_path.pl line 76. > # got: 'f' > # expected: 't' > # Looks like you failed 2 tests of 5. > [06:43:53] t/001_extension_control_path.pl .. > Dubious, test returned 2 (wstat 512, 0x200) > Failed 2/5 subtests > > Looking at the test, it presupposes that "amcheck" must be an > available extension. I do not see anything that guarantees > that that's so, though. It'd fail if contrib hasn't been > installed. Is there a reason to use "amcheck" rather than > something more certainly available, like "plpgsql"? There is no specific reason to use "amcheck" instead of "plpgsql". Attached a patch with this change, sorry about that. (Not sure if we should also improve the message to make the test failure less opaque?) -- Matheus Alcantara Attachments: [application/octet-stream] v1-0001-Fix-extension-control-path-tests.patch (1.6K, ../../CAFY6G8dwT=E_SDSobVqpz+2y0otAuKFT4nOwHxQORHXjWfcJ1A@mail.gmail.com/2-v1-0001-Fix-extension-control-path-tests.patch) download | inline diff: From 4fa81f1c04df649b183e1e55053662a35109d0b6 Mon Sep 17 00:00:00 2001 From: Matheus Alcantara <[email protected]> Date: Wed, 19 Mar 2025 16:15:43 -0300 Subject: [PATCH v1] Fix extension control path tests Change expected extension to be installed from amcheck to plpgsql since not all build farm animals has the contrib module installed. --- .../modules/test_extensions/t/001_extension_control_path.pl | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/modules/test_extensions/t/001_extension_control_path.pl b/src/test/modules/test_extensions/t/001_extension_control_path.pl index 7160009739a..c186c1470f7 100644 --- a/src/test/modules/test_extensions/t/001_extension_control_path.pl +++ b/src/test/modules/test_extensions/t/001_extension_control_path.pl @@ -64,14 +64,14 @@ is( $ret2, # Ensure that extensions installed on $system is still visible when using with # custom extension control path. my $ret3 = $node->safe_psql('postgres', - "select count(*) > 0 as ok from pg_available_extensions where name = 'amcheck'" + "select count(*) > 0 as ok from pg_available_extensions where name = 'plpgsql'" ); is($ret3, "t", "\$system extension is installed correctly on pg_available_extensions"); my $ret4 = $node->safe_psql('postgres', - "set extension_control_path = ''; select count(*) > 0 as ok from pg_available_extensions where name = 'amcheck'" + "set extension_control_path = ''; select count(*) > 0 as ok from pg_available_extensions where name = 'plpgsql'" ); is($ret4, "t", "\$system extension is installed correctly on pg_available_extensions with empty extension_control_path" -- 2.39.5 (Apple Git-154) ^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: RFC: Additional Directory for Extensions @ 2025-03-19 19:42 Tom Lane <[email protected]> parent: Matheus Alcantara <[email protected]> 2 siblings, 0 replies; 31+ messages in thread From: Tom Lane @ 2025-03-19 19:42 UTC (permalink / raw) To: Matheus Alcantara <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; Andrew Dunstan <[email protected]>; Gabriele Bartolini <[email protected]>; Craig Ringer <[email protected]>; David E. Wheeler <[email protected]>; Robert Haas <[email protected]>; Alvaro Herrera <[email protected]>; pgsql-hackers Matheus Alcantara <[email protected]> writes: > (Not sure if we should also improve the message to make the test failure less > opaque?) Yeah, I was wondering how to do that. The earlier tests in that script show the whole row from pg_available_extensions, not just a bool ... but that doesn't help if the problem is we don't find a row. regards, tom lane ^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: RFC: Additional Directory for Extensions @ 2025-03-20 10:01 Peter Eisentraut <[email protected]> parent: Matheus Alcantara <[email protected]> 2 siblings, 0 replies; 31+ messages in thread From: Peter Eisentraut @ 2025-03-20 10:01 UTC (permalink / raw) To: Matheus Alcantara <[email protected]>; Tom Lane <[email protected]>; +Cc: Andrew Dunstan <[email protected]>; Gabriele Bartolini <[email protected]>; Craig Ringer <[email protected]>; David E. Wheeler <[email protected]>; Robert Haas <[email protected]>; Alvaro Herrera <[email protected]>; pgsql-hackers On 19.03.25 20:25, Matheus Alcantara wrote: > On Wed, Mar 19, 2025 at 3:56 PM Tom Lane <[email protected]> wrote: >> >> Peter Eisentraut <[email protected]> writes: >>> Committed that, thanks. >> >> Buildfarm member snakefly doesn't like this too much. Since no other >> animals have failed, I guess it must be about local conditions on >> that machine, but the report is pretty opaque: >> >> # +++ tap check in src/test/modules/test_extensions +++ >> >> # Failed test '$system extension is installed correctly on pg_available_extensions' >> # at t/001_extension_control_path.pl line 69. >> # got: 'f' >> # expected: 't' >> >> # Failed test '$system extension is installed correctly on pg_available_extensions with empty extension_control_path' >> # at t/001_extension_control_path.pl line 76. >> # got: 'f' >> # expected: 't' >> # Looks like you failed 2 tests of 5. >> [06:43:53] t/001_extension_control_path.pl .. >> Dubious, test returned 2 (wstat 512, 0x200) >> Failed 2/5 subtests >> >> Looking at the test, it presupposes that "amcheck" must be an >> available extension. I do not see anything that guarantees >> that that's so, though. It'd fail if contrib hasn't been >> installed. Is there a reason to use "amcheck" rather than >> something more certainly available, like "plpgsql"? > > There is no specific reason to use "amcheck" instead of "plpgsql". Attached a > patch with this change, sorry about that. Committed. I was able to reproduce the problem from scratch using: ./configure ... make # no contrib make -C src/test/modules/test_extensions check So it depended on in which order you build the various components. ^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: RFC: Additional Directory for Extensions @ 2025-03-20 14:35 Tom Lane <[email protected]> parent: Matheus Alcantara <[email protected]> 2 siblings, 0 replies; 31+ messages in thread From: Tom Lane @ 2025-03-20 14:35 UTC (permalink / raw) To: Peter Eisentraut <[email protected]>; +Cc: Matheus Alcantara <[email protected]>; Andrew Dunstan <[email protected]>; Gabriele Bartolini <[email protected]>; Craig Ringer <[email protected]>; David E. Wheeler <[email protected]>; Robert Haas <[email protected]>; Alvaro Herrera <[email protected]>; pgsql-hackers Peter Eisentraut <[email protected]> writes: > On 19.03.25 20:25, Matheus Alcantara wrote: >> On Wed, Mar 19, 2025 at 3:56 PM Tom Lane <[email protected]> wrote: >>> Buildfarm member snakefly doesn't like this too much. > I was able to reproduce the problem from scratch using: > ./configure ... > make # no contrib > make -C src/test/modules/test_extensions check > So it depended on in which order you build the various components. That makes sense, but I wonder how snakefly hit it while other BF animals did not. It's running a reasonably up-to-date BF client version and there's nothing odd-looking about its configuration. Anyway, I see snakefly is green now so that tweak did fix it. regards, tom lane ^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: RFC: Additional Directory for Extensions @ 2025-03-20 14:53 Andrew Dunstan <[email protected]> parent: Tom Lane <[email protected]> 1 sibling, 1 reply; 31+ messages in thread From: Andrew Dunstan @ 2025-03-20 14:53 UTC (permalink / raw) To: Tom Lane <[email protected]>; Peter Eisentraut <[email protected]>; +Cc: Matheus Alcantara <[email protected]>; Gabriele Bartolini <[email protected]>; Craig Ringer <[email protected]>; David E. Wheeler <[email protected]>; Robert Haas <[email protected]>; Alvaro Herrera <[email protected]>; pgsql-hackers On 2025-03-19 We 2:55 PM, Tom Lane wrote: > Peter Eisentraut<[email protected]> writes: >> Committed that, thanks. > Buildfarm member snakefly doesn't like this too much. Since no other > animals have failed, I guess it must be about local conditions on > that machine, but the report is pretty opaque: > > # +++ tap check in src/test/modules/test_extensions +++ > > # Failed test '$system extension is installed correctly on pg_available_extensions' > # at t/001_extension_control_path.pl line 69. > # got: 'f' > # expected: 't' > > # Failed test '$system extension is installed correctly on pg_available_extensions with empty extension_control_path' > # at t/001_extension_control_path.pl line 76. > # got: 'f' > # expected: 't' > # Looks like you failed 2 tests of 5. > [06:43:53] t/001_extension_control_path.pl .. > Dubious, test returned 2 (wstat 512, 0x200) > Failed 2/5 subtests > > Looking at the test, it presupposes that "amcheck" must be an > available extension. I do not see anything that guarantees > that that's so, though. It'd fail if contrib hasn't been > installed. Is there a reason to use "amcheck" rather than > something more certainly available, like "plpgsql"? I think something else must be going on. The failure in question came after the step "install-contrib" succeeded, and the log file for that shows: make[1]: Entering directory `/opt/postgres/build-farm-18/HEAD/pgsql.build/contrib/amcheck' /usr/bin/mkdir -p '/opt/postgres/build-farm-18/HEAD/pgsql.build/tmp_install/opt/postgres/build-farm-18/HEAD/inst/lib' /usr/bin/mkdir -p '/opt/postgres/build-farm-18/HEAD/pgsql.build/tmp_install/opt/postgres/build-farm-18/HEAD/inst/share/extension' /usr/bin/mkdir -p '/opt/postgres/build-farm-18/HEAD/pgsql.build/tmp_install/opt/postgres/build-farm-18/HEAD/inst/share/extension' /usr/bin/install -c -m 755 amcheck.so '/opt/postgres/build-farm-18/HEAD/pgsql.build/tmp_install/opt/postgres/build-farm-18/HEAD/inst/lib/amcheck.so' /usr/bin/install -c -m 644 ./amcheck.control '/opt/postgres/build-farm-18/HEAD/pgsql.build/tmp_install/opt/postgres/build-farm-18/HEAD/inst/share/extension/' /usr/bin/install -c -m 644 ./amcheck--1.3--1.4.sql ./amcheck--1.2--1.3.sql ./amcheck--1.1--1.2.sql ./amcheck--1.0--1.1.sql ./amcheck--1.0.sql '/opt/postgres/build-farm-18/HEAD/pgsql.build/tmp_install/opt/postgres/build-farm-18/HEAD/inst/share/extension/' make[1]: Leaving directory `/opt/postgres/build-farm-18/HEAD/pgsql.build/contrib/amcheck' (wondering if this another of these cases where the "path includes postgres" thing bites us, and we're looking in the wrong place) cheers andrew -- Andrew Dunstan EDB:https://www.enterprisedb.com ^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: RFC: Additional Directory for Extensions @ 2025-03-20 22:38 Andrew Dunstan <[email protected]> parent: Andrew Dunstan <[email protected]> 0 siblings, 2 replies; 31+ messages in thread From: Andrew Dunstan @ 2025-03-20 22:38 UTC (permalink / raw) To: Tom Lane <[email protected]>; Peter Eisentraut <[email protected]>; +Cc: Matheus Alcantara <[email protected]>; Gabriele Bartolini <[email protected]>; Craig Ringer <[email protected]>; David E. Wheeler <[email protected]>; Robert Haas <[email protected]>; Alvaro Herrera <[email protected]>; pgsql-hackers On 2025-03-20 Th 10:53 AM, Andrew Dunstan wrote: > > > On 2025-03-19 We 2:55 PM, Tom Lane wrote: >> Peter Eisentraut<[email protected]> writes: >>> Committed that, thanks. >> Buildfarm member snakefly doesn't like this too much. Since no other >> animals have failed, I guess it must be about local conditions on >> that machine, but the report is pretty opaque: >> >> # +++ tap check in src/test/modules/test_extensions +++ >> >> # Failed test '$system extension is installed correctly on pg_available_extensions' >> # at t/001_extension_control_path.pl line 69. >> # got: 'f' >> # expected: 't' >> >> # Failed test '$system extension is installed correctly on pg_available_extensions with empty extension_control_path' >> # at t/001_extension_control_path.pl line 76. >> # got: 'f' >> # expected: 't' >> # Looks like you failed 2 tests of 5. >> [06:43:53] t/001_extension_control_path.pl .. >> Dubious, test returned 2 (wstat 512, 0x200) >> Failed 2/5 subtests >> >> Looking at the test, it presupposes that "amcheck" must be an >> available extension. I do not see anything that guarantees >> that that's so, though. It'd fail if contrib hasn't been >> installed. Is there a reason to use "amcheck" rather than >> something more certainly available, like "plpgsql"? > > > > I think something else must be going on. The failure in question came > after the step "install-contrib" succeeded, and the log file for that > shows: > > > make[1]: Entering directory `/opt/postgres/build-farm-18/HEAD/pgsql.build/contrib/amcheck' > /usr/bin/mkdir -p '/opt/postgres/build-farm-18/HEAD/pgsql.build/tmp_install/opt/postgres/build-farm-18/HEAD/inst/lib' > /usr/bin/mkdir -p '/opt/postgres/build-farm-18/HEAD/pgsql.build/tmp_install/opt/postgres/build-farm-18/HEAD/inst/share/extension' > /usr/bin/mkdir -p '/opt/postgres/build-farm-18/HEAD/pgsql.build/tmp_install/opt/postgres/build-farm-18/HEAD/inst/share/extension' > /usr/bin/install -c -m 755 amcheck.so '/opt/postgres/build-farm-18/HEAD/pgsql.build/tmp_install/opt/postgres/build-farm-18/HEAD/inst/lib/amcheck.so' > /usr/bin/install -c -m 644 ./amcheck.control '/opt/postgres/build-farm-18/HEAD/pgsql.build/tmp_install/opt/postgres/build-farm-18/HEAD/inst/share/extension/' > /usr/bin/install -c -m 644 ./amcheck--1.3--1.4.sql ./amcheck--1.2--1.3.sql ./amcheck--1.1--1.2.sql ./amcheck--1.0--1.1.sql ./amcheck--1.0.sql '/opt/postgres/build-farm-18/HEAD/pgsql.build/tmp_install/opt/postgres/build-farm-18/HEAD/inst/share/extension/' > make[1]: Leaving directory `/opt/postgres/build-farm-18/HEAD/pgsql.build/contrib/amcheck' > > > (wondering if this another of these cases where the "path includes postgres" thing bites us, and we're looking in the wrong place) > > > Nope, testing shows it's not that, so I am rather confused about what was going on. cheers andrew -- Andrew Dunstan EDB:https://www.enterprisedb.com ^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: RFC: Additional Directory for Extensions @ 2025-03-21 15:52 Matheus Alcantara <[email protected]> parent: Andrew Dunstan <[email protected]> 1 sibling, 1 reply; 31+ messages in thread From: Matheus Alcantara @ 2025-03-21 15:52 UTC (permalink / raw) To: Andrew Dunstan <[email protected]>; +Cc: Tom Lane <[email protected]>; Peter Eisentraut <[email protected]>; Gabriele Bartolini <[email protected]>; Craig Ringer <[email protected]>; David E. Wheeler <[email protected]>; Robert Haas <[email protected]>; Alvaro Herrera <[email protected]>; pgsql-hackers On Thu, Mar 20, 2025 at 7:38 PM Andrew Dunstan <[email protected]> wrote: >>> >>> Buildfarm member snakefly doesn't like this too much. Since no other >>> animals have failed, I guess it must be about local conditions on >>> that machine, but the report is pretty opaque: >>> >>> # +++ tap check in src/test/modules/test_extensions +++ >>> >>> # Failed test '$system extension is installed correctly on pg_available_extensions' >>> # at t/001_extension_control_path.pl line 69. >>> # got: 'f' >>> # expected: 't' >>> >>> # Failed test '$system extension is installed correctly on pg_available_extensions with empty extension_control_path' >>> # at t/001_extension_control_path.pl line 76. >>> # got: 'f' >>> # expected: 't' >>> # Looks like you failed 2 tests of 5. >>> [06:43:53] t/001_extension_control_path.pl .. >>> Dubious, test returned 2 (wstat 512, 0x200) >>> Failed 2/5 subtests >>> >>> Looking at the test, it presupposes that "amcheck" must be an >>> available extension. I do not see anything that guarantees >>> that that's so, though. It'd fail if contrib hasn't been >>> installed. Is there a reason to use "amcheck" rather than >>> something more certainly available, like "plpgsql"? >> >> I think something else must be going on. The failure in question came after the step "install-contrib" succeeded, and the log file for that shows: >> >> >> make[1]: Entering directory `/opt/postgres/build-farm-18/HEAD/pgsql.build/contrib/amcheck' >> /usr/bin/mkdir -p '/opt/postgres/build-farm-18/HEAD/pgsql.build/tmp_install/opt/postgres/build-farm-18/HEAD/inst/lib' >> /usr/bin/mkdir -p '/opt/postgres/build-farm-18/HEAD/pgsql.build/tmp_install/opt/postgres/build-farm-18/HEAD/inst/share/extension' >> /usr/bin/mkdir -p '/opt/postgres/build-farm-18/HEAD/pgsql.build/tmp_install/opt/postgres/build-farm-18/HEAD/inst/share/extension' >> /usr/bin/install -c -m 755 amcheck.so '/opt/postgres/build-farm-18/HEAD/pgsql.build/tmp_install/opt/postgres/build-farm-18/HEAD/inst/lib/amcheck.so' >> /usr/bin/install -c -m 644 ./amcheck.control '/opt/postgres/build-farm-18/HEAD/pgsql.build/tmp_install/opt/postgres/build-farm-18/HEAD/inst/share/extension/' >> /usr/bin/install -c -m 644 ./amcheck--1.3--1.4.sql ./amcheck--1.2--1.3.sql ./amcheck--1.1--1.2.sql ./amcheck--1.0--1.1.sql ./amcheck--1.0.sql '/opt/postgres/build-farm-18/HEAD/pgsql.build/tmp_install/opt/postgres/build-farm-18/HEAD/inst/share/extension/' >> make[1]: Leaving directory `/opt/postgres/build-farm-18/HEAD/pgsql.build/contrib/amcheck' >> >> >> (wondering if this another of these cases where the "path includes postgres" thing bites us, and we're looking in the wrong place) > > Nope, testing shows it's not that, so I am rather confused about what was going on. > I'm not sure if I'm checking on the right place [1] but it seems that the Contrib and ContribInstall is executed after Check step which causes this test failure? 'steps_completed' => [ 'SCM-checkout', 'Configure', 'Build', 'Check', 'Contrib', 'TestModules', 'Install', 'ContribInstall', 'TestModulesInstall', 'MiscCheck', ... ] [1] https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=snakefly&dt=2025-03-20%2009%3A46%3A05 -- Matheus Alcantara ^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: RFC: Additional Directory for Extensions @ 2025-03-21 16:38 Andrew Dunstan <[email protected]> parent: Matheus Alcantara <[email protected]> 0 siblings, 0 replies; 31+ messages in thread From: Andrew Dunstan @ 2025-03-21 16:38 UTC (permalink / raw) To: Matheus Alcantara <[email protected]>; +Cc: Tom Lane <[email protected]>; Peter Eisentraut <[email protected]>; Gabriele Bartolini <[email protected]>; Craig Ringer <[email protected]>; David E. Wheeler <[email protected]>; Robert Haas <[email protected]>; Alvaro Herrera <[email protected]>; pgsql-hackers On 2025-03-21 Fr 11:52 AM, Matheus Alcantara wrote: > On Thu, Mar 20, 2025 at 7:38 PM Andrew Dunstan <[email protected]> wrote: >>>> Buildfarm member snakefly doesn't like this too much. Since no other >>>> animals have failed, I guess it must be about local conditions on >>>> that machine, but the report is pretty opaque: >>>> >>>> # +++ tap check in src/test/modules/test_extensions +++ >>>> >>>> # Failed test '$system extension is installed correctly on pg_available_extensions' >>>> # at t/001_extension_control_path.pl line 69. >>>> # got: 'f' >>>> # expected: 't' >>>> >>>> # Failed test '$system extension is installed correctly on pg_available_extensions with empty extension_control_path' >>>> # at t/001_extension_control_path.pl line 76. >>>> # got: 'f' >>>> # expected: 't' >>>> # Looks like you failed 2 tests of 5. >>>> [06:43:53] t/001_extension_control_path.pl .. >>>> Dubious, test returned 2 (wstat 512, 0x200) >>>> Failed 2/5 subtests >>>> >>>> Looking at the test, it presupposes that "amcheck" must be an >>>> available extension. I do not see anything that guarantees >>>> that that's so, though. It'd fail if contrib hasn't been >>>> installed. Is there a reason to use "amcheck" rather than >>>> something more certainly available, like "plpgsql"? >>> I think something else must be going on. The failure in question came after the step "install-contrib" succeeded, and the log file for that shows: >>> >>> >>> make[1]: Entering directory `/opt/postgres/build-farm-18/HEAD/pgsql.build/contrib/amcheck' >>> /usr/bin/mkdir -p '/opt/postgres/build-farm-18/HEAD/pgsql.build/tmp_install/opt/postgres/build-farm-18/HEAD/inst/lib' >>> /usr/bin/mkdir -p '/opt/postgres/build-farm-18/HEAD/pgsql.build/tmp_install/opt/postgres/build-farm-18/HEAD/inst/share/extension' >>> /usr/bin/mkdir -p '/opt/postgres/build-farm-18/HEAD/pgsql.build/tmp_install/opt/postgres/build-farm-18/HEAD/inst/share/extension' >>> /usr/bin/install -c -m 755 amcheck.so '/opt/postgres/build-farm-18/HEAD/pgsql.build/tmp_install/opt/postgres/build-farm-18/HEAD/inst/lib/amcheck.so' >>> /usr/bin/install -c -m 644 ./amcheck.control '/opt/postgres/build-farm-18/HEAD/pgsql.build/tmp_install/opt/postgres/build-farm-18/HEAD/inst/share/extension/' >>> /usr/bin/install -c -m 644 ./amcheck--1.3--1.4.sql ./amcheck--1.2--1.3.sql ./amcheck--1.1--1.2.sql ./amcheck--1.0--1.1.sql ./amcheck--1.0.sql '/opt/postgres/build-farm-18/HEAD/pgsql.build/tmp_install/opt/postgres/build-farm-18/HEAD/inst/share/extension/' >>> make[1]: Leaving directory `/opt/postgres/build-farm-18/HEAD/pgsql.build/contrib/amcheck' >>> >>> >>> (wondering if this another of these cases where the "path includes postgres" thing bites us, and we're looking in the wrong place) >> Nope, testing shows it's not that, so I am rather confused about what was going on. >> > I'm not sure if I'm checking on the right place [1] but it seems that the > Contrib and ContribInstall is executed after Check step which causes this test > failure? > > 'steps_completed' => [ > 'SCM-checkout', > 'Configure', > 'Build', > 'Check', > 'Contrib', > 'TestModules', > 'Install', > 'ContribInstall', > 'TestModulesInstall', > 'MiscCheck', > ... > ] > > [1] https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=snakefly&dt=2025-03-20%2009%3A46%3A05 No. In the buildfarm, the Check step only runs the core regression tests, not any TAP tests. The above shows fairly clearly that the failure occurred after the ContribInstall step, which is what's puzzling me. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com ^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: RFC: Additional Directory for Extensions @ 2025-03-21 16:42 Tom Lane <[email protected]> parent: Andrew Dunstan <[email protected]> 1 sibling, 1 reply; 31+ messages in thread From: Tom Lane @ 2025-03-21 16:42 UTC (permalink / raw) To: Matheus Alcantara <[email protected]>; +Cc: Andrew Dunstan <[email protected]>; Peter Eisentraut <[email protected]>; Gabriele Bartolini <[email protected]>; Craig Ringer <[email protected]>; David E. Wheeler <[email protected]>; Robert Haas <[email protected]>; Alvaro Herrera <[email protected]>; pgsql-hackers Matheus Alcantara <[email protected]> writes: > On Thu, Mar 20, 2025 at 7:38 PM Andrew Dunstan <[email protected]> wrote: >>> (wondering if this another of these cases where the "path includes postgres" thing bites us, and we're looking in the wrong place) >> Nope, testing shows it's not that, so I am rather confused about what was going on. > I'm not sure if I'm checking on the right place [1] but it seems that the > Contrib and ContribInstall is executed after Check step which causes this test > failure? No, this is not failing in Check. I did just notice a clue though: on snakefly, the failing step's log [1] includes make[1]: Leaving directory `/opt/postgres/build-farm-18/HEAD/pgsql.build/src/backend' rm -rf '/opt/postgres/build-farm-18/HEAD/pgsql.build'/tmp_install /usr/bin/mkdir -p '/opt/postgres/build-farm-18/HEAD/pgsql.build'/tmp_install/log make -C '../../../..' DESTDIR='/opt/postgres/build-farm-18/HEAD/pgsql.build'/tmp_install install >'/opt/postgres/build-farm-18/HEAD/pgsql.build'/tmp_install/log/install.log 2>&1 make -j1 checkprep >>'/opt/postgres/build-farm-18/HEAD/pgsql.build'/tmp_install/log/install.log 2>&1 PATH="/opt/postgres/build-farm-18/HEAD/pgsql.build/tmp_install/opt/postgres/build-farm-18/HEAD/inst/bin:/opt/postgres/build-farm-18/HEAD/pgsql.build/src/test/modules/test_extensions:$PATH" LD_LIBRARY_PATH="/opt/postgres/build-farm-18/HEAD/pgsql.build/tmp_install/opt/postgres/build-farm-18/HEAD/inst/lib:$LD_LIBRARY_PATH" INITDB_TEMPLATE='/opt/postgres/build-farm-18/HEAD/pgsql.build'/tmp_install/initdb-template initdb --auth trust --no-sync --no-instructions --lc-messages=C --no-clean '/opt/postgres/build-farm-18/HEAD/pgsql.build'/tmp_install/initdb-template >>'/opt/postgres/build-farm-18/HEAD/pgsql.build'/tmp_install/log/initdb-template.log 2>&1 echo "# +++ regress check in src/test/modules/test_extensions +++" && PATH="/opt/postgres/build-farm-18/HEAD/pgsql.build/tmp_install/opt/postgres/build-farm-18/HEAD/inst/bin:/opt/postgres/build-farm-18/HEAD/pgsql.build/src/test/modules/test_extensions:$PATH" LD_LIBRARY_PATH="/opt/postgres/build-farm-18/HEAD/pgsql.build/tmp_install/opt/postgres/build-farm-18/HEAD/inst/lib:$LD_LIBRARY_PATH" INITDB_TEMPLATE='/opt/postgres/build-farm-18/HEAD/pgsql.build'/tmp_install/initdb-template ../../../../src/test/regress/pg_regress --temp-instance=./tmp_check --inputdir=. --bindir= --temp-config=/opt/postgres/build-farm-18/tmp/buildfarm-C9Iy3s/bfextra.conf --no-locale --port=5678 --dbname=contrib_regression test_extensions test_extdepend # +++ regress check in src/test/modules/test_extensions +++ # initializing database system by running initdb showing that the step made its own tmp_install, and that only the core "install" process was executed, so the lack of amcheck in that install tree is not surprising. But concurrent runs on other animals, eg [2], don't show a tmp_install rebuild happening. So those are using an installation tree that *does* include contrib modules. So what this comes down to is "why is snakefly doing a fresh install here?". I don't know the buildfarm client well enough to identify probable causes. I do note that Makefile.global.in conditionalizes tmp_install rebuild on several variables: temp-install: | submake-generated-headers ifndef NO_TEMP_INSTALL ifneq ($(abs_top_builddir),) ifeq ($(MAKELEVEL),0) rm -rf '$(abs_top_builddir)'/tmp_install $(MKDIR_P) '$(abs_top_builddir)'/tmp_install/log $(MAKE) -C '$(top_builddir)' DESTDIR='$(abs_top_builddir)'/tmp_install install >'$(abs_top_builddir)'/tmp_install/log/install.log 2>&1 $(MAKE) -j1 $(if $(CHECKPREP_TOP),-C $(CHECKPREP_TOP),) checkprep >>'$(abs_top_builddir)'/tmp_install/log/install.log 2>&1 I think we've had trouble before with that MAKELEVEL test... regards, tom lane [1] https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=snakefly&dt=2025-03-20%2009%3A46%3... [2] https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=alligator&dt=2025-03-19%2006%3A10%... ^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: RFC: Additional Directory for Extensions @ 2025-03-21 21:52 Matheus Alcantara <[email protected]> parent: David E. Wheeler <[email protected]> 1 sibling, 1 reply; 31+ messages in thread From: Matheus Alcantara @ 2025-03-21 21:52 UTC (permalink / raw) To: David E. Wheeler <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; Andrew Dunstan <[email protected]>; Gabriele Bartolini <[email protected]>; Craig Ringer <[email protected]>; Robert Haas <[email protected]>; Alvaro Herrera <[email protected]>; pgsql-hackers Hi David, thanks for testing! On Wed, Mar 19, 2025 at 2:29 PM David E. Wheeler <[email protected]> wrote: > > First thing I notice is that prefix= uses the magic to insert > “postgresql” into the path if it’s not already there: > > ``` console > ❯ make PG_CONFIG=~/dev/c/postgres/pgsql-devel/bin/pg_config prefix=/Users/david/Downloads install > /opt/homebrew/bin/gmkdir -p '/Users/david/Downloads/share/postgresql/extension' > /opt/homebrew/bin/gmkdir -p '/Users/david/Downloads/share/postgresql/extension' > /opt/homebrew/bin/gmkdir -p '/Users/david/Downloads/share/doc//postgresql/extension' > /opt/homebrew/bin/ginstall -c -m 644 .//pair.control '/Users/david/Downloads/share/postgresql/extension/' > /opt/homebrew/bin/ginstall -c -m 644 .//sql/pair--0.1.2.sql .//sql/pair--unpackaged--0.1.2.sql '/Users/david/Downloads/share/postgresql/extension/' > /opt/homebrew/bin/ginstall -c -m 644 .//doc/pair.md '/Users/david/Downloads/share/doc//postgresql/extension/‘ > ``` > > I think this should at least be documented, but generally feels > unexpected to me. I’ve attached a patch that fleshes out the docs, > along with an example of setting `extension_control_path` and > `dynamic_library_path` to use the locations. It might not have the > information right about the need for “postgresql” or “pgsql” in the > path. Did you miss to attach the patch? > Everything else works very nicely except for extensions that use the > Makefile `MODULEDIR` variable to install all of the share files except > the control file into a particular directory, and the `directory` in > the control file so that the files can be found. Here’s semver[2], > which has both: > > ```console > ❯ make PG_CONFIG=~/dev/c/postgres/pgsql-devel/bin/pg_config prefix=/Users/david/Downloads/postgresql install > /opt/homebrew/bin/gmkdir -p '/Users/david/Downloads/postgresql/share/extension' > /opt/homebrew/bin/gmkdir -p '/Users/david/Downloads/postgresql/share/semver' > /opt/homebrew/bin/gmkdir -p '/Users/david/Downloads/postgresql/lib' > /opt/homebrew/bin/gmkdir -p '/Users/david/Downloads/postgresql/share/doc//semver' > /opt/homebrew/bin/ginstall -c -m 644 .//semver.control '/Users/david/Downloads/postgresql/share/extension/' > /opt/homebrew/bin/ginstall -c -m 644 .//sql/semver--0.10.0--0.11.0.sql .//sql/semver--0.11.0--0.12.0.sql .//sql/semver--0.12.0--0.13.0.sql .//sql/semver--0.13.0--0.15.0.sql .//sql/semver--0.15.0--0.16.0.sql .//sql/semver--0.16.0--0.17.0.sql .//sql/semver--0.17.0--0.20.0.sql .//sql/semver--0.2.1--0.2.4.sql .//sql/semver--0.2.4--0.3.0.sql .//sql/semver--0.20.0--0.21.0.sql .//sql/semver--0.21.0--0.22.0.sql .//sql/semver--0.22.0--0.30.0.sql .//sql/semver--0.3.0--0.4.0.sql .//sql/semver--0.30.0--0.31.0.sql .//sql/semver--0.31.0--0.31.1.sql .//sql/semver--0.31.1--0.31.2.sql .//sql/semver--0.31.2--0.32.0.sql .//sql/semver--0.32.0--0.32.1.sql .//sql/semver--0.32.1--0.40.0.sql .//sql/semver--0.32.1.sql .//sql/semver--0.40.0.sql .//sql/semver--0.5.0--0.10.0.sql .//sql/semver--unpackaged--0.2.1.sql .//sql/semver.sql '/Users/david/Downloads/postgresql/share/semver/' > /opt/homebrew/bin/ginstall -c -m 755 src/semver.dylib '/Users/david/Downloads/postgresql/lib/' > /opt/homebrew/bin/gmkdir -p '/Users/david/Downloads/postgresql/lib/bitcode/src/semver' > /opt/homebrew/bin/gmkdir -p '/Users/david/Downloads/postgresql/lib/bitcode'/src/semver/src/ > /opt/homebrew/bin/ginstall -c -m 644 src/semver.bc '/Users/david/Downloads/postgresql/lib/bitcode'/src/semver/src/ > cd '/Users/david/Downloads/postgresql/lib/bitcode' && /opt/homebrew/Cellar/llvm/19.1.7_1/bin/llvm-lto -thinlto -thinlto-action=thinlink -o src/semver.index.bc src/semver/src/semver.bc > /opt/homebrew/bin/ginstall -c -m 644 .//doc/semver.mmd '/Users/david/Downloads/postgresql/share/doc//semver/‘ > ``` > > Following `MODULEDIR=semver`, it puts the SQL files into > `share/semver/` instead of `share/extension/`, as expected, but then, > even though the control file has `directory=semver`, it can’t load > them: > > ```pgsql > david=# create extension semver; > ERROR: could not open directory "/Users/david/dev/c/postgres/pgsql-devel/share/semver": No such file or directory > ``` I've managed to reproduce the issue. The problem is on get_ext_ver_list which calls get_extension_script_directory that try to search for .sql files only on $sharedir. > Looks like it’s only looking in the `semver` subdirectory under > $libdir and not the whole path. > > But given that the `directory` variable in the control file can be a > full path, I don’t see that there’s much of a way to generalize a > solution. I guess there are three options: > > 1. If directory is a full path, try to load the files there. It > probably already works that way, though I haven’t tired it. > Yes, if the directory is a full path it try to load the files from there. It is implemented on get_extension_script_directory. > 2. If the directory is not a full path, check for it under each > directory in `extension_control_path`? But no, that points to > `share/extension`, not `share`, so it can’t really searched unless it > also lops off `extension` from the end of each path. Maybe we could make the "extension" part of the extension control path explicitly, like Peter has mentioned in his first patch version [1]?. If "directory" is not set we could use "extension" otherwise use the "directory" as a path suffix when searching on extension_control_path? [1] https://www.postgresql.org/message-id/0d384836-7e6e-4932-af3b-8dad1f6fee43%40eisentraut.org -- Matheus Alcantara ^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: RFC: Additional Directory for Extensions @ 2025-03-21 22:05 David E. Wheeler <[email protected]> parent: Matheus Alcantara <[email protected]> 0 siblings, 0 replies; 31+ messages in thread From: David E. Wheeler @ 2025-03-21 22:05 UTC (permalink / raw) To: Matheus Alcantara <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; Andrew Dunstan <[email protected]>; Gabriele Bartolini <[email protected]>; Craig Ringer <[email protected]>; Robert Haas <[email protected]>; Alvaro Herrera <[email protected]>; pgsql-hackers On Mar 21, 2025, at 17:52, Matheus Alcantara <[email protected]> wrote: > Did you miss to attach the patch? No. You can see it in the archive[1]. Direct link[2]. > Maybe we could make the "extension" part of the extension control path > explicitly, like Peter has mentioned in his first patch version [1]?. > If "directory" is not set we could use "extension" otherwise use the > "directory" as a path suffix when searching on extension_control_path? So, omit “extension” from the path options, append it to search for control files, and then append the directory value (if not absolute) if it exists to look for files, and otherwise append “extensions” to find them, too. I think that makes sense. Essentially it becomes a SHAREDIR search path. Best, David [1]: https://postgr.es/m/[email protected] [2]: https://www.postgresql.org/message-id/attachment/174397/v1-0001-Flesh-out-docs-for-the-prefix-make-v... Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: RFC: Additional Directory for Extensions @ 2025-03-22 13:28 Andrew Dunstan <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 0 replies; 31+ messages in thread From: Andrew Dunstan @ 2025-03-22 13:28 UTC (permalink / raw) To: Tom Lane <[email protected]>; Matheus Alcantara <[email protected]>; [email protected]; +Cc: Peter Eisentraut <[email protected]>; Gabriele Bartolini <[email protected]>; Craig Ringer <[email protected]>; David E. Wheeler <[email protected]>; Robert Haas <[email protected]>; Alvaro Herrera <[email protected]>; pgsql-hackers On 2025-03-21 Fr 12:42 PM, Tom Lane wrote: > Matheus Alcantara<[email protected]> writes: >> On Thu, Mar 20, 2025 at 7:38 PM Andrew Dunstan<[email protected]> wrote: >>>> (wondering if this another of these cases where the "path includes postgres" thing bites us, and we're looking in the wrong place) >>> Nope, testing shows it's not that, so I am rather confused about what was going on. >> I'm not sure if I'm checking on the right place [1] but it seems that the >> Contrib and ContribInstall is executed after Check step which causes this test >> failure? > No, this is not failing in Check. > > I did just notice a clue though: on snakefly, the failing step's > log [1] includes > > make[1]: Leaving directory `/opt/postgres/build-farm-18/HEAD/pgsql.build/src/backend' > rm -rf '/opt/postgres/build-farm-18/HEAD/pgsql.build'/tmp_install > /usr/bin/mkdir -p '/opt/postgres/build-farm-18/HEAD/pgsql.build'/tmp_install/log > make -C '../../../..' DESTDIR='/opt/postgres/build-farm-18/HEAD/pgsql.build'/tmp_install install >'/opt/postgres/build-farm-18/HEAD/pgsql.build'/tmp_install/log/install.log 2>&1 > make -j1 checkprep >>'/opt/postgres/build-farm-18/HEAD/pgsql.build'/tmp_install/log/install.log 2>&1 > PATH="/opt/postgres/build-farm-18/HEAD/pgsql.build/tmp_install/opt/postgres/build-farm-18/HEAD/inst/bin:/opt/postgres/build-farm-18/HEAD/pgsql.build/src/test/modules/test_extensions:$PATH" LD_LIBRARY_PATH="/opt/postgres/build-farm-18/HEAD/pgsql.build/tmp_install/opt/postgres/build-farm-18/HEAD/inst/lib:$LD_LIBRARY_PATH" INITDB_TEMPLATE='/opt/postgres/build-farm-18/HEAD/pgsql.build'/tmp_install/initdb-template initdb --auth trust --no-sync --no-instructions --lc-messages=C --no-clean '/opt/postgres/build-farm-18/HEAD/pgsql.build'/tmp_install/initdb-template >>'/opt/postgres/build-farm-18/HEAD/pgsql.build'/tmp_install/log/initdb-template.log 2>&1 > echo "# +++ regress check in src/test/modules/test_extensions +++" && PATH="/opt/postgres/build-farm-18/HEAD/pgsql.build/tmp_install/opt/postgres/build-farm-18/HEAD/inst/bin:/opt/postgres/build-farm-18/HEAD/pgsql.build/src/test/modules/test_extensions:$PATH" LD_LIBRARY_PATH="/opt/postgres/build-farm-18/HEAD/pgsql.build/tmp_install/opt/postgres/build-farm-18/HEAD/inst/lib:$LD_LIBRARY_PATH" INITDB_TEMPLATE='/opt/postgres/build-farm-18/HEAD/pgsql.build'/tmp_install/initdb-template ../../../../src/test/regress/pg_regress --temp-instance=./tmp_check --inputdir=. --bindir= --temp-config=/opt/postgres/build-farm-18/tmp/buildfarm-C9Iy3s/bfextra.conf --no-locale --port=5678 --dbname=contrib_regression test_extensions test_extdepend > # +++ regress check in src/test/modules/test_extensions +++ > # initializing database system by running initdb > > showing that the step made its own tmp_install, and that only the core > "install" process was executed, so the lack of amcheck in that install > tree is not surprising. But concurrent runs on other animals, eg [2], > don't show a tmp_install rebuild happening. So those are using an > installation tree that *does* include contrib modules. > > So what this comes down to is "why is snakefly doing a fresh install > here?". I don't know the buildfarm client well enough to identify > probable causes. I do note that Makefile.global.in conditionalizes > tmp_install rebuild on several variables: > > temp-install: | submake-generated-headers > ifndef NO_TEMP_INSTALL > ifneq ($(abs_top_builddir),) > ifeq ($(MAKELEVEL),0) > rm -rf '$(abs_top_builddir)'/tmp_install > $(MKDIR_P) '$(abs_top_builddir)'/tmp_install/log > $(MAKE) -C '$(top_builddir)' DESTDIR='$(abs_top_builddir)'/tmp_install install >'$(abs_top_builddir)'/tmp_install/log/install.log 2>&1 > $(MAKE) -j1 $(if $(CHECKPREP_TOP),-C $(CHECKPREP_TOP),) checkprep >>'$(abs_top_builddir)'/tmp_install/log/install.log 2>&1 Good catch. This is happening because the owner hasn't updated the animal to REL_19_1. In 19 and 19.1 we fixed detection of exiting installs to take account of the 'Is there postgres or pgsql in the prefix' issue. So it was looking in the wrong place. cheers andrew -- Andrew Dunstan EDB:https://www.enterprisedb.com ^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: RFC: Additional Directory for Extensions @ 2025-04-24 22:59 David E. Wheeler <[email protected]> parent: David E. Wheeler <[email protected]> 1 sibling, 1 reply; 31+ messages in thread From: David E. Wheeler @ 2025-04-24 22:59 UTC (permalink / raw) To: Peter Eisentraut <[email protected]>; +Cc: Matheus Alcantara <[email protected]>; Andrew Dunstan <[email protected]>; Gabriele Bartolini <[email protected]>; Craig Ringer <[email protected]>; Robert Haas <[email protected]>; Alvaro Herrera <[email protected]>; pgsql-hackers On Mar 19, 2025, at 13:29, David E. Wheeler <[email protected]> wrote: > I think this should at least be documented, but generally feels unexpected to me. I’ve attached a patch that fleshes out the docs, along with an example of setting `extension_control_path` and `dynamic_library_path` to use the locations. It might not have the information right about the need for “postgresql” or “pgsql” in the path. Back in 2003[1] it was just “postgres”, but I couldn’t find the logic for it just now. Here’s a rebase. Best, David Attachments: [application/octet-stream] v2-0001-Flesh-out-docs-for-the-prefix-make-variable.patch (2.6K, ../../[email protected]/2-v2-0001-Flesh-out-docs-for-the-prefix-make-variable.patch) download | inline diff: From 448a03ac53fd145eb5da63c96ac99a99876de642 Mon Sep 17 00:00:00 2001 From: "David E. Wheeler" <[email protected]> Date: Thu, 24 Apr 2025 18:57:26 -0400 Subject: [PATCH v2] Flesh out docs for the `prefix` make variable The variable is a bit magical in how it requires "postgresql" or "pgsql" to be part of the path, and files end up in its `share` and `lib` subdirectories. So mention all that and show an example of setting `extension_control_path` and `dynamic_library_path` to use those locations. --- doc/src/sgml/extend.sgml | 33 +++++++++++++++++++++++++++++---- 1 file changed, 29 insertions(+), 4 deletions(-) diff --git a/doc/src/sgml/extend.sgml b/doc/src/sgml/extend.sgml index 64f8e133cae..4e75a01fae4 100644 --- a/doc/src/sgml/extend.sgml +++ b/doc/src/sgml/extend.sgml @@ -1809,10 +1809,35 @@ include $(PGXS) setting <varname>PG_CONFIG</varname> to point to its <command>pg_config</command> program, either within the makefile or on the <literal>make</literal> command line. - You can also select a separate installation directory for your extension - by setting the <literal>make</literal> variable <varname>prefix</varname> - on the <literal>make</literal> command line. (But this will then require - additional setup to get the server to find the extension there.) + </para> + + <para> + You can also select a separate directory prefix in which to install your + extension's files by setting the <literal>make</literal> variable + <varname>prefix</varname> when executing <literal>make install</literal> + like so: +<programlisting> +make install prefix=/etc/postgresql +</programlisting> + This will install the control SQL files into + <literal>/etc/postgresql/share</literal> and shared modules into + <literal>/etc/postgresql/lib</literal>. If the prefix does not + include the strings <literal>postgresql</literal> or + <literal>pgsql</literal>, such as: +<programlisting> +make install prefix=/etc/extras +</programlisting> + Then the <literal>postgresql</literal> directory will be appended io the + prefix, installing the control SQL files into + <literal>/etc/extras/postgresql/share</literal> and shared modules into + <literal>/etc/extras/postgresql/lib</literal>. Either way, you'll need to + set <xref linkend="guc-extension-control-path"/> and <xref + linkend="guc-dynamic-library-path"/> to allow + <productname>PostgreSQL</productname> to find the files: +</programlisting> +extension_control_path = '/etc/extras/postgresql/share/extension:$system' +dynamic_library_path = '/etc/extras/postgresql/lib:$libdir' + </programlisting> </para> <para> -- 2.48.1 [application/pgp-signature] signature.asc (833B, ../../[email protected]/3-signature.asc) download ^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: RFC: Additional Directory for Extensions @ 2025-04-25 11:33 Christoph Berg <[email protected]> parent: David E. Wheeler <[email protected]> 0 siblings, 1 reply; 31+ messages in thread From: Christoph Berg @ 2025-04-25 11:33 UTC (permalink / raw) To: David E. Wheeler <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; Matheus Alcantara <[email protected]>; Andrew Dunstan <[email protected]>; Gabriele Bartolini <[email protected]>; Craig Ringer <[email protected]>; Robert Haas <[email protected]>; Alvaro Herrera <[email protected]>; pgsql-hackers Re: David E. Wheeler > +<programlisting> > +make install prefix=/etc/postgresql I'd use /usr/local/postgresql there. "/etc" is just wrong. > +</programlisting> > + This will install the control SQL files into > + <literal>/etc/postgresql/share</literal> and shared modules into > + <literal>/etc/postgresql/lib</literal>. If the prefix does not > + include the strings <literal>postgresql</literal> or Just "postgres", see src/Makefile.global.in:86. > + <literal>pgsql</literal>, such as: > +<programlisting> > +make install prefix=/etc/extras /usr/local/extras > +</programlisting> > + Then the <literal>postgresql</literal> directory will be appended io the > + prefix, installing the control SQL files into "the extension control and SQL files" > + <literal>/etc/extras/postgresql/share</literal> and shared modules into .../postgresql/share/extension because ... > + <literal>/etc/extras/postgresql/lib</literal>. Either way, you'll need to > + set <xref linkend="guc-extension-control-path"/> and <xref > + linkend="guc-dynamic-library-path"/> to allow > + <productname>PostgreSQL</productname> to find the files: > +</programlisting> > +extension_control_path = '/etc/extras/postgresql/share/extension:$system' ... it's used here. Christoph ^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: RFC: Additional Directory for Extensions @ 2025-04-25 19:23 David E. Wheeler <[email protected]> parent: Christoph Berg <[email protected]> 0 siblings, 1 reply; 31+ messages in thread From: David E. Wheeler @ 2025-04-25 19:23 UTC (permalink / raw) To: Christoph Berg <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; Matheus Alcantara <[email protected]>; Andrew Dunstan <[email protected]>; Gabriele Bartolini <[email protected]>; Craig Ringer <[email protected]>; Robert Haas <[email protected]>; Alvaro Herrera <[email protected]>; pgsql-hackers On Apr 25, 2025, at 07:33, Christoph Berg <[email protected]> wrote: > > Re: David E. Wheeler >> +<programlisting> >> +make install prefix=/etc/postgresql > > I'd use /usr/local/postgresql there. "/etc" is just wrong. Thank you for the review. Here’s v3*. Best, David * Also reviewable as a GitHub PR[1]. [1]: https://github.com/theory/postgres/pull/10 Attachments: [application/octet-stream] v3-0001-Flesh-out-docs-for-the-prefix-make-variable.patch (2.7K, ../../[email protected]/2-v3-0001-Flesh-out-docs-for-the-prefix-make-variable.patch) download | inline diff: From d49d3445ca5bdde436713dc8a2ae7707683851e3 Mon Sep 17 00:00:00 2001 From: "David E. Wheeler" <[email protected]> Date: Fri, 25 Apr 2025 15:22:23 -0400 Subject: [PATCH v3] Flesh out docs for the `prefix` make variable The variable is a bit magical in how it requires "postgresql" or "pgsql" to be part of the path, and files end up in its `share` and `lib` subdirectories. So mention all that and show an example of setting `extension_control_path` and `dynamic_library_path` to use those locations. --- doc/src/sgml/extend.sgml | 33 +++++++++++++++++++++++++++++---- 1 file changed, 29 insertions(+), 4 deletions(-) diff --git a/doc/src/sgml/extend.sgml b/doc/src/sgml/extend.sgml index 64f8e133cae..05063d4e7bc 100644 --- a/doc/src/sgml/extend.sgml +++ b/doc/src/sgml/extend.sgml @@ -1809,10 +1809,35 @@ include $(PGXS) setting <varname>PG_CONFIG</varname> to point to its <command>pg_config</command> program, either within the makefile or on the <literal>make</literal> command line. - You can also select a separate installation directory for your extension - by setting the <literal>make</literal> variable <varname>prefix</varname> - on the <literal>make</literal> command line. (But this will then require - additional setup to get the server to find the extension there.) + </para> + + <para> + You can also select a separate directory prefix in which to install your + extension's files by setting the <literal>make</literal> variable + <varname>prefix</varname> when executing <literal>make install</literal> + like so: +<programlisting> +make install prefix=/usr/local/postgresql +</programlisting> + This will install the control SQL files into + <literal>/usr/local/postgresql/share</literal> and shared modules into + <literal>/usr/local/postgresql/lib</literal>. If the prefix does not + include the strings <literal>postgres</literal> or + <literal>pgsql</literal>, such as: +<programlisting> +make install prefix=/usr/local/extras +</programlisting> + Then the <literal>postgresql</literal> directory will be appended io the + prefix, installing the control and SQL files into + <literal>/usr/local/extras/postgresql/share/extension</literal> and shared + modules into <literal>/usr/local/extras/postgresql/lib</literal>. Either + way, you'll need to set <xref linkend="guc-extension-control-path"/> and + <xref linkend="guc-dynamic-library-path"/> to allow + <productname>PostgreSQL</productname> to find the files: +</programlisting> +extension_control_path = '/usr/local/extras/postgresql/share/extension:$system' +dynamic_library_path = '/usr/local/extras/postgresql/lib:$libdir' + </programlisting> </para> <para> -- 2.48.1 [application/pgp-signature] signature.asc (833B, ../../[email protected]/4-signature.asc) download ^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: RFC: Additional Directory for Extensions @ 2025-04-28 21:14 David E. Wheeler <[email protected]> parent: David E. Wheeler <[email protected]> 0 siblings, 1 reply; 31+ messages in thread From: David E. Wheeler @ 2025-04-28 21:14 UTC (permalink / raw) To: Christoph Berg <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; Matheus Alcantara <[email protected]>; Andrew Dunstan <[email protected]>; Gabriele Bartolini <[email protected]>; Craig Ringer <[email protected]>; Robert Haas <[email protected]>; Alvaro Herrera <[email protected]>; pgsql-hackers On Apr 25, 2025, at 15:23, David E. Wheeler <[email protected]> wrote: > Thank you for the review. Here’s v3*. V4 removes “/extension” from the end of the `extension_control_path` value. Best, David Attachments: [application/octet-stream] v4-0001-Flesh-out-docs-for-the-prefix-make-variable.patch (2.7K, ../../[email protected]/2-v4-0001-Flesh-out-docs-for-the-prefix-make-variable.patch) download | inline diff: From 8ff0470bd6b1110c43e9852c5196139df7b2734e Mon Sep 17 00:00:00 2001 From: "David E. Wheeler" <[email protected]> Date: Mon, 28 Apr 2025 17:13:20 -0400 Subject: [PATCH v4] Flesh out docs for the `prefix` make variable The variable is a bit magical in how it requires "postgresql" or "pgsql" to be part of the path, and files end up in its `share` and `lib` subdirectories. So mention all that and show an example of setting `extension_control_path` and `dynamic_library_path` to use those locations. --- doc/src/sgml/extend.sgml | 33 +++++++++++++++++++++++++++++---- 1 file changed, 29 insertions(+), 4 deletions(-) diff --git a/doc/src/sgml/extend.sgml b/doc/src/sgml/extend.sgml index 64f8e133cae..68358a5b15f 100644 --- a/doc/src/sgml/extend.sgml +++ b/doc/src/sgml/extend.sgml @@ -1809,10 +1809,35 @@ include $(PGXS) setting <varname>PG_CONFIG</varname> to point to its <command>pg_config</command> program, either within the makefile or on the <literal>make</literal> command line. - You can also select a separate installation directory for your extension - by setting the <literal>make</literal> variable <varname>prefix</varname> - on the <literal>make</literal> command line. (But this will then require - additional setup to get the server to find the extension there.) + </para> + + <para> + You can also select a separate directory prefix in which to install your + extension's files by setting the <literal>make</literal> variable + <varname>prefix</varname> when executing <literal>make install</literal> + like so: +<programlisting> +make install prefix=/usr/local/postgresql +</programlisting> + This will install the control SQL files into + <literal>/usr/local/postgresql/share</literal> and shared modules into + <literal>/usr/local/postgresql/lib</literal>. If the prefix does not + include the strings <literal>postgres</literal> or + <literal>pgsql</literal>, such as: +<programlisting> +make install prefix=/usr/local/extras +</programlisting> + Then the <literal>postgresql</literal> directory will be appended io the + prefix, installing the control and SQL files into + <literal>/usr/local/extras/postgresql/share/extension</literal> and shared + modules into <literal>/usr/local/extras/postgresql/lib</literal>. Either + way, you'll need to set <xref linkend="guc-extension-control-path"/> and + <xref linkend="guc-dynamic-library-path"/> to allow + <productname>PostgreSQL</productname> to find the files: +</programlisting> +extension_control_path = '/usr/local/extras/postgresql/share:$system' +dynamic_library_path = '/usr/local/extras/postgresql/lib:$libdir' + </programlisting> </para> <para> -- 2.49.0 [application/pgp-signature] signature.asc (833B, ../../[email protected]/3-signature.asc) download ^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: RFC: Additional Directory for Extensions @ 2025-05-01 11:50 Peter Eisentraut <[email protected]> parent: David E. Wheeler <[email protected]> 0 siblings, 1 reply; 31+ messages in thread From: Peter Eisentraut @ 2025-05-01 11:50 UTC (permalink / raw) To: David E. Wheeler <[email protected]>; +Cc: Matheus Alcantara <[email protected]>; pgsql-hackers; Christoph Berg <[email protected]> On 28.04.25 23:14, David E. Wheeler wrote: > On Apr 25, 2025, at 15:23, David E. Wheeler <[email protected]> wrote: > >> Thank you for the review. Here’s v3*. > > V4 removes “/extension” from the end of the `extension_control_path` value. The documentation in config.sgml says: Note that the path elements should typically end in <literal>extension</literal> if the normal installation layouts are followed. So I think your change here between v3 and v4 is incorrect. ^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: RFC: Additional Directory for Extensions @ 2025-05-01 14:31 David E. Wheeler <[email protected]> parent: Peter Eisentraut <[email protected]> 0 siblings, 1 reply; 31+ messages in thread From: David E. Wheeler @ 2025-05-01 14:31 UTC (permalink / raw) To: Peter Eisentraut <[email protected]>; +Cc: Matheus Alcantara <[email protected]>; pgsql-hackers; Christoph Berg <[email protected]> On May 1, 2025, at 07:50, Peter Eisentraut <[email protected]> wrote: > The documentation in config.sgml says: > > Note that the path elements should typically end in > <literal>extension</literal> if the normal installation layouts are > followed. > > So I think your change here between v3 and v4 is incorrect. Right, sorry, forgot about that, I updated it in anticipation of Matheus’s patch[1] being committed. So v3 is fine for now, but if that patch is committed, we’ll need to reconcile those docs. Best, David [1]: https://postgr.es/m/CAFY6G8dUXHRii5rNy7V8WmBrmBwp9W7y3g+HL6Tn-Lu8KkvK=A@mail.gmail.com Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: RFC: Additional Directory for Extensions @ 2025-05-01 20:24 Peter Eisentraut <[email protected]> parent: David E. Wheeler <[email protected]> 0 siblings, 2 replies; 31+ messages in thread From: Peter Eisentraut @ 2025-05-01 20:24 UTC (permalink / raw) To: David E. Wheeler <[email protected]>; +Cc: Matheus Alcantara <[email protected]>; pgsql-hackers; Christoph Berg <[email protected]> On 01.05.25 16:31, David E. Wheeler wrote: > On May 1, 2025, at 07:50, Peter Eisentraut <[email protected]> wrote: > >> The documentation in config.sgml says: >> >> Note that the path elements should typically end in >> <literal>extension</literal> if the normal installation layouts are >> followed. >> >> So I think your change here between v3 and v4 is incorrect. > > Right, sorry, forgot about that, I updated it in anticipation of Matheus’s patch[1] being committed. > > So v3 is fine for now, but if that patch is committed, we’ll need to reconcile those docs. I see. I have committed it now describing the current state. Btw., the shown directory names that illustrate how "postgresql" is appended were not correct. I have corrected that. ^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: RFC: Additional Directory for Extensions @ 2025-05-01 21:01 David E. Wheeler <[email protected]> parent: Peter Eisentraut <[email protected]> 1 sibling, 0 replies; 31+ messages in thread From: David E. Wheeler @ 2025-05-01 21:01 UTC (permalink / raw) To: Peter Eisentraut <[email protected]>; +Cc: Matheus Alcantara <[email protected]>; pgsql-hackers; Christoph Berg <[email protected]> On May 1, 2025, at 16:24, Peter Eisentraut <[email protected]> wrote: > I see. I have committed it now describing the current state. > > Btw., the shown directory names that illustrate how "postgresql" is appended were not correct. I have corrected that. Thank you. I gotta say I find them confusing TBH. Best, David Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: RFC: Additional Directory for Extensions @ 2025-05-02 16:48 David E. Wheeler <[email protected]> parent: Peter Eisentraut <[email protected]> 1 sibling, 0 replies; 31+ messages in thread From: David E. Wheeler @ 2025-05-02 16:48 UTC (permalink / raw) To: Peter Eisentraut <[email protected]>; +Cc: Matheus Alcantara <[email protected]>; pgsql-hackers; Christoph Berg <[email protected]> On May 1, 2025, at 16:24, Peter Eisentraut <[email protected]> wrote: > I see. I have committed it now describing the current state. Quick follow-up to tweak a couple of commas. --- a/doc/src/sgml/extend.sgml +++ b/doc/src/sgml/extend.sgml @@ -1813,8 +1813,8 @@ include $(PGXS) <para> You can select a separate directory prefix in which to install your - extension's files, by setting the <command>make</command> variable - <varname>prefix</varname> when executing <literal>make install</literal> + extension's files by setting the <command>make</command> variable + <varname>prefix</varname> when executing <literal>make install</literal>, like so: <programlisting> make install prefix=/usr/local/postgresql Best, David Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 31+ messages in thread
end of thread, other threads:[~2025-05-02 16:48 UTC | newest] Thread overview: 31+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2019-09-12 14:35 [PATCH] Avoid touching user indexes while they are being (re)built. Arseny Sher <[email protected]> 2024-03-08 21:45 [PATCH v3 07/17] Execute freezing in heap_page_prune() Melanie Plageman <[email protected]> 2025-03-10 20:25 Re: RFC: Additional Directory for Extensions Matheus Alcantara <[email protected]> 2025-03-11 15:58 ` Re: RFC: Additional Directory for Extensions Peter Eisentraut <[email protected]> 2025-03-12 13:17 ` Re: RFC: Additional Directory for Extensions Matheus Alcantara <[email protected]> 2025-03-19 06:42 ` Re: RFC: Additional Directory for Extensions Peter Eisentraut <[email protected]> 2025-03-19 07:03 ` Re: RFC: Additional Directory for Extensions Gabriele Bartolini <[email protected]> 2025-03-19 14:58 ` Re: RFC: Additional Directory for Extensions Christoph Berg <[email protected]> 2025-03-19 17:29 ` Re: RFC: Additional Directory for Extensions David E. Wheeler <[email protected]> 2025-03-21 21:52 ` Re: RFC: Additional Directory for Extensions Matheus Alcantara <[email protected]> 2025-03-21 22:05 ` Re: RFC: Additional Directory for Extensions David E. Wheeler <[email protected]> 2025-04-24 22:59 ` Re: RFC: Additional Directory for Extensions David E. Wheeler <[email protected]> 2025-04-25 11:33 ` Re: RFC: Additional Directory for Extensions Christoph Berg <[email protected]> 2025-04-25 19:23 ` Re: RFC: Additional Directory for Extensions David E. Wheeler <[email protected]> 2025-04-28 21:14 ` Re: RFC: Additional Directory for Extensions David E. Wheeler <[email protected]> 2025-05-01 11:50 ` Re: RFC: Additional Directory for Extensions Peter Eisentraut <[email protected]> 2025-05-01 14:31 ` Re: RFC: Additional Directory for Extensions David E. Wheeler <[email protected]> 2025-05-01 20:24 ` Re: RFC: Additional Directory for Extensions Peter Eisentraut <[email protected]> 2025-05-01 21:01 ` Re: RFC: Additional Directory for Extensions David E. Wheeler <[email protected]> 2025-05-02 16:48 ` Re: RFC: Additional Directory for Extensions David E. Wheeler <[email protected]> 2025-03-19 18:55 ` Re: RFC: Additional Directory for Extensions Tom Lane <[email protected]> 2025-03-19 19:25 ` Re: RFC: Additional Directory for Extensions Matheus Alcantara <[email protected]> 2025-03-19 19:42 ` Re: RFC: Additional Directory for Extensions Tom Lane <[email protected]> 2025-03-20 10:01 ` Re: RFC: Additional Directory for Extensions Peter Eisentraut <[email protected]> 2025-03-20 14:35 ` Re: RFC: Additional Directory for Extensions Tom Lane <[email protected]> 2025-03-20 14:53 ` Re: RFC: Additional Directory for Extensions Andrew Dunstan <[email protected]> 2025-03-20 22:38 ` Re: RFC: Additional Directory for Extensions Andrew Dunstan <[email protected]> 2025-03-21 15:52 ` Re: RFC: Additional Directory for Extensions Matheus Alcantara <[email protected]> 2025-03-21 16:38 ` Re: RFC: Additional Directory for Extensions Andrew Dunstan <[email protected]> 2025-03-21 16:42 ` Re: RFC: Additional Directory for Extensions Tom Lane <[email protected]> 2025-03-22 13:28 ` Re: RFC: Additional Directory for Extensions Andrew Dunstan <[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