public inbox for [email protected]  

help / color / mirror / Atom feed

[PATCH 5/5] Remove the GUC stats_temp_directory
16+ messages / 6 participants
[nested] [flat]

* [PATCH 5/5] Remove the GUC stats_temp_directory
@ 2018-11-27 05:42  Kyotaro Horiguchi <[email protected]>
  0 siblings, 0 replies; 16+ messages in thread

From: Kyotaro Horiguchi @ 2018-11-27 05:42 UTC (permalink / raw)

The guc used to specifie the directory to store temporary statistics
files. It is no longer needed by the stats collector but still used by
the programs in bin and contirb, and maybe other extensions. Thus this
patch removes the GUC but some backing variables and macro definitions
are left alone for backward comptibility.
---
 doc/src/sgml/backup.sgml                      |  2 --
 doc/src/sgml/config.sgml                      | 19 -------------
 doc/src/sgml/monitoring.sgml                  |  7 +----
 doc/src/sgml/storage.sgml                     |  3 +-
 src/backend/postmaster/pgstat.c               | 13 ++++-----
 src/backend/replication/basebackup.c          | 13 ++-------
 src/backend/utils/misc/guc.c                  | 41 ---------------------------
 src/backend/utils/misc/postgresql.conf.sample |  1 -
 src/include/pgstat.h                          |  5 +++-
 9 files changed, 14 insertions(+), 90 deletions(-)

diff --git a/doc/src/sgml/backup.sgml b/doc/src/sgml/backup.sgml
index b67da8916a..f0daffbc93 100644
--- a/doc/src/sgml/backup.sgml
+++ b/doc/src/sgml/backup.sgml
@@ -1145,8 +1145,6 @@ SELECT pg_stop_backup();
     <filename>pg_snapshots/</filename>, <filename>pg_stat_tmp/</filename>,
     and <filename>pg_subtrans/</filename> (but not the directories themselves) can be
     omitted from the backup as they will be initialized on postmaster startup.
-    If <xref linkend="guc-stats-temp-directory"/> is set and is under the data
-    directory then the contents of that directory can also be omitted.
    </para>
 
    <para>
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 84341a30e5..6ad619de47 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -6827,25 +6827,6 @@ COPY postgres_log FROM '/full/path/to/logfile.csv' WITH csv;
       </listitem>
      </varlistentry>
 
-     <varlistentry id="guc-stats-temp-directory" xreflabel="stats_temp_directory">
-      <term><varname>stats_temp_directory</varname> (<type>string</type>)
-      <indexterm>
-       <primary><varname>stats_temp_directory</varname> configuration parameter</primary>
-      </indexterm>
-      </term>
-      <listitem>
-       <para>
-        Sets the directory to store temporary statistics data in. This can be
-        a path relative to the data directory or an absolute path. The default
-        is <filename>pg_stat_tmp</filename>. Pointing this at a RAM-based
-        file system will decrease physical I/O requirements and can lead to
-        improved performance.
-        This parameter can only be set in the <filename>postgresql.conf</filename>
-        file or on the server command line.
-       </para>
-      </listitem>
-     </varlistentry>
-
      </variablelist>
     </sect2>
 
diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index 0fe888e4db..31fc3604ea 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -195,12 +195,7 @@ postgres   27093  0.0  0.0  30096  2752 ?        Ss   11:34   0:00 postgres: ser
 
   <para>
    The statistics collector transmits the collected information to other
-   <productname>PostgreSQL</productname> processes through temporary files.
-   These files are stored in the directory named by the
-   <xref linkend="guc-stats-temp-directory"/> parameter,
-   <filename>pg_stat_tmp</filename> by default.
-   For better performance, <varname>stats_temp_directory</varname> can be
-   pointed at a RAM-based file system, decreasing physical I/O requirements.
+   <productname>PostgreSQL</productname> processes through shared memory.
    When the server shuts down cleanly, a permanent copy of the statistics
    data is stored in the <filename>pg_stat</filename> subdirectory, so that
    statistics can be retained across server restarts.  When recovery is
diff --git a/doc/src/sgml/storage.sgml b/doc/src/sgml/storage.sgml
index c4bac87e80..c264f839e7 100644
--- a/doc/src/sgml/storage.sgml
+++ b/doc/src/sgml/storage.sgml
@@ -122,8 +122,7 @@ Item
 
 <row>
  <entry><filename>pg_stat_tmp</filename></entry>
- <entry>Subdirectory containing temporary files for the statistics
-  subsystem</entry>
+ <entry>Subdirectory containing ephemeral files for extensions</entry>
 </row>
 
 <row>
diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index 24dab5aa54..94df252597 100644
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -109,15 +109,12 @@ bool		pgstat_track_counts = false;
 int			pgstat_track_functions = TRACK_FUNC_OFF;
 int			pgstat_track_activity_query_size = 1024;
 
-/* ----------
- * Built from GUC parameter
- * ----------
+/*
+ * This used to be a GUC variable and is no longer used in this file, but left
+ * alone just for backward comptibility for extensions, having the default
+ * value.
  */
-char	   *pgstat_stat_directory = NULL;
-
-/* No longer used, but will be removed with GUC */
-char	   *pgstat_stat_filename = NULL;
-char	   *pgstat_stat_tmpname = NULL;
+char	   *pgstat_stat_directory = PG_STAT_TMP_DIR;
 
 #define		StatsLock (&StatsShmem->StatsMainLock)
 
diff --git a/src/backend/replication/basebackup.c b/src/backend/replication/basebackup.c
index 36dcb28754..5b6de31ff6 100644
--- a/src/backend/replication/basebackup.c
+++ b/src/backend/replication/basebackup.c
@@ -230,11 +230,8 @@ perform_base_backup(basebackup_options *opt)
 	TimeLineID	endtli;
 	StringInfo	labelfile;
 	StringInfo	tblspc_map_file = NULL;
-	int			datadirpathlen;
 	List	   *tablespaces = NIL;
 
-	datadirpathlen = strlen(DataDir);
-
 	backup_started_in_recovery = RecoveryInProgress();
 
 	labelfile = makeStringInfo();
@@ -265,13 +262,9 @@ perform_base_backup(basebackup_options *opt)
 		 * Calculate the relative path of temporary statistics directory in
 		 * order to skip the files which are located in that directory later.
 		 */
-		if (is_absolute_path(pgstat_stat_directory) &&
-			strncmp(pgstat_stat_directory, DataDir, datadirpathlen) == 0)
-			statrelpath = psprintf("./%s", pgstat_stat_directory + datadirpathlen + 1);
-		else if (strncmp(pgstat_stat_directory, "./", 2) != 0)
-			statrelpath = psprintf("./%s", pgstat_stat_directory);
-		else
-			statrelpath = pgstat_stat_directory;
+
+		Assert(strchr(PG_STAT_TMP_DIR, '/') == NULL);
+		statrelpath = psprintf("./%s", PG_STAT_TMP_DIR);
 
 		/* Add a node for the base directory at the end */
 		ti = palloc0(sizeof(tablespaceinfo));
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index ed51da4234..5c7e83ffdb 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -194,7 +194,6 @@ static bool check_max_wal_senders(int *newval, void **extra, GucSource source);
 static bool check_autovacuum_work_mem(int *newval, void **extra, GucSource source);
 static bool check_effective_io_concurrency(int *newval, void **extra, GucSource source);
 static void assign_effective_io_concurrency(int newval, void *extra);
-static void assign_pgstat_temp_directory(const char *newval, void *extra);
 static bool check_application_name(char **newval, void **extra, GucSource source);
 static void assign_application_name(const char *newval, void *extra);
 static bool check_cluster_name(char **newval, void **extra, GucSource source);
@@ -4085,17 +4084,6 @@ static struct config_string ConfigureNamesString[] =
 		NULL, NULL, NULL
 	},
 
-	{
-		{"stats_temp_directory", PGC_SIGHUP, STATS_COLLECTOR,
-			gettext_noop("Writes temporary statistics files to the specified directory."),
-			NULL,
-			GUC_SUPERUSER_ONLY
-		},
-		&pgstat_temp_directory,
-		PG_STAT_TMP_DIR,
-		check_canonical_path, assign_pgstat_temp_directory, NULL
-	},
-
 	{
 		{"synchronous_standby_names", PGC_SIGHUP, REPLICATION_MASTER,
 			gettext_noop("Number of synchronous standbys and list of names of potential synchronous ones."),
@@ -11363,35 +11351,6 @@ assign_effective_io_concurrency(int newval, void *extra)
 #endif							/* USE_PREFETCH */
 }
 
-static void
-assign_pgstat_temp_directory(const char *newval, void *extra)
-{
-	/* check_canonical_path already canonicalized newval for us */
-	char	   *dname;
-	char	   *tname;
-	char	   *fname;
-
-	/* directory */
-	dname = guc_malloc(ERROR, strlen(newval) + 1);	/* runtime dir */
-	sprintf(dname, "%s", newval);
-
-	/* global stats */
-	tname = guc_malloc(ERROR, strlen(newval) + 12); /* /global.tmp */
-	sprintf(tname, "%s/global.tmp", newval);
-	fname = guc_malloc(ERROR, strlen(newval) + 13); /* /global.stat */
-	sprintf(fname, "%s/global.stat", newval);
-
-	if (pgstat_stat_directory)
-		free(pgstat_stat_directory);
-	pgstat_stat_directory = dname;
-	if (pgstat_stat_tmpname)
-		free(pgstat_stat_tmpname);
-	pgstat_stat_tmpname = tname;
-	if (pgstat_stat_filename)
-		free(pgstat_stat_filename);
-	pgstat_stat_filename = fname;
-}
-
 static bool
 check_application_name(char **newval, void **extra, GucSource source)
 {
diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample
index 5ee5e09ddf..0ba984b074 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -562,7 +562,6 @@
 #track_io_timing = off
 #track_functions = none			# none, pl, all
 #track_activity_query_size = 1024	# (change requires restart)
-#stats_temp_directory = 'pg_stat_tmp'
 
 
 # - Monitoring -
diff --git a/src/include/pgstat.h b/src/include/pgstat.h
index 2196b0bd38..4e0a05b2ce 100644
--- a/src/include/pgstat.h
+++ b/src/include/pgstat.h
@@ -31,7 +31,10 @@
 #define PGSTAT_STAT_PERMANENT_FILENAME		"pg_stat/global.stat"
 #define PGSTAT_STAT_PERMANENT_TMPFILE		"pg_stat/global.tmp"
 
-/* Default directory to store temporary statistics data in */
+/*
+ * This used to be the directory to store temporary statistics data in but is
+ * no longer used. Defined here for backward compatibility.
+ */
 #define PG_STAT_TMP_DIR		"pg_stat_tmp"
 
 /* Values for track_functions GUC variable --- order is significant! */
-- 
2.16.3


----Next_Part(Fri_May_17_15_47_20_2019_554)----






^ permalink  raw  reply  [nested|flat] 16+ messages in thread

* [PATCH 3/4] GiST-Optimal-WAL-Usage
@ 2019-04-01 04:07  Andrey V. Lepikhov <[email protected]>
  0 siblings, 0 replies; 16+ messages in thread

From: Andrey V. Lepikhov @ 2019-04-01 04:07 UTC (permalink / raw)

---
 src/backend/access/gist/gist.c         | 46 ++++++++++++++++++--------
 src/backend/access/gist/gistbuild.c    | 32 ++++++++++--------
 src/backend/access/gist/gistutil.c     |  2 +-
 src/backend/access/gist/gistxlog.c     | 22 ------------
 src/backend/access/rmgrdesc/gistdesc.c |  5 ---
 src/include/access/gist_private.h      |  7 ++--
 src/include/access/gistxlog.h          |  1 -
 7 files changed, 56 insertions(+), 59 deletions(-)

diff --git a/src/backend/access/gist/gist.c b/src/backend/access/gist/gist.c
index 2fddb23496..0e2b6c3014 100644
--- a/src/backend/access/gist/gist.c
+++ b/src/backend/access/gist/gist.c
@@ -172,7 +172,7 @@ gistinsert(Relation r, Datum *values, bool *isnull,
 						 values, isnull, true /* size is currently bogus */ );
 	itup->t_tid = *ht_ctid;
 
-	gistdoinsert(r, itup, 0, giststate, heapRel);
+	gistdoinsert(r, itup, 0, giststate, heapRel, false);
 
 	/* cleanup */
 	MemoryContextSwitchTo(oldCxt);
@@ -219,7 +219,8 @@ gistplacetopage(Relation rel, Size freespace, GISTSTATE *giststate,
 				Buffer leftchildbuf,
 				List **splitinfo,
 				bool markfollowright,
-				Relation heapRel)
+				Relation heapRel,
+				bool is_build)
 {
 	BlockNumber blkno = BufferGetBlockNumber(buffer);
 	Page		page = BufferGetPage(buffer);
@@ -458,7 +459,7 @@ gistplacetopage(Relation rel, Size freespace, GISTSTATE *giststate,
 		 * insertion for that. NB: The number of pages and data segments
 		 * specified here must match the calculations in gistXLogSplit()!
 		 */
-		if (RelationNeedsWAL(rel))
+		if (RelationNeedsWAL(rel) && !is_build)
 			XLogEnsureRecordSpace(npage, 1 + npage * 2);
 
 		START_CRIT_SECTION();
@@ -479,18 +480,20 @@ gistplacetopage(Relation rel, Size freespace, GISTSTATE *giststate,
 		PageRestoreTempPage(dist->page, BufferGetPage(dist->buffer));
 		dist->page = BufferGetPage(dist->buffer);
 
-		/* Write the WAL record */
-		if (RelationNeedsWAL(rel))
+		/*
+		 * Write the WAL record.
+		 * Do not write XLog entry if the insertion is caused by
+		 * index build process.
+		 */
+		if (RelationNeedsWAL(rel) && !is_build)
 			recptr = gistXLogSplit(is_leaf,
-								   dist, oldrlink, oldnsn, leftchildbuf,
-								   markfollowright);
+								dist, oldrlink, oldnsn, leftchildbuf,
+								markfollowright);
 		else
 			recptr = gistGetFakeLSN(rel);
 
 		for (ptr = dist; ptr; ptr = ptr->next)
-		{
 			PageSetLSN(ptr->page, recptr);
-		}
 
 		/*
 		 * Return the new child buffers to the caller.
@@ -544,7 +547,8 @@ gistplacetopage(Relation rel, Size freespace, GISTSTATE *giststate,
 		if (BufferIsValid(leftchildbuf))
 			MarkBufferDirty(leftchildbuf);
 
-		if (RelationNeedsWAL(rel))
+
+		if (RelationNeedsWAL(rel) && !is_build)
 		{
 			OffsetNumber ndeloffs = 0,
 						deloffs[1];
@@ -567,6 +571,7 @@ gistplacetopage(Relation rel, Size freespace, GISTSTATE *giststate,
 			PageSetLSN(page, recptr);
 		}
 
+
 		if (newblkno)
 			*newblkno = blkno;
 	}
@@ -583,17 +588,28 @@ gistplacetopage(Relation rel, Size freespace, GISTSTATE *giststate,
 	 * the full page image. There's a chicken-and-egg problem: if we updated
 	 * the child pages first, we wouldn't know the recptr of the WAL record
 	 * we're about to write.
+	 *
+	 * We use fakeLSNs for inserions caused by index build. And when it is
+	 * finished, we write generic_xlog entry for each index page and update
+	 * all LSNs. In order to keep NSNs less then LSNs after this update, we
+	 * set NSN to InvalidXLogRecPtr, which is the smallest possible NSN.
 	 */
+
 	if (BufferIsValid(leftchildbuf))
 	{
 		Page		leftpg = BufferGetPage(leftchildbuf);
+		XLogRecPtr	fakerecptr = InvalidXLogRecPtr;
 
-		GistPageSetNSN(leftpg, recptr);
-		GistClearFollowRight(leftpg);
+		if (!is_build)
+			GistPageSetNSN(leftpg, recptr);
+		else
+			GistPageSetNSN(leftpg, fakerecptr);
 
+		GistClearFollowRight(leftpg);
 		PageSetLSN(leftpg, recptr);
 	}
 
+
 	END_CRIT_SECTION();
 
 	return is_split;
@@ -606,7 +622,7 @@ gistplacetopage(Relation rel, Size freespace, GISTSTATE *giststate,
  */
 void
 gistdoinsert(Relation r, IndexTuple itup, Size freespace,
-			 GISTSTATE *giststate, Relation heapRel)
+			 GISTSTATE *giststate, Relation heapRel, bool is_build)
 {
 	ItemId		iid;
 	IndexTuple	idxtuple;
@@ -619,6 +635,7 @@ gistdoinsert(Relation r, IndexTuple itup, Size freespace,
 	state.freespace = freespace;
 	state.r = r;
 	state.heapRel = heapRel;
+	state.is_build = is_build;
 
 	/* Start from the root */
 	firststack.blkno = GIST_ROOT_BLKNO;
@@ -1251,7 +1268,8 @@ gistinserttuples(GISTInsertState *state, GISTInsertStack *stack,
 							   leftchild,
 							   &splitinfo,
 							   true,
-							   state->heapRel);
+							   state->heapRel,
+							   state->is_build);
 
 	/*
 	 * Before recursing up in case the page was split, release locks on the
diff --git a/src/backend/access/gist/gistbuild.c b/src/backend/access/gist/gistbuild.c
index 3652fde5bb..8d0d285cab 100644
--- a/src/backend/access/gist/gistbuild.c
+++ b/src/backend/access/gist/gistbuild.c
@@ -17,6 +17,7 @@
 #include <math.h>
 
 #include "access/genam.h"
+#include "access/generic_xlog.h"
 #include "access/gist_private.h"
 #include "access/gistxlog.h"
 #include "access/tableam.h"
@@ -181,18 +182,12 @@ gistbuild(Relation heap, Relation index, IndexInfo *indexInfo)
 
 	MarkBufferDirty(buffer);
 
-	if (RelationNeedsWAL(index))
-	{
-		XLogRecPtr	recptr;
-
-		XLogBeginInsert();
-		XLogRegisterBuffer(0, buffer, REGBUF_WILL_INIT);
-
-		recptr = XLogInsert(RM_GIST_ID, XLOG_GIST_CREATE_INDEX);
-		PageSetLSN(page, recptr);
-	}
-	else
-		PageSetLSN(page, gistGetFakeLSN(heap));
+	/*
+	 * Do not write index pages to WAL unitl index build is finished.
+	 * But we still need increasing LSNs on each page, so use FakeLSN,
+	 * even for relations which eventually need WAL.
+	 */
+	PageSetLSN(page, gistGetFakeLSN(heap));
 
 	UnlockReleaseBuffer(buffer);
 
@@ -226,6 +221,15 @@ gistbuild(Relation heap, Relation index, IndexInfo *indexInfo)
 
 	freeGISTstate(buildstate.giststate);
 
+	/*
+	 * Create generic wal records for all pages of relation, if necessary.
+	 * It seems reasonable not to generate WAL, if we recieved interrupt
+	 * signal.
+	 */
+	CHECK_FOR_INTERRUPTS();
+	if (RelationNeedsWAL(index))
+		generic_log_relation(index);
+
 	/*
 	 * Return statistics
 	 */
@@ -488,7 +492,7 @@ gistBuildCallback(Relation index,
 		 * locked, we call gistdoinsert directly.
 		 */
 		gistdoinsert(index, itup, buildstate->freespace,
-					 buildstate->giststate, buildstate->heaprel);
+					 buildstate->giststate, buildstate->heaprel, true);
 	}
 
 	/* Update tuple count and total size. */
@@ -695,7 +699,7 @@ gistbufferinginserttuples(GISTBuildState *buildstate, Buffer buffer, int level,
 							   InvalidBuffer,
 							   &splitinfo,
 							   false,
-							   buildstate->heaprel);
+							   buildstate->heaprel, true);
 
 	/*
 	 * If this is a root split, update the root path item kept in memory. This
diff --git a/src/backend/access/gist/gistutil.c b/src/backend/access/gist/gistutil.c
index 2163cc482d..af278e5ded 100644
--- a/src/backend/access/gist/gistutil.c
+++ b/src/backend/access/gist/gistutil.c
@@ -1004,6 +1004,7 @@ gistproperty(Oid index_oid, int attno,
  * Temporary and unlogged GiST indexes are not WAL-logged, but we need LSNs
  * to detect concurrent page splits anyway. This function provides a fake
  * sequence of LSNs for that purpose.
+ * Persistent relations are also not WAL-logged while we build index.
  */
 XLogRecPtr
 gistGetFakeLSN(Relation rel)
@@ -1024,7 +1025,6 @@ gistGetFakeLSN(Relation rel)
 		 * Unlogged relations are accessible from other backends, and survive
 		 * (clean) restarts. GetFakeLSNForUnloggedRel() handles that for us.
 		 */
-		Assert(rel->rd_rel->relpersistence == RELPERSISTENCE_UNLOGGED);
 		return GetFakeLSNForUnloggedRel();
 	}
 }
diff --git a/src/backend/access/gist/gistxlog.c b/src/backend/access/gist/gistxlog.c
index cb80ab00cd..4fb1855e89 100644
--- a/src/backend/access/gist/gistxlog.c
+++ b/src/backend/access/gist/gistxlog.c
@@ -490,25 +490,6 @@ gistRedoPageSplitRecord(XLogReaderState *record)
 	UnlockReleaseBuffer(firstbuffer);
 }
 
-static void
-gistRedoCreateIndex(XLogReaderState *record)
-{
-	XLogRecPtr	lsn = record->EndRecPtr;
-	Buffer		buffer;
-	Page		page;
-
-	buffer = XLogInitBufferForRedo(record, 0);
-	Assert(BufferGetBlockNumber(buffer) == GIST_ROOT_BLKNO);
-	page = (Page) BufferGetPage(buffer);
-
-	GISTInitBuffer(buffer, F_LEAF);
-
-	PageSetLSN(page, lsn);
-
-	MarkBufferDirty(buffer);
-	UnlockReleaseBuffer(buffer);
-}
-
 /* redo page deletion */
 static void
 gistRedoPageDelete(XLogReaderState *record)
@@ -594,9 +575,6 @@ gist_redo(XLogReaderState *record)
 		case XLOG_GIST_PAGE_SPLIT:
 			gistRedoPageSplitRecord(record);
 			break;
-		case XLOG_GIST_CREATE_INDEX:
-			gistRedoCreateIndex(record);
-			break;
 		case XLOG_GIST_PAGE_DELETE:
 			gistRedoPageDelete(record);
 			break;
diff --git a/src/backend/access/rmgrdesc/gistdesc.c b/src/backend/access/rmgrdesc/gistdesc.c
index 3ff4f83d38..eb308c72d6 100644
--- a/src/backend/access/rmgrdesc/gistdesc.c
+++ b/src/backend/access/rmgrdesc/gistdesc.c
@@ -71,8 +71,6 @@ gist_desc(StringInfo buf, XLogReaderState *record)
 		case XLOG_GIST_PAGE_SPLIT:
 			out_gistxlogPageSplit(buf, (gistxlogPageSplit *) rec);
 			break;
-		case XLOG_GIST_CREATE_INDEX:
-			break;
 		case XLOG_GIST_PAGE_DELETE:
 			out_gistxlogPageDelete(buf, (gistxlogPageDelete *) rec);
 			break;
@@ -98,9 +96,6 @@ gist_identify(uint8 info)
 		case XLOG_GIST_PAGE_SPLIT:
 			id = "PAGE_SPLIT";
 			break;
-		case XLOG_GIST_CREATE_INDEX:
-			id = "CREATE_INDEX";
-			break;
 		case XLOG_GIST_PAGE_DELETE:
 			id = "PAGE_DELETE";
 			break;
diff --git a/src/include/access/gist_private.h b/src/include/access/gist_private.h
index 02dc285a78..78e2e3fb31 100644
--- a/src/include/access/gist_private.h
+++ b/src/include/access/gist_private.h
@@ -244,6 +244,7 @@ typedef struct
 	Relation	r;
 	Relation	heapRel;
 	Size		freespace;		/* free space to be left */
+	bool		is_build;
 
 	GISTInsertStack *stack;
 } GISTInsertState;
@@ -393,7 +394,8 @@ extern void gistdoinsert(Relation r,
 			 IndexTuple itup,
 			 Size freespace,
 			 GISTSTATE *GISTstate,
-			 Relation heapRel);
+			 Relation heapRel,
+			 bool is_build);
 
 /* A List of these is returned from gistplacetopage() in *splitinfo */
 typedef struct
@@ -409,7 +411,8 @@ extern bool gistplacetopage(Relation rel, Size freespace, GISTSTATE *giststate,
 				Buffer leftchildbuf,
 				List **splitinfo,
 				bool markleftchild,
-				Relation heapRel);
+				Relation heapRel,
+				bool is_build);
 
 extern SplitedPageLayout *gistSplit(Relation r, Page page, IndexTuple *itup,
 		  int len, GISTSTATE *giststate);
diff --git a/src/include/access/gistxlog.h b/src/include/access/gistxlog.h
index 2f87b67a53..80931497ca 100644
--- a/src/include/access/gistxlog.h
+++ b/src/include/access/gistxlog.h
@@ -23,7 +23,6 @@
 										  * FSM */
 #define XLOG_GIST_PAGE_SPLIT		0x30
  /* #define XLOG_GIST_INSERT_COMPLETE	 0x40 */	/* not used anymore */
-#define XLOG_GIST_CREATE_INDEX		0x50
 #define XLOG_GIST_PAGE_DELETE		0x60
 
 /*
-- 
2.17.1


--------------5B85550EC82B346D9FBFC34E
Content-Type: text/x-patch;
 name="v2_0004-SP-GiST-Optimal-WAL-Usage.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename="v2_0004-SP-GiST-Optimal-WAL-Usage.patch"



^ permalink  raw  reply  [nested|flat] 16+ messages in thread

* [PATCH 3/4] GiST-Optimal-WAL-Usage
@ 2019-04-02 04:43  Andrey V. Lepikhov <[email protected]>
  0 siblings, 0 replies; 16+ messages in thread

From: Andrey V. Lepikhov @ 2019-04-02 04:43 UTC (permalink / raw)

---
 src/backend/access/gist/gist.c         | 46 ++++++++++++++++++--------
 src/backend/access/gist/gistbuild.c    | 32 ++++++++++--------
 src/backend/access/gist/gistutil.c     |  2 +-
 src/backend/access/gist/gistxlog.c     | 22 ------------
 src/backend/access/rmgrdesc/gistdesc.c |  5 ---
 src/include/access/gist_private.h      |  7 ++--
 src/include/access/gistxlog.h          |  1 -
 7 files changed, 56 insertions(+), 59 deletions(-)

diff --git a/src/backend/access/gist/gist.c b/src/backend/access/gist/gist.c
index 2fddb23496..0e2b6c3014 100644
--- a/src/backend/access/gist/gist.c
+++ b/src/backend/access/gist/gist.c
@@ -172,7 +172,7 @@ gistinsert(Relation r, Datum *values, bool *isnull,
 						 values, isnull, true /* size is currently bogus */ );
 	itup->t_tid = *ht_ctid;
 
-	gistdoinsert(r, itup, 0, giststate, heapRel);
+	gistdoinsert(r, itup, 0, giststate, heapRel, false);
 
 	/* cleanup */
 	MemoryContextSwitchTo(oldCxt);
@@ -219,7 +219,8 @@ gistplacetopage(Relation rel, Size freespace, GISTSTATE *giststate,
 				Buffer leftchildbuf,
 				List **splitinfo,
 				bool markfollowright,
-				Relation heapRel)
+				Relation heapRel,
+				bool is_build)
 {
 	BlockNumber blkno = BufferGetBlockNumber(buffer);
 	Page		page = BufferGetPage(buffer);
@@ -458,7 +459,7 @@ gistplacetopage(Relation rel, Size freespace, GISTSTATE *giststate,
 		 * insertion for that. NB: The number of pages and data segments
 		 * specified here must match the calculations in gistXLogSplit()!
 		 */
-		if (RelationNeedsWAL(rel))
+		if (RelationNeedsWAL(rel) && !is_build)
 			XLogEnsureRecordSpace(npage, 1 + npage * 2);
 
 		START_CRIT_SECTION();
@@ -479,18 +480,20 @@ gistplacetopage(Relation rel, Size freespace, GISTSTATE *giststate,
 		PageRestoreTempPage(dist->page, BufferGetPage(dist->buffer));
 		dist->page = BufferGetPage(dist->buffer);
 
-		/* Write the WAL record */
-		if (RelationNeedsWAL(rel))
+		/*
+		 * Write the WAL record.
+		 * Do not write XLog entry if the insertion is caused by
+		 * index build process.
+		 */
+		if (RelationNeedsWAL(rel) && !is_build)
 			recptr = gistXLogSplit(is_leaf,
-								   dist, oldrlink, oldnsn, leftchildbuf,
-								   markfollowright);
+								dist, oldrlink, oldnsn, leftchildbuf,
+								markfollowright);
 		else
 			recptr = gistGetFakeLSN(rel);
 
 		for (ptr = dist; ptr; ptr = ptr->next)
-		{
 			PageSetLSN(ptr->page, recptr);
-		}
 
 		/*
 		 * Return the new child buffers to the caller.
@@ -544,7 +547,8 @@ gistplacetopage(Relation rel, Size freespace, GISTSTATE *giststate,
 		if (BufferIsValid(leftchildbuf))
 			MarkBufferDirty(leftchildbuf);
 
-		if (RelationNeedsWAL(rel))
+
+		if (RelationNeedsWAL(rel) && !is_build)
 		{
 			OffsetNumber ndeloffs = 0,
 						deloffs[1];
@@ -567,6 +571,7 @@ gistplacetopage(Relation rel, Size freespace, GISTSTATE *giststate,
 			PageSetLSN(page, recptr);
 		}
 
+
 		if (newblkno)
 			*newblkno = blkno;
 	}
@@ -583,17 +588,28 @@ gistplacetopage(Relation rel, Size freespace, GISTSTATE *giststate,
 	 * the full page image. There's a chicken-and-egg problem: if we updated
 	 * the child pages first, we wouldn't know the recptr of the WAL record
 	 * we're about to write.
+	 *
+	 * We use fakeLSNs for inserions caused by index build. And when it is
+	 * finished, we write generic_xlog entry for each index page and update
+	 * all LSNs. In order to keep NSNs less then LSNs after this update, we
+	 * set NSN to InvalidXLogRecPtr, which is the smallest possible NSN.
 	 */
+
 	if (BufferIsValid(leftchildbuf))
 	{
 		Page		leftpg = BufferGetPage(leftchildbuf);
+		XLogRecPtr	fakerecptr = InvalidXLogRecPtr;
 
-		GistPageSetNSN(leftpg, recptr);
-		GistClearFollowRight(leftpg);
+		if (!is_build)
+			GistPageSetNSN(leftpg, recptr);
+		else
+			GistPageSetNSN(leftpg, fakerecptr);
 
+		GistClearFollowRight(leftpg);
 		PageSetLSN(leftpg, recptr);
 	}
 
+
 	END_CRIT_SECTION();
 
 	return is_split;
@@ -606,7 +622,7 @@ gistplacetopage(Relation rel, Size freespace, GISTSTATE *giststate,
  */
 void
 gistdoinsert(Relation r, IndexTuple itup, Size freespace,
-			 GISTSTATE *giststate, Relation heapRel)
+			 GISTSTATE *giststate, Relation heapRel, bool is_build)
 {
 	ItemId		iid;
 	IndexTuple	idxtuple;
@@ -619,6 +635,7 @@ gistdoinsert(Relation r, IndexTuple itup, Size freespace,
 	state.freespace = freespace;
 	state.r = r;
 	state.heapRel = heapRel;
+	state.is_build = is_build;
 
 	/* Start from the root */
 	firststack.blkno = GIST_ROOT_BLKNO;
@@ -1251,7 +1268,8 @@ gistinserttuples(GISTInsertState *state, GISTInsertStack *stack,
 							   leftchild,
 							   &splitinfo,
 							   true,
-							   state->heapRel);
+							   state->heapRel,
+							   state->is_build);
 
 	/*
 	 * Before recursing up in case the page was split, release locks on the
diff --git a/src/backend/access/gist/gistbuild.c b/src/backend/access/gist/gistbuild.c
index 3652fde5bb..aa05c0a8ee 100644
--- a/src/backend/access/gist/gistbuild.c
+++ b/src/backend/access/gist/gistbuild.c
@@ -17,6 +17,7 @@
 #include <math.h>
 
 #include "access/genam.h"
+#include "access/generic_xlog.h"
 #include "access/gist_private.h"
 #include "access/gistxlog.h"
 #include "access/tableam.h"
@@ -181,18 +182,12 @@ gistbuild(Relation heap, Relation index, IndexInfo *indexInfo)
 
 	MarkBufferDirty(buffer);
 
-	if (RelationNeedsWAL(index))
-	{
-		XLogRecPtr	recptr;
-
-		XLogBeginInsert();
-		XLogRegisterBuffer(0, buffer, REGBUF_WILL_INIT);
-
-		recptr = XLogInsert(RM_GIST_ID, XLOG_GIST_CREATE_INDEX);
-		PageSetLSN(page, recptr);
-	}
-	else
-		PageSetLSN(page, gistGetFakeLSN(heap));
+	/*
+	 * Do not write index pages to WAL unitl index build is finished.
+	 * But we still need increasing LSNs on each page, so use FakeLSN,
+	 * even for relations which eventually need WAL.
+	 */
+	PageSetLSN(page, gistGetFakeLSN(heap));
 
 	UnlockReleaseBuffer(buffer);
 
@@ -226,6 +221,15 @@ gistbuild(Relation heap, Relation index, IndexInfo *indexInfo)
 
 	freeGISTstate(buildstate.giststate);
 
+	/*
+	 * Create generic wal records for all pages of relation, if necessary.
+	 * It seems reasonable not to generate WAL, if we recieved interrupt
+	 * signal.
+	 */
+	CHECK_FOR_INTERRUPTS();
+	if (RelationNeedsWAL(index))
+		log_relation(index);
+
 	/*
 	 * Return statistics
 	 */
@@ -488,7 +492,7 @@ gistBuildCallback(Relation index,
 		 * locked, we call gistdoinsert directly.
 		 */
 		gistdoinsert(index, itup, buildstate->freespace,
-					 buildstate->giststate, buildstate->heaprel);
+					 buildstate->giststate, buildstate->heaprel, true);
 	}
 
 	/* Update tuple count and total size. */
@@ -695,7 +699,7 @@ gistbufferinginserttuples(GISTBuildState *buildstate, Buffer buffer, int level,
 							   InvalidBuffer,
 							   &splitinfo,
 							   false,
-							   buildstate->heaprel);
+							   buildstate->heaprel, true);
 
 	/*
 	 * If this is a root split, update the root path item kept in memory. This
diff --git a/src/backend/access/gist/gistutil.c b/src/backend/access/gist/gistutil.c
index 2163cc482d..af278e5ded 100644
--- a/src/backend/access/gist/gistutil.c
+++ b/src/backend/access/gist/gistutil.c
@@ -1004,6 +1004,7 @@ gistproperty(Oid index_oid, int attno,
  * Temporary and unlogged GiST indexes are not WAL-logged, but we need LSNs
  * to detect concurrent page splits anyway. This function provides a fake
  * sequence of LSNs for that purpose.
+ * Persistent relations are also not WAL-logged while we build index.
  */
 XLogRecPtr
 gistGetFakeLSN(Relation rel)
@@ -1024,7 +1025,6 @@ gistGetFakeLSN(Relation rel)
 		 * Unlogged relations are accessible from other backends, and survive
 		 * (clean) restarts. GetFakeLSNForUnloggedRel() handles that for us.
 		 */
-		Assert(rel->rd_rel->relpersistence == RELPERSISTENCE_UNLOGGED);
 		return GetFakeLSNForUnloggedRel();
 	}
 }
diff --git a/src/backend/access/gist/gistxlog.c b/src/backend/access/gist/gistxlog.c
index cb80ab00cd..4fb1855e89 100644
--- a/src/backend/access/gist/gistxlog.c
+++ b/src/backend/access/gist/gistxlog.c
@@ -490,25 +490,6 @@ gistRedoPageSplitRecord(XLogReaderState *record)
 	UnlockReleaseBuffer(firstbuffer);
 }
 
-static void
-gistRedoCreateIndex(XLogReaderState *record)
-{
-	XLogRecPtr	lsn = record->EndRecPtr;
-	Buffer		buffer;
-	Page		page;
-
-	buffer = XLogInitBufferForRedo(record, 0);
-	Assert(BufferGetBlockNumber(buffer) == GIST_ROOT_BLKNO);
-	page = (Page) BufferGetPage(buffer);
-
-	GISTInitBuffer(buffer, F_LEAF);
-
-	PageSetLSN(page, lsn);
-
-	MarkBufferDirty(buffer);
-	UnlockReleaseBuffer(buffer);
-}
-
 /* redo page deletion */
 static void
 gistRedoPageDelete(XLogReaderState *record)
@@ -594,9 +575,6 @@ gist_redo(XLogReaderState *record)
 		case XLOG_GIST_PAGE_SPLIT:
 			gistRedoPageSplitRecord(record);
 			break;
-		case XLOG_GIST_CREATE_INDEX:
-			gistRedoCreateIndex(record);
-			break;
 		case XLOG_GIST_PAGE_DELETE:
 			gistRedoPageDelete(record);
 			break;
diff --git a/src/backend/access/rmgrdesc/gistdesc.c b/src/backend/access/rmgrdesc/gistdesc.c
index 3ff4f83d38..eb308c72d6 100644
--- a/src/backend/access/rmgrdesc/gistdesc.c
+++ b/src/backend/access/rmgrdesc/gistdesc.c
@@ -71,8 +71,6 @@ gist_desc(StringInfo buf, XLogReaderState *record)
 		case XLOG_GIST_PAGE_SPLIT:
 			out_gistxlogPageSplit(buf, (gistxlogPageSplit *) rec);
 			break;
-		case XLOG_GIST_CREATE_INDEX:
-			break;
 		case XLOG_GIST_PAGE_DELETE:
 			out_gistxlogPageDelete(buf, (gistxlogPageDelete *) rec);
 			break;
@@ -98,9 +96,6 @@ gist_identify(uint8 info)
 		case XLOG_GIST_PAGE_SPLIT:
 			id = "PAGE_SPLIT";
 			break;
-		case XLOG_GIST_CREATE_INDEX:
-			id = "CREATE_INDEX";
-			break;
 		case XLOG_GIST_PAGE_DELETE:
 			id = "PAGE_DELETE";
 			break;
