public inbox for [email protected]
help / color / mirror / Atom feedFrom: Matheus Alcantara <[email protected]>
To: David E. Wheeler <[email protected]>
Cc: Christoph Berg <[email protected]>
Cc: Peter Eisentraut <[email protected]>
Cc: [email protected]
Subject: Re: extension_control_path and "directory"
Date: Tue, 29 Apr 2025 12:06:51 -0300
Message-ID: <CAFY6G8dUXHRii5rNy7V8WmBrmBwp9W7y3g+HL6Tn-Lu8KkvK=A@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
<[email protected]>
<CAFY6G8dDjimxaSQV68ExWMmrAi8xNhiOT3RZOD8oQZmdGCG5Jg@mail.gmail.com>
<[email protected]>
<[email protected]>
<CAFY6G8c=W8CMFwcx2fA0RSFtvw=5BDGKOjGSV=nEUiZ6use2Bw@mail.gmail.com>
<[email protected]>
<CAFY6G8d-4Nb34dqb_92kGrXfo4tO012m-9YzznKmb7f5JhJpEA@mail.gmail.com>
<[email protected]>
<CAFY6G8fjaVpkZHFk3CQR=ShiysZ3Y_ti568-vuKYOrMepLB_sQ@mail.gmail.com>
<[email protected]>
<CAFY6G8dwnxqBDcQbVYfjJJ6RYMTxc04gip64Nx7X2fk4ggA+rQ@mail.gmail.com>
<[email protected]>
<CAFY6G8cmNw6H=Xz8Q45eKVy9aSKLy+GASGiq55nsnyxVo_pcwg@mail.gmail.com>
<[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, 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/<bla></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)
view thread (20+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: extension_control_path and "directory"
In-Reply-To: <CAFY6G8dUXHRii5rNy7V8WmBrmBwp9W7y3g+HL6Tn-Lu8KkvK=A@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox