public inbox for [email protected]
help / color / mirror / Atom feedFrom: Matheus Alcantara <[email protected]>
To: Christoph Berg <[email protected]>
Cc: David E. Wheeler <[email protected]>
Cc: Peter Eisentraut <[email protected]>
Cc: [email protected]
Subject: Re: extension_control_path and "directory"
Date: Thu, 24 Apr 2025 12:18:28 -0300
Message-ID: <CAFY6G8d-4Nb34dqb_92kGrXfo4tO012m-9YzznKmb7f5JhJpEA@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]>
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, 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)
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: <CAFY6G8d-4Nb34dqb_92kGrXfo4tO012m-9YzznKmb7f5JhJpEA@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