diff --git a/src/include/access/gist_private.h b/src/include/access/gist_private.h
index 02dc285a78..78e2e3fb31 100644
--- a/src/include/access/gist_private.h
+++ b/src/include/access/gist_private.h
@@ -244,6 +244,7 @@ typedef struct
 	Relation	r;
 	Relation	heapRel;
 	Size		freespace;		/* free space to be left */
+	bool		is_build;
 
 	GISTInsertStack *stack;
 } GISTInsertState;
@@ -393,7 +394,8 @@ extern void gistdoinsert(Relation r,
 			 IndexTuple itup,
 			 Size freespace,
 			 GISTSTATE *GISTstate,
-			 Relation heapRel);
+			 Relation heapRel,
+			 bool is_build);
 
 /* A List of these is returned from gistplacetopage() in *splitinfo */
 typedef struct
@@ -409,7 +411,8 @@ extern bool gistplacetopage(Relation rel, Size freespace, GISTSTATE *giststate,
 				Buffer leftchildbuf,
 				List **splitinfo,
 				bool markleftchild,
-				Relation heapRel);
+				Relation heapRel,
+				bool is_build);
 
 extern SplitedPageLayout *gistSplit(Relation r, Page page, IndexTuple *itup,
 		  int len, GISTSTATE *giststate);
diff --git a/src/include/access/gistxlog.h b/src/include/access/gistxlog.h
index 2f87b67a53..80931497ca 100644
--- a/src/include/access/gistxlog.h
+++ b/src/include/access/gistxlog.h
@@ -23,7 +23,6 @@
 										  * FSM */
 #define XLOG_GIST_PAGE_SPLIT		0x30
  /* #define XLOG_GIST_INSERT_COMPLETE	 0x40 */	/* not used anymore */
-#define XLOG_GIST_CREATE_INDEX		0x50
 #define XLOG_GIST_PAGE_DELETE		0x60
 
 /*
-- 
2.17.1


--------------58E4AA393B5F48ED79D1D18E
Content-Type: text/x-patch;
 name="v3_0004-SP-GiST-Optimal-WAL-Usage.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename="v3_0004-SP-GiST-Optimal-WAL-Usage.patch"



^ permalink  raw  reply  [nested|flat] 16+ messages in thread

* Re: extension_control_path and "directory"
@ 2025-04-24 15:18  Matheus Alcantara <[email protected]>
  0 siblings, 1 reply; 16+ messages in thread

From: Matheus Alcantara @ 2025-04-24 15:18 UTC (permalink / raw)
  To: Christoph Berg <[email protected]>; +Cc: David E. Wheeler <[email protected]>; Peter Eisentraut <[email protected]>; [email protected]

On Thu, Apr 24, 2025 at 7:21 AM Christoph Berg <[email protected]> wrote:
>
> Re: Matheus Alcantara
> > I've tested with the semver extension and it seems to work fine with
> > this patch. Can you please check on your side to see if it's also
> > working?
>
> Hi Matheus,
>
> thanks for the patch, it does indeed work.
>
Thanks for testing and for reviewing.

> diff --git a/src/backend/commands/extension.c b/src/backend/commands/extension.c
> index 180f4af9be3..d68efd59118 100644
> --- a/src/backend/commands/extension.c
> +++ b/src/backend/commands/extension.c
> @@ -376,6 +376,14 @@ get_extension_control_directories(void)
>
>                         /* Substitute the path macro if needed */
>                         mangled = substitute_path_macro(piece, "$system", system_dir);
> +
> +                       /*
> +                        * Append "extension" suffix in case is a custom extension control
> +                        * path.
> +                        */
> +                       if (strcmp(piece, "$system") != 0)
> +                               mangled = psprintf("%s/extension", mangled);
>
> This would look prettier if it was something like
>
>                         mangled = substitute_path_macro(piece, "$system", system_dir "/extension");
>
> ... but I'm wondering if it wouldn't be saner if the control path
> should be stored without "extension" in that struct. Then opening the
> control file would be path + "extension/" + filename and the extra
> directory would be path + directory, without any on-the-fly stripping
> of trailing components.
>
> The extension_control_path GUC could also be adjusted to refer to the
> directory one level above the extension/foo.control location.
>
Storing the control path directly without any code to remove the
/extension at the end would be more trick I think, because we would need
to change the find_in_path() function to return the path without the
suffix.

In this new version I've introduced a new "basedir" field on
ExtensionControlFile so that we can save the base directory to search
for .control files and scripts. With this new field, on
get_extension_script_directory() we just need to join control->basedir
with control->directory. Note that we still need to handle the removal
of the /extension at the end of control path but I think that on this
new version the code looks a bit better (IMHO) since we just need to
handle on find_extension_control_filename(). WYT?

>
> +       /*
> +        * Assert that the control->control_dir end with /extension suffix so that
> +        * we can replace with the value from control->directory.
> +        */
> +       Assert(ctrldir_len >= suffix_len &&
> +                  strcmp(control->control_dir + ctrldir_len - suffix_len, "extension") == 0);
>
> If control_dir is coming from extension_control_path, it might have a
> different suffix. Replace the Assert by elog(ERROR). (Or see above.)
>
In v2 I've moved the logic to remove the /extension to
parse_extension_control_file(), do you think that this Assert on this
function would still be wrong? IIUC we should always have /extension at
the end of "control_dir" at this place, because the
extension_control_path GUC will omit the /extension at the end and we
will force it to have the suffix on the path at
find_extension_control_filename() and
get_extension_control_directories() functions. I'm missing something
here?

I've also included some more TAP tests on this new version.

-- 
Matheus Alcantara


Attachments:

  [application/octet-stream] v2-0001-Make-directory-work-with-extension-control-path.patch (8.1K, ../../CAFY6G8d-4Nb34dqb_92kGrXfo4tO012m-9YzznKmb7f5JhJpEA@mail.gmail.com/2-v2-0001-Make-directory-work-with-extension-control-path.patch)
  download | inline diff:
From b74590b75b0a08c74625a1731c2544ec1271fa34 Mon Sep 17 00:00:00 2001
From: Matheus Alcantara <[email protected]>
Date: Wed, 23 Apr 2025 16:11:24 -0300
Subject: [PATCH v2] Make "directory" work with extension control path

Previously extensions installed on a custom path that is available via
extension_control_path GUC that set the "directory" field on .control
file was not being able to CREATE. This was happening because on
get_extension_script_directory was hard coded to search for the script
files only on the share system dir.

This commit fix this issue by using the control->control_dir as a share
dir to return the path of the extension script files.
---
 src/backend/commands/extension.c              | 28 ++++--
 .../t/001_extension_control_path.pl           | 88 ++++++++++++++-----
 2 files changed, 84 insertions(+), 32 deletions(-)

