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: Fri, 25 Apr 2025 18:18:58 -0300
Message-ID: <CAFY6G8dwnxqBDcQbVYfjJJ6RYMTxc04gip64Nx7X2fk4ggA+rQ@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]>
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, 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)
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: <CAFY6G8dwnxqBDcQbVYfjJJ6RYMTxc04gip64Nx7X2fk4ggA+rQ@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