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 10:25:14 -0300
Message-ID: <CAFY6G8fjaVpkZHFk3CQR=ShiysZ3Y_ti568-vuKYOrMepLB_sQ@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]>
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, 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)
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: <CAFY6G8fjaVpkZHFk3CQR=ShiysZ3Y_ti568-vuKYOrMepLB_sQ@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