diff --git a/src/backend/commands/extension.c b/src/backend/commands/extension.c
index 180f4af9be3..3c85c745d0a 100644
--- a/src/backend/commands/extension.c
+++ b/src/backend/commands/extension.c
@@ -83,6 +83,8 @@ Oid			CurrentExtensionObject = InvalidOid;
 typedef struct ExtensionControlFile
 {
 	char	   *name;			/* name of the extension */
+	char	   *basedir;		/* base directory where control and script
+								 * files are located */
 	char	   *control_dir;	/* directory where control file was found */
 	char	   *directory;		/* directory for script files */
 	char	   *default_version;	/* default install target version, if any */
@@ -376,6 +378,14 @@ get_extension_control_directories(void)
 
 			/* Substitute the path macro if needed */
 			mangled = substitute_path_macro(piece, "$system", system_dir);
+
+			/*
+			 * Append "extension" suffix in case is a custom extension control
+			 * path.
+			 */
+			if (strcmp(piece, "$system") != 0)
+				mangled = psprintf("%s/extension", mangled);
+
 			pfree(piece);
 
 			/* Canonicalize the path based on the OS and add to the list */
@@ -422,6 +432,9 @@ find_extension_control_filename(ExtensionControlFile *control)
 	ecp = Extension_control_path;
 	if (strlen(ecp) == 0)
 		ecp = "$system";
+	else if (strcmp(ecp, "$system") != 0)
+		ecp = psprintf("%s/extension", ecp);
+
 	result = find_in_path(basename, ecp, "extension_control_path", "$system", system_dir);
 
 	if (result)
@@ -439,9 +452,6 @@ find_extension_control_filename(ExtensionControlFile *control)
 static char *
 get_extension_script_directory(ExtensionControlFile *control)
 {
-	char		sharepath[MAXPGPATH];
-	char	   *result;
-
 	/*
 	 * The directory parameter can be omitted, absolute, or relative to the
 	 * installation's share directory.
@@ -452,11 +462,8 @@ get_extension_script_directory(ExtensionControlFile *control)
 	if (is_absolute_path(control->directory))
 		return pstrdup(control->directory);
 
-	get_share_path(my_exec_path, sharepath);
-	result = (char *) palloc(MAXPGPATH);
-	snprintf(result, MAXPGPATH, "%s/%s", sharepath, control->directory);
-
-	return result;
+	Assert(control->basedir != NULL);
+	return psprintf("%s/%s", control->basedir, control->directory);
 }
 
 static char *
@@ -542,6 +549,11 @@ parse_extension_control_file(ExtensionControlFile *control,
 			filename = find_extension_control_filename(control);
 	}
 
+	Assert(control->control_dir != NULL);
+	control->basedir = pnstrdup(
+								control->control_dir,
+								strlen(control->control_dir) - strlen("/extension"));
+
 	if (!filename)
 	{
 		ereport(ERROR,
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 c186c1470f7..827f293fbc9 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
@@ -5,6 +5,7 @@ use warnings FATAL => 'all';
 use PostgreSQL::Test::Cluster;
 use PostgreSQL::Test::Utils;
 use Test::More;
+use File::Path qw( make_path );
 
 my $node = PostgreSQL::Test::Cluster->new('node');
 
@@ -12,25 +13,14 @@ $node->init;
 
 # Create a temporary directory for the extension control file
 my $ext_dir = PostgreSQL::Test::Utils::tempdir();
+make_path("$ext_dir/extension");
+
 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;
+create_extension($ext_name, $ext_dir);
+
+my $ext_name2 = "test_custom_ext_paths_using_directory";
+make_path("$ext_dir/$ext_name2");
+create_extension($ext_name2, $ext_dir, $ext_name2);
 
 # Use the correct separator and escape \ when running on Windows.
 my $sep = $windows_os ? ";" : ":";
@@ -48,6 +38,7 @@ is($ecp, "\$system$sep$ext_dir",
 	"custom extension control directory path configured");
 
 $node->safe_psql('postgres', "CREATE EXTENSION $ext_name");
+$node->safe_psql('postgres', "CREATE EXTENSION $ext_name2");
 
 my $ret = $node->safe_psql('postgres',
 	"select * from pg_available_extensions where name = '$ext_name'");
@@ -55,26 +46,75 @@ 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',
+$ret = $node->safe_psql('postgres',
 	"select * from pg_available_extension_versions where name = '$ext_name'");
-is( $ret2,
+is( $ret,
 	"test_custom_ext_paths|1.0|t|t|f|t|||Test extension_control_path",
 	"extension is installed correctly on pg_available_extension_versions");
 
+$ret = $node->safe_psql('postgres',
+	"select * from pg_available_extensions where name = '$ext_name2'");
+is( $ret,
+	"test_custom_ext_paths_using_directory|1.0|1.0|Test extension_control_path",
+	"extension is installed correctly on pg_available_extensions");
+
+$ret = $node->safe_psql('postgres',
+	"select * from pg_available_extension_versions where name = '$ext_name2'");
+is( $ret,
+	"test_custom_ext_paths_using_directory|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',
+$ret = $node->safe_psql('postgres',
 	"select count(*) > 0 as ok from pg_available_extensions where name = 'plpgsql'"
 );
-is($ret3, "t",
+is($ret, "t",
 	"\$system extension is installed correctly on pg_available_extensions");
 
 
-my $ret4 = $node->safe_psql('postgres',
+$ret = $node->safe_psql('postgres',
 	"set extension_control_path = ''; select count(*) > 0 as ok from pg_available_extensions where name = 'plpgsql'"
 );
-is($ret4, "t",
+is($ret, "t",
 	"\$system extension is installed correctly on pg_available_extensions with empty extension_control_path"
 );
 
+sub create_extension
+{
+	my ($ext_name, $ext_dir, $directory) = @_;
+
+	my $control_file = "$ext_dir/extension/$ext_name.control";
+	my $sql_file;
+
+	if (defined $directory)
+	{
+		$sql_file = "$ext_dir/$directory/$ext_name--1.0.sql";
+	}
+	else
+	{
+		$sql_file = "$ext_dir/extension/$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";
+	if (defined $directory)
+	{
+		print $cf "directory = $directory";
+	}
+	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;
+}
+
 done_testing();
-- 
2.39.5 (Apple Git-154)



^ permalink  raw  reply  [nested|flat] 16+ messages in thread

* Re: extension_control_path and "directory"
@ 2025-04-24 22:27  David E. Wheeler <[email protected]>
  parent: Matheus Alcantara <[email protected]>
  0 siblings, 1 reply; 16+ messages in thread

From: David E. Wheeler @ 2025-04-24 22:27 UTC (permalink / raw)
  To: Matheus Alcantara <[email protected]>; +Cc: Christoph Berg <[email protected]>; Peter Eisentraut <[email protected]>; [email protected]

On Apr 24, 2025, at 11:18, Matheus Alcantara <[email protected]> wrote:

> In v2 I've moved the logic to remove the /extension to
> parse_extension_control_file(), do you think that this Assert on this
> function would still be wrong? IIUC we should always have /extension at
> the end of "control_dir" at this place, because the
> extension_control_path GUC will omit the /extension at the end and we
> will force it to have the suffix on the path at
> find_extension_control_filename() and
> get_extension_control_directories() functions. I'm missing something
> here?

I took this patch for a spin and managed to make it core dump. How? Well I installed semver with this command:

```sh
make prefix=/Users/david/Downloads install
```

Then set the search paths and restarted:

```ini
extension_control_path = '/Users/david/Downloads/share/extension:$system'
dynamic_library_path = '/Users/david/Downloads/lib:$libdir'
```

Then I connected and ran `CREATE EXTENSION semver` and it segfaulted. I poked around for a few minutes and realized that my prefix is not what I expected. Because it doesn’t contain the string “postgres”, PGXS helpfully adds it. The actual paths are:

```ini
extension_control_path = '/Users/david/Downloads/share/postgresql/extension:$system'
dynamic_library_path = '/Users/david/Downloads/lib/postgresql:$libdir'
```

With that fix it no longer segafulted.

So I presume something crashes when a directory or file doesn’t exist.

But I am not at all sure we want this prefix behavior for installing extensions. I get that has been the behavior for setting the main sharedir and libdir for Postgres, but I don’t know that it makes sense for extension prefixes.

Best,

David




Attachments:

  [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
  download

^ permalink  raw  reply  [nested|flat] 16+ messages in thread

* Re: extension_control_path and "directory"
@ 2025-04-25 13:25  Matheus Alcantara <[email protected]>
  parent: David E. Wheeler <[email protected]>
  0 siblings, 1 reply; 16+ messages in thread

From: Matheus Alcantara @ 2025-04-25 13:25 UTC (permalink / raw)
  To: David E. Wheeler <[email protected]>; +Cc: Christoph Berg <[email protected]>; Peter Eisentraut <[email protected]>; [email protected]

On Thu, Apr 24, 2025 at 7:27 PM David E. Wheeler <[email protected]> wrote:
>
> On Apr 24, 2025, at 11:18, Matheus Alcantara <[email protected]> wrote:
>
> > In v2 I've moved the logic to remove the /extension to
> > parse_extension_control_file(), do you think that this Assert on this
> > function would still be wrong? IIUC we should always have /extension at
> > the end of "control_dir" at this place, because the
> > extension_control_path GUC will omit the /extension at the end and we
> > will force it to have the suffix on the path at
> > find_extension_control_filename() and
> > get_extension_control_directories() functions. I'm missing something
> > here?
>
> I took this patch for a spin and managed to make it core dump. How? Well I installed semver with this command:
>
> ```sh
> make prefix=/Users/david/Downloads install
> ```
>
> Then set the search paths and restarted:
>
> ```ini
> extension_control_path = '/Users/david/Downloads/share/extension:$system'
> dynamic_library_path = '/Users/david/Downloads/lib:$libdir'
> ```
>
> Then I connected and ran `CREATE EXTENSION semver` and it segfaulted. I poked around for a few minutes and realized that my prefix is not what I expected. Because it doesn’t contain the string “postgres”, PGXS helpfully adds it. The actual paths are:
>
> ```ini
> extension_control_path = '/Users/david/Downloads/share/postgresql/extension:$system'
> dynamic_library_path = '/Users/david/Downloads/lib/postgresql:$libdir'
> ```
>
> With that fix it no longer segafulted.
>
> So I presume something crashes when a directory or file doesn’t exist.
>

Yes, you are right. The problem was where I was asserting
control->control_dir != NULL. I've moved the assert after the "if
(!filename)" check that returns an error if the extension was not found.

Attached v3 with this fix and also a TAP test for this scenario.

I'm just a bit confused how you get it working using /extension at the
end of extension_control_path since with this patch this suffix is not
necessary and since we hard coded append this it should return an error
when trying to search on something like
/Users/david/Downloads/share/postgresql/extension/extension

-- 
Matheus Alcantara


Attachments:

  [application/octet-stream] v3-0001-Make-directory-work-with-extension-control-path.patch (8.7K, ../../CAFY6G8fjaVpkZHFk3CQR=ShiysZ3Y_ti568-vuKYOrMepLB_sQ@mail.gmail.com/2-v3-0001-Make-directory-work-with-extension-control-path.patch)
  download | inline diff:
From 0a19076958b684fd98d65fb97f4b228d2ddd8b2c Mon Sep 17 00:00:00 2001
From: Matheus Alcantara <[email protected]>
Date: Wed, 23 Apr 2025 16:11:24 -0300
Subject: [PATCH v3] Make "directory" work with extension control path

Previously extensions installed on a custom path that is available via
extension_control_path GUC that set the "directory" field on .control
file was not being able to CREATE. This was happening because on
get_extension_script_directory was hard coded to search for the script
files only on the share system dir.

This commit fix this issue by using the control->control_dir as a share
dir to return the path of the extension script files.
---
 src/backend/commands/extension.c              | 31 +++++--
 .../t/001_extension_control_path.pl           | 93 ++++++++++++++-----
 2 files changed, 92 insertions(+), 32 deletions(-)

diff --git a/src/backend/commands/extension.c b/src/backend/commands/extension.c
index 180f4af9be3..ce53ed85401 100644
--- a/src/backend/commands/extension.c
+++ b/src/backend/commands/extension.c
@@ -83,6 +83,8 @@ Oid			CurrentExtensionObject = InvalidOid;
 typedef struct ExtensionControlFile
 {
 	char	   *name;			/* name of the extension */
+	char	   *basedir;		/* base directory where control and script
+								 * files are located */
 	char	   *control_dir;	/* directory where control file was found */
 	char	   *directory;		/* directory for script files */
 	char	   *default_version;	/* default install target version, if any */
@@ -376,6 +378,14 @@ get_extension_control_directories(void)
 
 			/* Substitute the path macro if needed */
 			mangled = substitute_path_macro(piece, "$system", system_dir);
+
+			/*
+			 * Append "extension" suffix in case is a custom extension control
+			 * path.
+			 */
+			if (strcmp(piece, "$system") != 0)
+				mangled = psprintf("%s/extension", mangled);
+
 			pfree(piece);
 
 			/* Canonicalize the path based on the OS and add to the list */
@@ -422,6 +432,9 @@ find_extension_control_filename(ExtensionControlFile *control)
 	ecp = Extension_control_path;
 	if (strlen(ecp) == 0)
 		ecp = "$system";
+	else if (strcmp(ecp, "$system") != 0)
+		ecp = psprintf("%s/extension", ecp);
+
 	result = find_in_path(basename, ecp, "extension_control_path", "$system", system_dir);
 
 	if (result)
@@ -439,9 +452,6 @@ find_extension_control_filename(ExtensionControlFile *control)
 static char *
 get_extension_script_directory(ExtensionControlFile *control)
 {
-	char		sharepath[MAXPGPATH];
-	char	   *result;
-
 	/*
 	 * The directory parameter can be omitted, absolute, or relative to the
 	 * installation's share directory.
@@ -452,11 +462,8 @@ get_extension_script_directory(ExtensionControlFile *control)
 	if (is_absolute_path(control->directory))
 		return pstrdup(control->directory);
 
-	get_share_path(my_exec_path, sharepath);
-	result = (char *) palloc(MAXPGPATH);
-	snprintf(result, MAXPGPATH, "%s/%s", sharepath, control->directory);
-
-	return result;
+	Assert(control->basedir != NULL);
+	return psprintf("%s/%s", control->basedir, control->directory);
 }
 
 static char *
@@ -550,6 +557,14 @@ parse_extension_control_file(ExtensionControlFile *control,
 				 errhint("The extension must first be installed on the system where PostgreSQL is running.")));
 	}
 
+	/* Assert that the control_dir ends with /extension */
+	Assert(control->control_dir != NULL);
+	Assert(strcmp(control->control_dir + strlen(control->control_dir) - strlen("/extension"), "/extension") == 0);
+
+	control->basedir = pnstrdup(
+								control->control_dir,
+								strlen(control->control_dir) - strlen("/extension"));
+
 	if ((file = AllocateFile(filename, "r")) == NULL)
 	{
 		/* no complaint for missing auxiliary file */
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 c186c1470f7..1ef79d7574f 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
@@ -5,6 +5,7 @@ use warnings FATAL => 'all';
 use PostgreSQL::Test::Cluster;
 use PostgreSQL::Test::Utils;
 use Test::More;
+use File::Path qw( make_path );
 
 my $node = PostgreSQL::Test::Cluster->new('node');
 
@@ -12,25 +13,14 @@ $node->init;
 
 # Create a temporary directory for the extension control file
 my $ext_dir = PostgreSQL::Test::Utils::tempdir();
+make_path("$ext_dir/extension");
+
 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;
+create_extension($ext_name, $ext_dir);
+
+my $ext_name2 = "test_custom_ext_paths_using_directory";
+make_path("$ext_dir/$ext_name2");
+create_extension($ext_name2, $ext_dir, $ext_name2);
 
 # Use the correct separator and escape \ when running on Windows.
 my $sep = $windows_os ? ";" : ":";
@@ -48,6 +38,7 @@ is($ecp, "\$system$sep$ext_dir",
 	"custom extension control directory path configured");
 
 $node->safe_psql('postgres', "CREATE EXTENSION $ext_name");
+$node->safe_psql('postgres', "CREATE EXTENSION $ext_name2");
 
 my $ret = $node->safe_psql('postgres',
 	"select * from pg_available_extensions where name = '$ext_name'");
@@ -55,26 +46,80 @@ 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',
+$ret = $node->safe_psql('postgres',
 	"select * from pg_available_extension_versions where name = '$ext_name'");
-is( $ret2,
+is( $ret,
 	"test_custom_ext_paths|1.0|t|t|f|t|||Test extension_control_path",
 	"extension is installed correctly on pg_available_extension_versions");
 
+$ret = $node->safe_psql('postgres',
+	"select * from pg_available_extensions where name = '$ext_name2'");
+is( $ret,
+	"test_custom_ext_paths_using_directory|1.0|1.0|Test extension_control_path",
+	"extension is installed correctly on pg_available_extensions");
+
+$ret = $node->safe_psql('postgres',
+	"select * from pg_available_extension_versions where name = '$ext_name2'");
+is( $ret,
+	"test_custom_ext_paths_using_directory|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',
+$ret = $node->safe_psql('postgres',
 	"select count(*) > 0 as ok from pg_available_extensions where name = 'plpgsql'"
 );
-is($ret3, "t",
+is($ret, "t",
 	"\$system extension is installed correctly on pg_available_extensions");
 
 
-my $ret4 = $node->safe_psql('postgres',
+$ret = $node->safe_psql('postgres',
 	"set extension_control_path = ''; select count(*) > 0 as ok from pg_available_extensions where name = 'plpgsql'"
 );
-is($ret4, "t",
+is($ret, "t",
 	"\$system extension is installed correctly on pg_available_extensions with empty extension_control_path"
 );
 
+# Test with an extension that does not exists
+my ($code, $stdout, $stderr) =  $node->psql('postgres', "CREATE EXTENSION invalid");
+is($code, 3, 'error to create an extension that does not exists');
+like($stderr, qr/ERROR:  extension "invalid" is not available/);
+
+sub create_extension
+{
+	my ($ext_name, $ext_dir, $directory) = @_;
+
+	my $control_file = "$ext_dir/extension/$ext_name.control";
+	my $sql_file;
+
+	if (defined $directory)
+	{
+		$sql_file = "$ext_dir/$directory/$ext_name--1.0.sql";
+	}
+	else
+	{
+		$sql_file = "$ext_dir/extension/$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";
+	if (defined $directory)
+	{
+		print $cf "directory = $directory";
+	}
+	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;
+}
+
 done_testing();
-- 
2.39.5 (Apple Git-154)



^ permalink  raw  reply  [nested|flat] 16+ messages in thread

* Re: extension_control_path and "directory"
@ 2025-04-25 19:13  David E. Wheeler <[email protected]>
  parent: Matheus Alcantara <[email protected]>
  0 siblings, 1 reply; 16+ messages in thread

From: David E. Wheeler @ 2025-04-25 19:13 UTC (permalink / raw)
  To: Matheus Alcantara <[email protected]>; +Cc: Christoph Berg <[email protected]>; Peter Eisentraut <[email protected]>; [email protected]

On Apr 25, 2025, at 09:25, Matheus Alcantara <[email protected]> wrote:

> Yes, you are right. The problem was where I was asserting
> control->control_dir != NULL. I've moved the assert after the "if
> (!filename)" check that returns an error if the extension was not found.
> 
> Attached v3 with this fix and also a TAP test for this scenario.

That fixes the segfault, thank you.

> I'm just a bit confused how you get it working using /extension at the
> end of extension_control_path since with this patch this suffix is not
> necessary and since we hard coded append this it should return an error
> when trying to search on something like

It worked with

extension_control_path = '/Users/david/Downloads/share/postgresql/extension:$system’

But not with

extension_control_path = '/Users/david/Downloads/share/postgresql:$system’

And here is where the control file actually is:

❯ ll ~/Downloads/share/postgresql/extension  total 8
-rw-r--r--  1 david  staff   161B Apr 24 18:07 semver.control

So I don’t know the answer to your question, but it’d be handy to have functions that return a list of resolved paths from extension_control_path and dynamic_library_path, since they get mangled.

Best,

David



Attachments:

  [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
  download

^ permalink  raw  reply  [nested|flat] 16+ messages in thread

* Re: extension_control_path and "directory"
@ 2025-04-25 21:18  Matheus Alcantara <[email protected]>
  parent: David E. Wheeler <[email protected]>
  0 siblings, 1 reply; 16+ messages in thread

From: Matheus Alcantara @ 2025-04-25 21:18 UTC (permalink / raw)
  To: David E. Wheeler <[email protected]>; +Cc: Christoph Berg <[email protected]>; Peter Eisentraut <[email protected]>; [email protected]

On Fri, Apr 25, 2025 at 4:13 PM David E. Wheeler <[email protected]> wrote:
>
> On Apr 25, 2025, at 09:25, Matheus Alcantara <[email protected]> wrote:
>
> > Yes, you are right. The problem was where I was asserting
> > control->control_dir != NULL. I've moved the assert after the "if
> > (!filename)" check that returns an error if the extension was not found.
> >
> > Attached v3 with this fix and also a TAP test for this scenario.
>
> That fixes the segfault, thank you.
>
Great, thanks for testing!

> > I'm just a bit confused how you get it working using /extension at the
> > end of extension_control_path since with this patch this suffix is not
> > necessary and since we hard coded append this it should return an error
> > when trying to search on something like
>
> It worked with
>
> extension_control_path = '/Users/david/Downloads/share/postgresql/extension:$system’
>
> But not with
>
> extension_control_path = '/Users/david/Downloads/share/postgresql:$system’
>
> And here is where the control file actually is:
>
> ❯ ll ~/Downloads/share/postgresql/extension  total 8
> -rw-r--r--  1 david  staff   161B Apr 24 18:07 semver.control
>
> So I don’t know the answer to your question, but it’d be handy to have functions that return a list of resolved paths from extension_control_path and dynamic_library_path, since they get mangled.
>
Ok, I was testing using extension_control_path = '$system:/my/custom/path'
(starting with the macro) and it was working as expected, testing with
the macro at the end does not work.

The problem was on find_extension_control_filename() that was appending
the /extension at the end of the entire extension_control_path GUC value
instead of just the custom paths.

To append the /extension at each path on extension_control_path would
require some changes on find_in_path() that
find_extension_control_filename() calls, which I think that it would
make the function more complicated. I've them created a similar
find_in_paths() function that works in the same way but it receives a
List of paths instead of the string of paths separated by ":". We can
get this List of paths using get_extension_control_directories() that
also handle the macro substitution like find_in_path().

Attached v4 with these fixes. I hope that now you should be able to omit
the /extension from the GUC value.

-- 
Matheus Alcantara


Attachments:

  [application/octet-stream] v4-0001-Make-directory-work-with-extension-control-path.patch (11.0K, ../../CAFY6G8dwnxqBDcQbVYfjJJ6RYMTxc04gip64Nx7X2fk4ggA+rQ@mail.gmail.com/2-v4-0001-Make-directory-work-with-extension-control-path.patch)
  download | inline diff:
From 6eb655fbe59083a60d3dfdb3804eeaeaa171ad2c Mon Sep 17 00:00:00 2001
From: Matheus Alcantara <[email protected]>
Date: Wed, 23 Apr 2025 16:11:24 -0300
Subject: [PATCH v4] Make "directory" work with extension control path

Previously extensions installed on a custom path that is available via
extension_control_path GUC that set the "directory" field on .control
file was not being able to CREATE. This was happening because on
get_extension_script_directory was hard coded to search for the script
files only on the share system dir.

This commit fix this issue by using the control->control_dir as a share
dir to return the path of the extension script files.
---
 src/backend/commands/extension.c              | 99 ++++++++++++++-----
 .../t/001_extension_control_path.pl           | 93 ++++++++++++-----
 2 files changed, 144 insertions(+), 48 deletions(-)

diff --git a/src/backend/commands/extension.c b/src/backend/commands/extension.c
index 180f4af9be3..c2b9874b3e2 100644
--- a/src/backend/commands/extension.c
+++ b/src/backend/commands/extension.c
@@ -83,6 +83,8 @@ Oid			CurrentExtensionObject = InvalidOid;
 typedef struct ExtensionControlFile
 {
 	char	   *name;			/* name of the extension */
+	char	   *basedir;		/* base directory where control and script
+								 * files are located */
 	char	   *control_dir;	/* directory where control file was found */
 	char	   *directory;		/* directory for script files */
 	char	   *default_version;	/* default install target version, if any */
@@ -153,6 +155,7 @@ static void ExecAlterExtensionContentsRecurse(AlterExtensionContentsStmt *stmt,
 static char *read_whole_file(const char *filename, int *length);
 static ExtensionControlFile *new_ExtensionControlFile(const char *extname);
 
+char	   *find_in_paths(const char *basename, List *paths);
 
 /*
  * get_extension_oid - given an extension name, look up the OID
@@ -376,6 +379,14 @@ get_extension_control_directories(void)
 
 			/* Substitute the path macro if needed */
 			mangled = substitute_path_macro(piece, "$system", system_dir);
+
+			/*
+			 * Append "extension" suffix in case is a custom extension control
+			 * path.
+			 */
+			if (strcmp(piece, "$system") != 0)
+				mangled = psprintf("%s/extension", mangled);
+
 			pfree(piece);
 
 			/* Canonicalize the path based on the OS and add to the list */
@@ -401,28 +412,16 @@ get_extension_control_directories(void)
 static char *
 find_extension_control_filename(ExtensionControlFile *control)
 {
-	char		sharepath[MAXPGPATH];
-	char	   *system_dir;
 	char	   *basename;
-	char	   *ecp;
 	char	   *result;
+	List	   *paths;
 
 	Assert(control->name);
 
-	get_share_path(my_exec_path, sharepath);
-	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);
+	paths = get_extension_control_directories();
+	result = find_in_paths(basename, paths);
 
 	if (result)
 	{
@@ -439,12 +438,11 @@ find_extension_control_filename(ExtensionControlFile *control)
 static char *
 get_extension_script_directory(ExtensionControlFile *control)
 {
-	char		sharepath[MAXPGPATH];
-	char	   *result;
-
 	/*
 	 * The directory parameter can be omitted, absolute, or relative to the
-	 * installation's share directory.
+	 * installation's base directory, which can be the sharedir or a custom
+	 * path that it was set extension_control_path. It depends where the
+	 * .control file was found.
 	 */
 	if (!control->directory)
 		return pstrdup(control->control_dir);
@@ -452,11 +450,8 @@ get_extension_script_directory(ExtensionControlFile *control)
 	if (is_absolute_path(control->directory))
 		return pstrdup(control->directory);
 
-	get_share_path(my_exec_path, sharepath);
-	result = (char *) palloc(MAXPGPATH);
-	snprintf(result, MAXPGPATH, "%s/%s", sharepath, control->directory);
-
-	return result;
+	Assert(control->basedir != NULL);
+	return psprintf("%s/%s", control->basedir, control->directory);
 }
 
 static char *
@@ -550,6 +545,14 @@ parse_extension_control_file(ExtensionControlFile *control,
 				 errhint("The extension must first be installed on the system where PostgreSQL is running.")));
 	}
 
+	/* Assert that the control_dir ends with /extension */
+	Assert(control->control_dir != NULL);
+	Assert(strcmp(control->control_dir + strlen(control->control_dir) - strlen("/extension"), "/extension") == 0);
+
+	control->basedir = pnstrdup(
+								control->control_dir,
+								strlen(control->control_dir) - strlen("/extension"));
+
 	if ((file = AllocateFile(filename, "r")) == NULL)
 	{
 		/* no complaint for missing auxiliary file */
@@ -3863,3 +3866,51 @@ new_ExtensionControlFile(const char *extname)
 
 	return control;
 }
+
+
+/*
+ * Work in a very similar way with find_in_path but it receives an already
+ * parsed List of paths to search the basename and it do not support macro
+ * replacement or custom error messages (for simplicity).
+ *
+ * By "already parsed List of paths" this function expected that paths already
+ * have all macros replaced.
+ */
+char *
+find_in_paths(const char *basename, List *paths)
+{
+	ListCell   *cell;
+
+	/*
+	 * If the paths variable is empty, don't do a path search.
+	 */
+	if (paths == NIL)
+		return NULL;
+
+	foreach(cell, paths)
+	{
+		char	   *path = (char *) lfirst(cell);
+		char	   *full;
+
+		Assert(path != NULL);
+
+		canonicalize_path(path);
+
+		/* only absolute paths */
+		if (!is_absolute_path(path))
+			ereport(ERROR,
+					(errcode(ERRCODE_INVALID_NAME),
+					 errmsg("component in parameter \"extension_control_path\" is not an absolute path")));
+
+		full = psprintf("%s/%s", path, basename);
+
+		elog(DEBUG3, "%s: trying \"%s\"", __func__, full);
+
+		if (pg_file_exists(full))
+			return full;
+
+		pfree(full);
+	}
+
+	return NULL;
+}
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 c186c1470f7..1ef79d7574f 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
@@ -5,6 +5,7 @@ use warnings FATAL => 'all';
 use PostgreSQL::Test::Cluster;
 use PostgreSQL::Test::Utils;
 use Test::More;
+use File::Path qw( make_path );
 
 my $node = PostgreSQL::Test::Cluster->new('node');
 
@@ -12,25 +13,14 @@ $node->init;
 
 # Create a temporary directory for the extension control file
 my $ext_dir = PostgreSQL::Test::Utils::tempdir();
+make_path("$ext_dir/extension");
+
 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;
+create_extension($ext_name, $ext_dir);
+
+my $ext_name2 = "test_custom_ext_paths_using_directory";
+make_path("$ext_dir/$ext_name2");
+create_extension($ext_name2, $ext_dir, $ext_name2);
 
 # Use the correct separator and escape \ when running on Windows.
 my $sep = $windows_os ? ";" : ":";
@@ -48,6 +38,7 @@ is($ecp, "\$system$sep$ext_dir",
 	"custom extension control directory path configured");
 
 $node->safe_psql('postgres', "CREATE EXTENSION $ext_name");
+$node->safe_psql('postgres', "CREATE EXTENSION $ext_name2");
 
 my $ret = $node->safe_psql('postgres',
 	"select * from pg_available_extensions where name = '$ext_name'");
@@ -55,26 +46,80 @@ 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',
+$ret = $node->safe_psql('postgres',
 	"select * from pg_available_extension_versions where name = '$ext_name'");
-is( $ret2,
+is( $ret,
 	"test_custom_ext_paths|1.0|t|t|f|t|||Test extension_control_path",
 	"extension is installed correctly on pg_available_extension_versions");
 
+$ret = $node->safe_psql('postgres',
+	"select * from pg_available_extensions where name = '$ext_name2'");
+is( $ret,
+	"test_custom_ext_paths_using_directory|1.0|1.0|Test extension_control_path",
+	"extension is installed correctly on pg_available_extensions");
+
+$ret = $node->safe_psql('postgres',
+	"select * from pg_available_extension_versions where name = '$ext_name2'");
+is( $ret,
+	"test_custom_ext_paths_using_directory|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',
+$ret = $node->safe_psql('postgres',
 	"select count(*) > 0 as ok from pg_available_extensions where name = 'plpgsql'"
 );
-is($ret3, "t",
+is($ret, "t",
 	"\$system extension is installed correctly on pg_available_extensions");
 
 
-my $ret4 = $node->safe_psql('postgres',
+$ret = $node->safe_psql('postgres',
 	"set extension_control_path = ''; select count(*) > 0 as ok from pg_available_extensions where name = 'plpgsql'"
 );
-is($ret4, "t",
+is($ret, "t",
 	"\$system extension is installed correctly on pg_available_extensions with empty extension_control_path"
 );
 
+# Test with an extension that does not exists
+my ($code, $stdout, $stderr) =  $node->psql('postgres', "CREATE EXTENSION invalid");
+is($code, 3, 'error to create an extension that does not exists');
+like($stderr, qr/ERROR:  extension "invalid" is not available/);
+
+sub create_extension
+{
+	my ($ext_name, $ext_dir, $directory) = @_;
+
+	my $control_file = "$ext_dir/extension/$ext_name.control";
+	my $sql_file;
+
+	if (defined $directory)
+	{
+		$sql_file = "$ext_dir/$directory/$ext_name--1.0.sql";
+	}
+	else
+	{
+		$sql_file = "$ext_dir/extension/$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";
+	if (defined $directory)
+	{
+		print $cf "directory = $directory";
+	}
+	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;
+}
+
 done_testing();
-- 
2.39.5 (Apple Git-154)



^ permalink  raw  reply  [nested|flat] 16+ messages in thread

* Re: extension_control_path and "directory"
@ 2025-04-28 20:49  David E. Wheeler <[email protected]>
  parent: Matheus Alcantara <[email protected]>
  0 siblings, 1 reply; 16+ messages in thread

From: David E. Wheeler @ 2025-04-28 20:49 UTC (permalink / raw)
  To: Matheus Alcantara <[email protected]>; +Cc: Christoph Berg <[email protected]>; Peter Eisentraut <[email protected]>; [email protected]

On Apr 25, 2025, at 17:18, Matheus Alcantara <[email protected]> wrote:

> Ok, I was testing using extension_control_path = '$system:/my/custom/path'
> (starting with the macro) and it was working as expected, testing with
> the macro at the end does not work.

Great example of why it’s useful to do as much testing as possible! That’s an entirely reasonable place to start testing :-)

> The problem was on find_extension_control_filename() that was appending
> the /extension at the end of the entire extension_control_path GUC value
> instead of just the custom paths.

Oh yeah, lol, that wouldn’t work.

> To append the /extension at each path on extension_control_path would
> require some changes on find_in_path() that
> find_extension_control_filename() calls, which I think that it would
> make the function more complicated. I've them created a similar
> find_in_paths() function that works in the same way but it receives a
> List of paths instead of the string of paths separated by ":". We can
> get this List of paths using get_extension_control_directories() that
> also handle the macro substitution like find_in_path().
> 
> Attached v4 with these fixes. I hope that now you should be able to omit
> the /extension from the GUC value.

Yes! It now works with this configuration:

```ini
extension_control_path = '/Users/david/Downloads/share/postgresql:$system'
dynamic_library_path = '/Users/david/Downloads/lib/postgresql:$libdir’
```

Which is nicely more consistent. Kind of want that first one to be called “share_path” now, though, since it’s not just extensions. Although I guess it’s only extension control file searching that uses it (for now).

If I understand this bit correctly:

```c
            /* Substitute the path macro if needed */
mangled = substitute_path_macro(piece, "$system", system_dir);

/*
* Append "extension" suffix in case is a custom extension control
* path.
*/
if (strcmp(piece, "$system") != 0)
mangled = psprintf("%s/extension", mangled);
```

The value of `piece` is a single path from the search path, right? If so, I think it’s either `$system` or something else; I don’t it would ever be that `$system` is a substring of a single path. Is that right?

Other than that, I think this patch is good to go. Thanks!

Best,

David

`

Attachments:

  [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
  download

^ permalink  raw  reply  [nested|flat] 16+ messages in thread

* Re: extension_control_path and "directory"
@ 2025-04-29 13:49  Matheus Alcantara <[email protected]>
  parent: David E. Wheeler <[email protected]>
  0 siblings, 1 reply; 16+ messages in thread

From: Matheus Alcantara @ 2025-04-29 13:49 UTC (permalink / raw)
  To: David E. Wheeler <[email protected]>; +Cc: Christoph Berg <[email protected]>; Peter Eisentraut <[email protected]>; [email protected]

On Mon, Apr 28, 2025 at 5:49 PM David E. Wheeler <[email protected]> wrote:
>
> > To append the /extension at each path on extension_control_path would
> > require some changes on find_in_path() that
> > find_extension_control_filename() calls, which I think that it would
> > make the function more complicated. I've them created a similar
> > find_in_paths() function that works in the same way but it receives a
> > List of paths instead of the string of paths separated by ":". We can
> > get this List of paths using get_extension_control_directories() that
> > also handle the macro substitution like find_in_path().
> >
> > Attached v4 with these fixes. I hope that now you should be able to omit
> > the /extension from the GUC value.
>
> Yes! It now works with this configuration:
>
> ```ini
> extension_control_path = '/Users/david/Downloads/share/postgresql:$system'
> dynamic_library_path = '/Users/david/Downloads/lib/postgresql:$libdir’
> ```
>
> Which is nicely more consistent. Kind of want that first one to be called “share_path” now, though, since it’s not just extensions. Although I guess it’s only extension control file searching that uses it (for now).
>
Thanks for testing!

> If I understand this bit correctly:
>
> ```c
>             /* Substitute the path macro if needed */
> mangled = substitute_path_macro(piece, "$system", system_dir);
>
> /*
> * Append "extension" suffix in case is a custom extension control
> * path.
> */
> if (strcmp(piece, "$system") != 0)
> mangled = psprintf("%s/extension", mangled);
> ```
>
> The value of `piece` is a single path from the search path, right? If so, I think it’s either `$system` or something else; I don’t it would ever be that `$system` is a substring of a single path. Is that right?
>
Yes, it is a single path from the search path, in your case it will be
"/Users/david/Downloads/share/postgresql" and "$system". We split these
paths based on the system path separator and get the next "piece" here:

            char       *piece = first_path_var_separator(ecp);

The first_path_var_separator() changes the "ecp" parameter on every call,
it returns the next path on "ecp" and changes it to have the remaining
paths to iterate over it.


> Other than that, I think this patch is good to go. Thanks!
>
Thanks for reviewing!

-- 
Matheus Alcantara






^ permalink  raw  reply  [nested|flat] 16+ messages in thread

* Re: extension_control_path and "directory"
@ 2025-04-29 14:08  David E. Wheeler <[email protected]>
  parent: Matheus Alcantara <[email protected]>
  0 siblings, 1 reply; 16+ messages in thread

From: David E. Wheeler @ 2025-04-29 14:08 UTC (permalink / raw)
  To: Matheus Alcantara <[email protected]>; +Cc: Christoph Berg <[email protected]>; Peter Eisentraut <[email protected]>; [email protected]

On Apr 29, 2025, at 09:49, Matheus Alcantara <[email protected]> wrote:

> Yes, it is a single path from the search path, in your case it will be
> "/Users/david/Downloads/share/postgresql" and "$system". We split these
> paths based on the system path separator and get the next "piece" here:
> 
>            char       *piece = first_path_var_separator(ecp);
> 
> The first_path_var_separator() changes the "ecp" parameter on every call,
> it returns the next path on "ecp" and changes it to have the remaining
> paths to iterate over it.

Right. My point is a minor one, but I thin you can use an if/ else there:

```c
if (strcmp(piece, "$system") == 0) {
	/* Substitute the path macro if needed */
	mangled = substitute_path_macro(piece, "$system", system_dir);
} else {
	/*
	* Append "extension" suffix in case is a custom extension
	* control path.
	*/
	mangled = psprintf("%s/extension", mangled);
}
```

Best,

David



Attachments:

  [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
  download

^ permalink  raw  reply  [nested|flat] 16+ messages in thread

* Re: extension_control_path and "directory"
@ 2025-04-29 15:06  Matheus Alcantara <[email protected]>
  parent: David E. Wheeler <[email protected]>
  0 siblings, 2 replies; 16+ messages in thread

From: Matheus Alcantara @ 2025-04-29 15:06 UTC (permalink / raw)
  To: David E. Wheeler <[email protected]>; +Cc: Christoph Berg <[email protected]>; Peter Eisentraut <[email protected]>; [email protected]

On Tue, Apr 29, 2025 at 11:08 AM David E. Wheeler <[email protected]> wrote:
> Right. My point is a minor one, but I thin you can use an if/ else there:
>
> ```c
> if (strcmp(piece, "$system") == 0) {
>         /* Substitute the path macro if needed */
>         mangled = substitute_path_macro(piece, "$system", system_dir);
> } else {
>         /*
>         * Append "extension" suffix in case is a custom extension
>         * control path.
>         */
>         mangled = psprintf("%s/extension", mangled);
> }
> ```
>

The substitute_path_macro() already handles the if/else on "piece" but I
think that this if/else version looks nicer. Fixed.

I've also included some documentation changes for this v5 version to
remove the "extension" from the examples and also mention the scenario
when using the "directory" on the .control file.

-- 
Matheus Alcantara


Attachments:

  [application/octet-stream] v5-0001-Make-directory-work-with-extension-control-path.patch (13.2K, ../../CAFY6G8dUXHRii5rNy7V8WmBrmBwp9W7y3g+HL6Tn-Lu8KkvK=A@mail.gmail.com/2-v5-0001-Make-directory-work-with-extension-control-path.patch)
  download | inline diff:
From 2c2bedf39e6d62dea80fe9d9366ccb6c8e0b45da Mon Sep 17 00:00:00 2001
From: Matheus Alcantara <[email protected]>
Date: Wed, 23 Apr 2025 16:11:24 -0300
Subject: [PATCH v5] Make "directory" work with extension control path

Previously extensions installed on a custom path that is available via
extension_control_path GUC that set the "directory" field on .control
file was not being able to CREATE. This was happening because on
get_extension_script_directory was hard coded to search for the script
files only on the share system dir.

This commit fix this issue by using the control->control_dir as a share
dir to return the path of the extension script files.
---
 doc/src/sgml/config.sgml                      |  22 ++--
 src/backend/commands/extension.c              | 102 +++++++++++++-----
 .../t/001_extension_control_path.pl           |  93 +++++++++++-----
 3 files changed, 161 insertions(+), 56 deletions(-)

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 007f1fbe006..6c941f82c92 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -11029,16 +11029,26 @@ dynamic_library_path = 'C:\tools\postgresql;H:\my_project\lib;$libdir'
         (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'
+extension_control_path = '/usr/local/share/postgresql:/home/my_project/share:$system'
 </programlisting>
         or, in a Windows environment:
 <programlisting>
-extension_control_path = 'C:\tools\postgresql\extension;H:\my_project\share\extension;$system'
+extension_control_path = 'C:\tools\postgresql;H:\my_project\share;$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.)
+        Note that all specified path elements are expected to have a
+        <literal>extension</literal> subdirectory which will have the .control
+        and .sql files, this path suffix is automatically appended at the end
+        of each path. (The value for <literal>$system</literal> already
+        includes the <literal>extension</literal> subdirectory.)
+       </para>
+
+       <para>
+       Also note that extension .control file may configure the .sql files to
+       be placed in another directory using the <literal>directory</literal>
+       field (See <xref linkend="extend-extensions-files"/> for details.). If
+       the configured directory is a relative path, it will search based on the
+       path that the .control file was found, for example,
+       <literal>/home/my_project/share/&lt;bla&gt;</literal>.
        </para>
 
        <para>
diff --git a/src/backend/commands/extension.c b/src/backend/commands/extension.c
index 180f4af9be3..577b9f2ff0d 100644
--- a/src/backend/commands/extension.c
+++ b/src/backend/commands/extension.c
@@ -83,6 +83,8 @@ Oid			CurrentExtensionObject = InvalidOid;
 typedef struct ExtensionControlFile
 {
 	char	   *name;			/* name of the extension */
+	char	   *basedir;		/* base directory where control and script
+								 * files are located */
 	char	   *control_dir;	/* directory where control file was found */
 	char	   *directory;		/* directory for script files */
 	char	   *default_version;	/* default install target version, if any */
@@ -153,6 +155,7 @@ static void ExecAlterExtensionContentsRecurse(AlterExtensionContentsStmt *stmt,
 static char *read_whole_file(const char *filename, int *length);
 static ExtensionControlFile *new_ExtensionControlFile(const char *extname);
 
+char	   *find_in_paths(const char *basename, List *paths);
 
 /*
  * get_extension_oid - given an extension name, look up the OID
@@ -374,8 +377,15 @@ get_extension_control_directories(void)
 			piece = palloc(len + 1);
 			strlcpy(piece, ecp, len + 1);
 
-			/* Substitute the path macro if needed */
-			mangled = substitute_path_macro(piece, "$system", system_dir);
+			/*
+			 * Substitute the path macro if needed or append "extension"
+			 * suffix in case is a custom extension control path.
+			 */
+			if (strcmp(piece, "$system") == 0)
+				mangled = substitute_path_macro(piece, "$system", system_dir);
+			else
+				mangled = psprintf("%s/extension", piece);
+
 			pfree(piece);
 
 			/* Canonicalize the path based on the OS and add to the list */
@@ -401,28 +411,16 @@ get_extension_control_directories(void)
 static char *
 find_extension_control_filename(ExtensionControlFile *control)
 {
-	char		sharepath[MAXPGPATH];
-	char	   *system_dir;
 	char	   *basename;
-	char	   *ecp;
 	char	   *result;
+	List	   *paths;
 
 	Assert(control->name);
 
-	get_share_path(my_exec_path, sharepath);
-	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);
+	paths = get_extension_control_directories();
+	result = find_in_paths(basename, paths);
 
 	if (result)
 	{
@@ -439,12 +437,11 @@ find_extension_control_filename(ExtensionControlFile *control)
 static char *
 get_extension_script_directory(ExtensionControlFile *control)
 {
-	char		sharepath[MAXPGPATH];
-	char	   *result;
-
 	/*
 	 * The directory parameter can be omitted, absolute, or relative to the
-	 * installation's share directory.
+	 * installation's base directory, which can be the sharedir or a custom
+	 * path that it was set extension_control_path. It depends where the
+	 * .control file was found.
 	 */
 	if (!control->directory)
 		return pstrdup(control->control_dir);
@@ -452,11 +449,8 @@ get_extension_script_directory(ExtensionControlFile *control)
 	if (is_absolute_path(control->directory))
 		return pstrdup(control->directory);
 
-	get_share_path(my_exec_path, sharepath);
-	result = (char *) palloc(MAXPGPATH);
-	snprintf(result, MAXPGPATH, "%s/%s", sharepath, control->directory);
-
-	return result;
+	Assert(control->basedir != NULL);
+	return psprintf("%s/%s", control->basedir, control->directory);
 }
 
 static char *
@@ -550,6 +544,14 @@ parse_extension_control_file(ExtensionControlFile *control,
 				 errhint("The extension must first be installed on the system where PostgreSQL is running.")));
 	}
 
+	/* Assert that the control_dir ends with /extension */
+	Assert(control->control_dir != NULL);
+	Assert(strcmp(control->control_dir + strlen(control->control_dir) - strlen("/extension"), "/extension") == 0);
+
+	control->basedir = pnstrdup(
+								control->control_dir,
+								strlen(control->control_dir) - strlen("/extension"));
+
 	if ((file = AllocateFile(filename, "r")) == NULL)
 	{
 		/* no complaint for missing auxiliary file */
@@ -3863,3 +3865,51 @@ new_ExtensionControlFile(const char *extname)
 
 	return control;
 }
+
+
+/*
+ * Work in a very similar way with find_in_path but it receives an already
+ * parsed List of paths to search the basename and it do not support macro
+ * replacement or custom error messages (for simplicity).
+ *
+ * By "already parsed List of paths" this function expected that paths already
+ * have all macros replaced.
+ */
+char *
+find_in_paths(const char *basename, List *paths)
+{
+	ListCell   *cell;
+
+	/*
+	 * If the paths variable is empty, don't do a path search.
+	 */
+	if (paths == NIL)
+		return NULL;
+
+	foreach(cell, paths)
+	{
+		char	   *path = (char *) lfirst(cell);
+		char	   *full;
+
+		Assert(path != NULL);
+
+		canonicalize_path(path);
+
+		/* only absolute paths */
+		if (!is_absolute_path(path))
+			ereport(ERROR,
+					(errcode(ERRCODE_INVALID_NAME),
+					 errmsg("component in parameter \"extension_control_path\" is not an absolute path")));
+
+		full = psprintf("%s/%s", path, basename);
+
+		elog(DEBUG3, "%s: trying \"%s\"", __func__, full);
+
+		if (pg_file_exists(full))
+			return full;
+
+		pfree(full);
+	}
+
+	return NULL;
+}
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 c186c1470f7..1ef79d7574f 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
@@ -5,6 +5,7 @@ use warnings FATAL => 'all';
 use PostgreSQL::Test::Cluster;
 use PostgreSQL::Test::Utils;
 use Test::More;
+use File::Path qw( make_path );
 
 my $node = PostgreSQL::Test::Cluster->new('node');
 
@@ -12,25 +13,14 @@ $node->init;
 
 # Create a temporary directory for the extension control file
 my $ext_dir = PostgreSQL::Test::Utils::tempdir();
+make_path("$ext_dir/extension");
+
 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;
+create_extension($ext_name, $ext_dir);
+
+my $ext_name2 = "test_custom_ext_paths_using_directory";
+make_path("$ext_dir/$ext_name2");
+create_extension($ext_name2, $ext_dir, $ext_name2);
 
 # Use the correct separator and escape \ when running on Windows.
 my $sep = $windows_os ? ";" : ":";
@@ -48,6 +38,7 @@ is($ecp, "\$system$sep$ext_dir",
 	"custom extension control directory path configured");
 
 $node->safe_psql('postgres', "CREATE EXTENSION $ext_name");
+$node->safe_psql('postgres', "CREATE EXTENSION $ext_name2");
 
 my $ret = $node->safe_psql('postgres',
 	"select * from pg_available_extensions where name = '$ext_name'");
@@ -55,26 +46,80 @@ 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',
+$ret = $node->safe_psql('postgres',
 	"select * from pg_available_extension_versions where name = '$ext_name'");
-is( $ret2,
+is( $ret,
 	"test_custom_ext_paths|1.0|t|t|f|t|||Test extension_control_path",
 	"extension is installed correctly on pg_available_extension_versions");
 
+$ret = $node->safe_psql('postgres',
+	"select * from pg_available_extensions where name = '$ext_name2'");
+is( $ret,
+	"test_custom_ext_paths_using_directory|1.0|1.0|Test extension_control_path",
+	"extension is installed correctly on pg_available_extensions");
+
+$ret = $node->safe_psql('postgres',
+	"select * from pg_available_extension_versions where name = '$ext_name2'");
+is( $ret,
+	"test_custom_ext_paths_using_directory|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',
+$ret = $node->safe_psql('postgres',
 	"select count(*) > 0 as ok from pg_available_extensions where name = 'plpgsql'"
 );
-is($ret3, "t",
+is($ret, "t",
 	"\$system extension is installed correctly on pg_available_extensions");
 
 
-my $ret4 = $node->safe_psql('postgres',
+$ret = $node->safe_psql('postgres',
 	"set extension_control_path = ''; select count(*) > 0 as ok from pg_available_extensions where name = 'plpgsql'"
 );
-is($ret4, "t",
+is($ret, "t",
 	"\$system extension is installed correctly on pg_available_extensions with empty extension_control_path"
 );
 
+# Test with an extension that does not exists
+my ($code, $stdout, $stderr) =  $node->psql('postgres', "CREATE EXTENSION invalid");
+is($code, 3, 'error to create an extension that does not exists');
+like($stderr, qr/ERROR:  extension "invalid" is not available/);
+
+sub create_extension
+{
+	my ($ext_name, $ext_dir, $directory) = @_;
+
+	my $control_file = "$ext_dir/extension/$ext_name.control";
+	my $sql_file;
+
+	if (defined $directory)
+	{
+		$sql_file = "$ext_dir/$directory/$ext_name--1.0.sql";
+	}
+	else
+	{
+		$sql_file = "$ext_dir/extension/$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";
+	if (defined $directory)
+	{
+		print $cf "directory = $directory";
+	}
+	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;
+}
+
 done_testing();
-- 
2.39.5 (Apple Git-154)



^ permalink  raw  reply  [nested|flat] 16+ messages in thread

* Re: extension_control_path and "directory"
@ 2025-04-29 15:12  David E. Wheeler <[email protected]>
  parent: Matheus Alcantara <[email protected]>
  1 sibling, 0 replies; 16+ messages in thread

From: David E. Wheeler @ 2025-04-29 15:12 UTC (permalink / raw)
  To: Matheus Alcantara <[email protected]>; +Cc: Christoph Berg <[email protected]>; Peter Eisentraut <[email protected]>; [email protected]

On Apr 29, 2025, at 11:06, Matheus Alcantara <[email protected]> wrote:

> The substitute_path_macro() already handles the if/else on "piece" but I
> think that this if/else version looks nicer. Fixed.
> 
> I've also included some documentation changes for this v5 version to
> remove the "extension" from the examples and also mention the scenario
> when using the "directory" on the .control file.

Nice, thanks. I’ve made a PR in my GitHub clone for anyone who likes to look it over that way.

https://github.com/theory/postgres/pull/11/files

Best,

David



Attachments:

  [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
  download

^ permalink  raw  reply  [nested|flat] 16+ messages in thread

* Re: extension_control_path and "directory"
@ 2025-05-02 14:51  Peter Eisentraut <[email protected]>
  parent: Matheus Alcantara <[email protected]>
  1 sibling, 1 reply; 16+ messages in thread

From: Peter Eisentraut @ 2025-05-02 14:51 UTC (permalink / raw)
  To: Matheus Alcantara <[email protected]>; David E. Wheeler <[email protected]>; +Cc: Christoph Berg <[email protected]>; [email protected]

On 29.04.25 17:06, Matheus Alcantara wrote:
> On Tue, Apr 29, 2025 at 11:08 AM David E. Wheeler <[email protected]> wrote:
>> Right. My point is a minor one, but I thin you can use an if/ else there:
>>
>> ```c
>> if (strcmp(piece, "$system") == 0) {
>>          /* Substitute the path macro if needed */
>>          mangled = substitute_path_macro(piece, "$system", system_dir);
>> } else {
>>          /*
>>          * Append "extension" suffix in case is a custom extension
>>          * control path.
>>          */
>>          mangled = psprintf("%s/extension", mangled);
>> }
>> ```
>>
> 
> The substitute_path_macro() already handles the if/else on "piece" but I
> think that this if/else version looks nicer. Fixed.
> 
> I've also included some documentation changes for this v5 version to
> remove the "extension" from the examples and also mention the scenario
> when using the "directory" on the .control file.

Thanks, I have committed this.  I did a bit of code reformatting and 
adjusted the documentation a bit.  It's good to get this in before beta1 
so that we don't have to change the valid values of 
extension_control_path past beta1.






^ permalink  raw  reply  [nested|flat] 16+ messages in thread

* Re: extension_control_path and "directory"
@ 2025-05-02 15:04  Matheus Alcantara <[email protected]>
  parent: Peter Eisentraut <[email protected]>
  0 siblings, 1 reply; 16+ messages in thread

From: Matheus Alcantara @ 2025-05-02 15:04 UTC (permalink / raw)
  To: Peter Eisentraut <[email protected]>; +Cc: David E. Wheeler <[email protected]>; Christoph Berg <[email protected]>; [email protected]

On Fri, May 2, 2025 at 11:51 AM Peter Eisentraut <[email protected]> wrote:
>
> Thanks, I have committed this.  I did a bit of code reformatting and
> adjusted the documentation a bit.  It's good to get this in before beta1
> so that we don't have to change the valid values of
> extension_control_path past beta1.
>

Thanks Peter!


-- 
Matheus Alcantara





^ permalink  raw  reply  [nested|flat] 16+ messages in thread

* Re: extension_control_path and "directory"
@ 2025-05-02 15:53  Christoph Berg <[email protected]>
  parent: Matheus Alcantara <[email protected]>
  0 siblings, 0 replies; 16+ messages in thread

From: Christoph Berg @ 2025-05-02 15:53 UTC (permalink / raw)
  To: Matheus Alcantara <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; David E. Wheeler <[email protected]>; [email protected]

Re: Matheus Alcantara
> > Thanks, I have committed this.  I did a bit of code reformatting and
> > adjusted the documentation a bit.  It's good to get this in before beta1
> > so that we don't have to change the valid values of
> > extension_control_path past beta1.
> 
> Thanks Peter!

And thanks everyone!

Christoph





^ permalink  raw  reply  [nested|flat] 16+ messages in thread

end of thread, other threads:[~2025-05-02 15:53 UTC | newest]

Thread overview: 16+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2018-11-27 05:42 [PATCH 5/5] Remove the GUC stats_temp_directory Kyotaro Horiguchi <[email protected]>
2019-04-01 04:07 [PATCH 3/4] GiST-Optimal-WAL-Usage Andrey V. Lepikhov <[email protected]>
2019-04-02 04:43 [PATCH 3/4] GiST-Optimal-WAL-Usage Andrey V. Lepikhov <[email protected]>
2025-04-24 15:18 Re: extension_control_path and "directory" Matheus Alcantara <[email protected]>
2025-04-24 22:27 ` Re: extension_control_path and "directory" David E. Wheeler <[email protected]>
2025-04-25 13:25   ` Re: extension_control_path and "directory" Matheus Alcantara <[email protected]>
2025-04-25 19:13     ` Re: extension_control_path and "directory" David E. Wheeler <[email protected]>
2025-04-25 21:18       ` Re: extension_control_path and "directory" Matheus Alcantara <[email protected]>
2025-04-28 20:49         ` Re: extension_control_path and "directory" David E. Wheeler <[email protected]>
2025-04-29 13:49           ` Re: extension_control_path and "directory" Matheus Alcantara <[email protected]>
2025-04-29 14:08             ` Re: extension_control_path and "directory" David E. Wheeler <[email protected]>
2025-04-29 15:06               ` Re: extension_control_path and "directory" Matheus Alcantara <[email protected]>
2025-04-29 15:12                 ` Re: extension_control_path and "directory" David E. Wheeler <[email protected]>
2025-05-02 14:51                 ` Re: extension_control_path and "directory" Peter Eisentraut <[email protected]>
2025-05-02 15:04                   ` Re: extension_control_path and "directory" Matheus Alcantara <[email protected]>
2025-05-02 15:53                     ` Re: extension_control_path and "directory" Christoph Berg <[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