Potential problem in commit f777d773878 and 4f7f7b03758

Started by Dilip Kumar5 months ago15 messages
#1Dilip Kumar
dilipbalaut@gmail.com

Simple test to reproduce:

Change the module path for any extension as below, like I have done
for amcheck and then copy the .so file at $libdir/test/ folder.
module_pathname = '$libdir/test/amcheck'

While creating the extension it fail to load the file because, after
commit f777d773878 we remove the $libdir from the path[1]+ if (strncmp(filename, "$libdir/", 8) == 0) + filename += 8; and later in
expand_dynamic_library_name() since there is a '/' in remaining path
we will call full = substitute_path_macro(name, "$libdir",
pkglib_path); to generate the full path by substituting the "$libdir"
macro[2]if (!have_slash) { full = find_in_path(name, Dynamic_library_path, "dynamic_library_path", "$libdir", pkglib_path); if (full) return full; } else { full = substitute_path_macro(name, "$libdir", pkglib_path); if (pg_file_exists(full)) return full; pfree(full. But we have already removed $libdir from the filename, so
there will be no substitution and it will just search the
'test/amcheck' path, which was not intended.

IMHO the fix should be that we need to preserve the original name as
well, and in the else case we should pass the original name which is
'$libdir/test/amcheck' in this case so that macro substitution can
work properly?

[1]
+       if (strncmp(filename, "$libdir/", 8) == 0)
+               filename += 8;

[2]: if (!have_slash) { full = find_in_path(name, Dynamic_library_path, "dynamic_library_path", "$libdir", pkglib_path); if (full) return full; } else { full = substitute_path_macro(name, "$libdir", pkglib_path); if (pg_file_exists(full)) return full; pfree(full
if (!have_slash)
{
full = find_in_path(name, Dynamic_library_path,
"dynamic_library_path", "$libdir", pkglib_path);
if (full)
return full;
}
else
{
full = substitute_path_macro(name, "$libdir", pkglib_path);
if (pg_file_exists(full))
return full;
pfree(full

--
Regards,
Dilip Kumar
Google

#2Dilip Kumar
dilipbalaut@gmail.com
In reply to: Dilip Kumar (#1)
1 attachment(s)
Re: Potential problem in commit f777d773878 and 4f7f7b03758

On Fri, Aug 22, 2025 at 4:04 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:

Simple test to reproduce:

Change the module path for any extension as below, like I have done
for amcheck and then copy the .so file at $libdir/test/ folder.
module_pathname = '$libdir/test/amcheck'

While creating the extension it fail to load the file because, after
commit f777d773878 we remove the $libdir from the path[1] and later in
expand_dynamic_library_name() since there is a '/' in remaining path
we will call full = substitute_path_macro(name, "$libdir",
pkglib_path); to generate the full path by substituting the "$libdir"
macro[2]. But we have already removed $libdir from the filename, so
there will be no substitution and it will just search the
'test/amcheck' path, which was not intended.

IMHO the fix should be that we need to preserve the original name as
well, and in the else case we should pass the original name which is
'$libdir/test/amcheck' in this case so that macro substitution can
work properly?

I just quickly changed this and it's working fine, but I haven't
analyzed fully whether it breaks something which was intended by the
commit (f777d773878 and 4f7f7b03758) but check-world is passing.

--
Regards,
Dilip Kumar
Google

Attachments:

poc_fix.patchapplication/octet-stream; name=poc_fix.patchDownload
diff --git a/src/backend/utils/fmgr/dfmgr.c b/src/backend/utils/fmgr/dfmgr.c
index 4bb84ff7087..3eb791e541b 100644
--- a/src/backend/utils/fmgr/dfmgr.c
+++ b/src/backend/utils/fmgr/dfmgr.c
@@ -71,7 +71,7 @@ char	   *Dynamic_library_path;
 static void *internal_load_library(const char *libname);
 pg_noreturn static void incompatible_module_error(const char *libname,
 												  const Pg_abi_values *module_magic_data);
-static char *expand_dynamic_library_name(const char *name);
+static char *expand_dynamic_library_name(const char *name, const char *original_name);
 static void check_restricted_library_name(const char *name);
 
 /* ABI values that module needs to match to be accepted */
@@ -96,6 +96,7 @@ load_external_function(const char *filename, const char *funcname,
 					   bool signalNotFound, void **filehandle)
 {
 	char	   *fullname;
+	const char *original_name = filename;
 	void	   *lib_handle;
 	void	   *retval;
 
@@ -108,7 +109,7 @@ load_external_function(const char *filename, const char *funcname,
 		filename += 8;
 
 	/* Expand the possibly-abbreviated filename to an exact path name */
-	fullname = expand_dynamic_library_name(filename);
+	fullname = expand_dynamic_library_name(filename, original_name);
 
 	/* Load the shared library, unless we already did */
 	lib_handle = internal_load_library(fullname);
@@ -148,7 +149,7 @@ load_file(const char *filename, bool restricted)
 		check_restricted_library_name(filename);
 
 	/* Expand the possibly-abbreviated filename to an exact path name */
-	fullname = expand_dynamic_library_name(filename);
+	fullname = expand_dynamic_library_name(filename, filename);
 
 	/* Load the shared library, unless we already did */
 	(void) internal_load_library(fullname);
@@ -456,7 +457,7 @@ get_loaded_module_details(DynamicFileList *dfptr,
  * The result will always be freshly palloc'd.
  */
 static char *
-expand_dynamic_library_name(const char *name)
+expand_dynamic_library_name(const char *name, const char *original_name)
 {
 	bool		have_slash;
 	char	   *new;
@@ -474,16 +475,16 @@ expand_dynamic_library_name(const char *name)
 	}
 	else
 	{
-		full = substitute_path_macro(name, "$libdir", pkglib_path);
+		full = substitute_path_macro(original_name, "$libdir", pkglib_path);
 		if (pg_file_exists(full))
 			return full;
 		pfree(full);
 	}
 
-	new = psprintf("%s%s", name, DLSUFFIX);
-
 	if (!have_slash)
 	{
+		new = psprintf("%s%s", name, DLSUFFIX);
+
 		full = find_in_path(new, Dynamic_library_path, "dynamic_library_path", "$libdir", pkglib_path);
 		pfree(new);
 		if (full)
@@ -491,6 +492,8 @@ expand_dynamic_library_name(const char *name)
 	}
 	else
 	{
+		new = psprintf("%s%s", original_name, DLSUFFIX);
+
 		full = substitute_path_macro(new, "$libdir", pkglib_path);
 		pfree(new);
 		if (pg_file_exists(full))
#3Matheus Alcantara
matheusssilv97@gmail.com
In reply to: Dilip Kumar (#1)
1 attachment(s)
Re: Potential problem in commit f777d773878 and 4f7f7b03758

Hi,

Thanks for reporting this!

On Fri Aug 22, 2025 at 7:34 AM -03, Dilip Kumar wrote:

Simple test to reproduce:

Change the module path for any extension as below, like I have done
for amcheck and then copy the .so file at $libdir/test/ folder.
module_pathname = '$libdir/test/amcheck'

While creating the extension it fail to load the file because, after
commit f777d773878 we remove the $libdir from the path[1] and later in
expand_dynamic_library_name() since there is a '/' in remaining path
we will call full = substitute_path_macro(name, "$libdir",
pkglib_path); to generate the full path by substituting the "$libdir"
macro[2]. But we have already removed $libdir from the filename, so
there will be no substitution and it will just search the
'test/amcheck' path, which was not intended.

IMHO the fix should be that we need to preserve the original name as
well, and in the else case we should pass the original name which is
'$libdir/test/amcheck' in this case so that macro substitution can
work properly?

Using multiple names seems a bit confusing to me TBH. I think that a
more simple approach would be strip the $libdir from filename on
load_external_function() only if after the strip it doesn't have any
remaining path separator. With this, if the user write $libdir/foo/bar
we assume that he wants to load from a chield directory on $libidir, so
we don't strip from filename, otherwise if module_pathname only has
$libdir/bar we assume that it is using the previous behaviour and we can
strip to enable the search path on extension_control_path GUC.

Please see the attached patch to check if it solves your issue? I
still need to perform some other tests to validate that this fix is
fully correct but the check-world is passing.

Thoughts?

--
Matheus Alcantara

Attachments:

v0-0001-Don-t-strip-libdir-from-nested-module_pathnames.patchapplication/x-patch; name=v0-0001-Don-t-strip-libdir-from-nested-module_pathnames.patchDownload
From c635b700b229a94101461e034bf739451c9f3b67 Mon Sep 17 00:00:00 2001
From: Matheus Alcantara <mths.dev@pm.me>
Date: Fri, 22 Aug 2025 10:23:31 -0300
Subject: [PATCH v0] Don't strip $libdir from nested module_pathnames

---
 src/backend/utils/fmgr/dfmgr.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/src/backend/utils/fmgr/dfmgr.c b/src/backend/utils/fmgr/dfmgr.c
index 4bb84ff7087..2ba86f3aeba 100644
--- a/src/backend/utils/fmgr/dfmgr.c
+++ b/src/backend/utils/fmgr/dfmgr.c
@@ -100,12 +100,20 @@ load_external_function(const char *filename, const char *funcname,
 	void	   *retval;
 
 	/*
-	 * If the value starts with "$libdir/", strip that.  This is because many
-	 * extensions have hardcoded '$libdir/foo' as their library name, which
-	 * prevents using the path.
+	 * Check if the filename starts with "$libdir/". If it does, and the
+	 * remaining part of the string contains no directory separators, advance
+	 * the pointer to strip the prefix. This ensures that only simple
+	 * filenames (e.g., "$libdir/foo") are stripped, while full paths (e.g.,
+	 * "$libdir/foo/bar") are left untouched.
+	 *
+	 * This is because many extensions have hardcoded '$libdir/foo' as their
+	 * library name, which prevents using the search path.
 	 */
 	if (strncmp(filename, "$libdir/", 8) == 0)
-		filename += 8;
+	{
+		if (first_dir_separator(filename + 8) == NULL)
+			filename += 8;
+	}
 
 	/* Expand the possibly-abbreviated filename to an exact path name */
 	fullname = expand_dynamic_library_name(filename);
-- 
2.39.5 (Apple Git-154)

#4Dilip Kumar
dilipbalaut@gmail.com
In reply to: Matheus Alcantara (#3)
Re: Potential problem in commit f777d773878 and 4f7f7b03758

On Fri, Aug 22, 2025 at 7:04 PM Matheus Alcantara
<matheusssilv97@gmail.com> wrote:

Hi,

Thanks for reporting this!

On Fri Aug 22, 2025 at 7:34 AM -03, Dilip Kumar wrote:

Simple test to reproduce:

Change the module path for any extension as below, like I have done
for amcheck and then copy the .so file at $libdir/test/ folder.
module_pathname = '$libdir/test/amcheck'

While creating the extension it fail to load the file because, after
commit f777d773878 we remove the $libdir from the path[1] and later in
expand_dynamic_library_name() since there is a '/' in remaining path
we will call full = substitute_path_macro(name, "$libdir",
pkglib_path); to generate the full path by substituting the "$libdir"
macro[2]. But we have already removed $libdir from the filename, so
there will be no substitution and it will just search the
'test/amcheck' path, which was not intended.

IMHO the fix should be that we need to preserve the original name as
well, and in the else case we should pass the original name which is
'$libdir/test/amcheck' in this case so that macro substitution can
work properly?

Using multiple names seems a bit confusing to me TBH. I think that a
more simple approach would be strip the $libdir from filename on
load_external_function() only if after the strip it doesn't have any
remaining path separator. With this, if the user write $libdir/foo/bar
we assume that he wants to load from a chield directory on $libidir, so
we don't strip from filename, otherwise if module_pathname only has
$libdir/bar we assume that it is using the previous behaviour and we can
strip to enable the search path on extension_control_path GUC.

Please see the attached patch to check if it solves your issue? I
still need to perform some other tests to validate that this fix is
fully correct but the check-world is passing.

Thoughts?

Yeah I first thought of something like that but I thought we could
avoid calling first_dir_separator() multiple times once in
load_external_function and other inside expand_dynamic_library_name.
But maybe this doesn't matter as this is not really a performance
path. So this change makes sense, thanks.

--
Regards,
Dilip Kumar
Google

#5Srinath Reddy Sadipiralla
srinath2133@gmail.com
In reply to: Dilip Kumar (#4)
Re: Potential problem in commit f777d773878 and 4f7f7b03758

Hi,
Thanks Dilip and Matheus for working on this , i reviewed the latest patch
given my Matheus and it LGTM but i have doubt that in f777d773878 commit
the $libdir was moved out from expand_dynamic_library_name
into load_external_function because if someone specifies LOAD '$libdir/foo'
explicitly they want to get the foo.so from $libdir not from other paths
given in dynamic_library_path ,i think same should go for the case when we
do "create extension" will try to execute the sql script which will replace
the MODULE_PATHNAME with module_pathname from .control file lets say which
is $libdir/foo ,now during the sql functions execution this calls the
load_external_function where we strip $libdir and we are going to load the
foo.so from other paths specified in dynamic_library_path, isn't that a
problem , i think this case is also same as with the LOAD.

--
Thanks,
Srinath Reddy Sadipiralla
EDB: https://www.enterprisedb.com/

#6Srinath Reddy Sadipiralla
srinath2133@gmail.com
In reply to: Srinath Reddy Sadipiralla (#5)
Re: Potential problem in commit f777d773878 and 4f7f7b03758

On Sun, Aug 24, 2025 at 5:58 PM Srinath Reddy Sadipiralla <
srinath2133@gmail.com> wrote:

Hi,
Thanks Dilip and Matheus for working on this , i reviewed the latest patch
given my Matheus and it LGTM but i have doubt that in f777d773878 commit
the $libdir was moved out from expand_dynamic_library_name
into load_external_function because if someone specifies LOAD '$libdir/foo'
explicitly they want to get the foo.so from $libdir not from other paths
given in dynamic_library_path ,i think same should go for the case when we
do "create extension" will try to execute the sql script which will replace
the MODULE_PATHNAME with module_pathname from .control file lets say which
is $libdir/foo ,now during the sql functions execution this calls the
load_external_function where we strip $libdir and we are going to load the
foo.so from other paths specified in dynamic_library_path, isn't that a
problem , i think this case is also same as with the LOAD.

sorry i missed to mention "stripping of $libdir" here " but i have a doubt
that in f777d773878 commit the stripping of $libdir was moved out
from expand_dynamic_library_name into load_external_function "

--
Thanks,
Srinath Reddy Sadipiralla
EDB: https://www.enterprisedb.com/

#7Dilip Kumar
dilipbalaut@gmail.com
In reply to: Srinath Reddy Sadipiralla (#5)
Re: Potential problem in commit f777d773878 and 4f7f7b03758

On Sun, Aug 24, 2025 at 5:59 PM Srinath Reddy Sadipiralla
<srinath2133@gmail.com> wrote:

Hi,
Thanks Dilip and Matheus for working on this , i reviewed the latest patch given my Matheus and it LGTM but i have doubt that in f777d773878 commit the $libdir was moved out from expand_dynamic_library_name into load_external_function because if someone specifies LOAD '$libdir/foo' explicitly they want to get the foo.so from $libdir not from other paths given in dynamic_library_path ,i think same should go for the case when we do "create extension" will try to execute the sql script which will replace the MODULE_PATHNAME with module_pathname from .control file lets say which is $libdir/foo ,now during the sql functions execution this calls the load_external_function where we strip $libdir and we are going to load the foo.so from other paths specified in dynamic_library_path, isn't that a problem , i think this case is also same as with the LOAD.

IMHO the cases like $libdir/foo is not a problem because first we will
strip the $libdir and then we will try to find the foo in the
'dynamic_library_path' and the default value of dynamic_library_path
is $libdir so everything should work fine. OTOH the case I report has
$libdir/xyz/foo, in this case it doesn't search in
'dynamic_library_path' instead try to replace the $libdir macro, but
we have already stripped the $libdir so this will not behave
correctly, so if we have any extra slash in path we need to retain the
$libdir

--
Regards,
Dilip Kumar
Google

#8Dilip Kumar
dilipbalaut@gmail.com
In reply to: Dilip Kumar (#7)
1 attachment(s)
Re: Potential problem in commit f777d773878 and 4f7f7b03758

On Tue, Aug 26, 2025 at 11:31 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Sun, Aug 24, 2025 at 5:59 PM Srinath Reddy Sadipiralla
<srinath2133@gmail.com> wrote:

Hi,
Thanks Dilip and Matheus for working on this , i reviewed the latest patch given my Matheus and it LGTM but i have doubt that in f777d773878 commit the $libdir was moved out from expand_dynamic_library_name into load_external_function because if someone specifies LOAD '$libdir/foo' explicitly they want to get the foo.so from $libdir not from other paths given in dynamic_library_path ,i think same should go for the case when we do "create extension" will try to execute the sql script which will replace the MODULE_PATHNAME with module_pathname from .control file lets say which is $libdir/foo ,now during the sql functions execution this calls the load_external_function where we strip $libdir and we are going to load the foo.so from other paths specified in dynamic_library_path, isn't that a problem , i think this case is also same as with the LOAD.

IMHO the cases like $libdir/foo is not a problem because first we will
strip the $libdir and then we will try to find the foo in the
'dynamic_library_path' and the default value of dynamic_library_path
is $libdir so everything should work fine. OTOH the case I report has
$libdir/xyz/foo, in this case it doesn't search in
'dynamic_library_path' instead try to replace the $libdir macro, but
we have already stripped the $libdir so this will not behave
correctly, so if we have any extra slash in path we need to retain the
$libdir

Please find the revised patch with improved comments and commit messages.

@Peter Eisentraut since you committed this feature, so tagging you to
see do you think this needs to be fixed? Thanks.

--
Regards,
Dilip Kumar
Google

Attachments:

v1-0001-Fix-Don-t-strip-libdir-from-nested-module_pathnam.patchapplication/octet-stream; name=v1-0001-Fix-Don-t-strip-libdir-from-nested-module_pathnam.patchDownload
From b08f54adddc0beaa79a330dd3c9e2f3ff25b3d08 Mon Sep 17 00:00:00 2001
From: Matheus Alcantara <mths.dev@pm.me>
Date: Fri, 22 Aug 2025 10:23:31 -0300
Subject: [PATCH v1] Fix: Don't strip $libdir from nested module_pathnames

This patch fixes a bug in how 'load_external_function' handles
'$libdir/ prefixes in module paths.

Previously, 'load_external_function' would unconditionally strip
'$libdir/' from the beginning of the 'filename' string. This
caused an issue when the path was nested, such as "$libdir/nested/my_lib".
Stripping the prefix resulted in a path of "nested/my_lib", which would
fail to be found by the expand_dynamic_library_name function because
the original '$libdir' macro was removed.

To fix this, the code now checks for the presence of an additional
directory separator ('/' or '\') after the '$libdir/' prefix. The prefix
is only stripped if the remaining string does not contain a separator.
This ensures that simple filenames like '"$libdir/my_lib"' are correctly
handled, while nested paths are left intact for 'expand_dynamic_library_name'
to process correctly.

Reported By: Dilip Kumar
Author: Matheus Alcantara
Author: Dilip Kumar
Reviewer: Srinath Reddy Sadipiralla
---
 src/backend/utils/fmgr/dfmgr.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/src/backend/utils/fmgr/dfmgr.c b/src/backend/utils/fmgr/dfmgr.c
index 4bb84ff7087..d39966829fd 100644
--- a/src/backend/utils/fmgr/dfmgr.c
+++ b/src/backend/utils/fmgr/dfmgr.c
@@ -100,12 +100,19 @@ load_external_function(const char *filename, const char *funcname,
 	void	   *retval;
 
 	/*
-	 * If the value starts with "$libdir/", strip that.  This is because many
-	 * extensions have hardcoded '$libdir/foo' as their library name, which
-	 * prevents using the path.
+	 * For extensions with hardcoded '$libdir/' library names, we strip the
+	 * prefix to allow the library search path to be used. This is done only
+	 * for simple names (e.g., "$libdir/foo"), not for nested paths
+	 * (e.g., "$libdir/foo/bar").
+	 *
+	 * For nested paths, 'expand_dynamic_library_name' directly expands the
+	 * '$libdir' macro, so we leave them untouched.
 	 */
 	if (strncmp(filename, "$libdir/", 8) == 0)
-		filename += 8;
+	{
+		if (first_dir_separator(filename + 8) == NULL)
+			filename += 8;
+	}
 
 	/* Expand the possibly-abbreviated filename to an exact path name */
 	fullname = expand_dynamic_library_name(filename);
-- 
2.51.0.261.g7ce5a0a67e-goog

#9Matheus Alcantara
matheusssilv97@gmail.com
In reply to: Dilip Kumar (#8)
Re: Potential problem in commit f777d773878 and 4f7f7b03758

On Tue Aug 26, 2025 at 3:26 AM -03, Dilip Kumar wrote:

On Tue, Aug 26, 2025 at 11:31 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Sun, Aug 24, 2025 at 5:59 PM Srinath Reddy Sadipiralla
<srinath2133@gmail.com> wrote:

Hi,
Thanks Dilip and Matheus for working on this , i reviewed the
latest patch given my Matheus and it LGTM but i have doubt that in
f777d773878 commit the $libdir was moved out from
expand_dynamic_library_name into load_external_function because if
someone specifies LOAD '$libdir/foo' explicitly they want to get
the foo.so from $libdir not from other paths given in
dynamic_library_path ,i think same should go for the case when we
do "create extension" will try to execute the sql script which will
replace the MODULE_PATHNAME with module_pathname from .control file
lets say which is $libdir/foo ,now during the sql functions
execution this calls the load_external_function where we strip
$libdir and we are going to load the foo.so from other paths
specified in dynamic_library_path, isn't that a problem , i think
this case is also same as with the LOAD.

IMHO the cases like $libdir/foo is not a problem because first we will
strip the $libdir and then we will try to find the foo in the
'dynamic_library_path' and the default value of dynamic_library_path
is $libdir so everything should work fine. OTOH the case I report has
$libdir/xyz/foo, in this case it doesn't search in
'dynamic_library_path' instead try to replace the $libdir macro, but
we have already stripped the $libdir so this will not behave
correctly, so if we have any extra slash in path we need to retain the
$libdir

Please find the revised patch with improved comments and commit messages.

Thanks for the improvements! LGTM.

--
Matheus Alcantara

#10Peter Eisentraut
peter@eisentraut.org
In reply to: Matheus Alcantara (#9)
Re: Potential problem in commit f777d773878 and 4f7f7b03758

On 27.08.25 14:39, Matheus Alcantara wrote:

On Tue Aug 26, 2025 at 3:26 AM -03, Dilip Kumar wrote:

On Tue, Aug 26, 2025 at 11:31 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Sun, Aug 24, 2025 at 5:59 PM Srinath Reddy Sadipiralla
<srinath2133@gmail.com> wrote:

Hi,
Thanks Dilip and Matheus for working on this , i reviewed the
latest patch given my Matheus and it LGTM but i have doubt that in
f777d773878 commit the $libdir was moved out from
expand_dynamic_library_name into load_external_function because if
someone specifies LOAD '$libdir/foo' explicitly they want to get
the foo.so from $libdir not from other paths given in
dynamic_library_path ,i think same should go for the case when we
do "create extension" will try to execute the sql script which will
replace the MODULE_PATHNAME with module_pathname from .control file
lets say which is $libdir/foo ,now during the sql functions
execution this calls the load_external_function where we strip
$libdir and we are going to load the foo.so from other paths
specified in dynamic_library_path, isn't that a problem , i think
this case is also same as with the LOAD.

IMHO the cases like $libdir/foo is not a problem because first we will
strip the $libdir and then we will try to find the foo in the
'dynamic_library_path' and the default value of dynamic_library_path
is $libdir so everything should work fine. OTOH the case I report has
$libdir/xyz/foo, in this case it doesn't search in
'dynamic_library_path' instead try to replace the $libdir macro, but
we have already stripped the $libdir so this will not behave
correctly, so if we have any extra slash in path we need to retain the
$libdir

Please find the revised patch with improved comments and commit messages.

Thanks for the improvements! LGTM.

committed, thanks

#11Dilip Kumar
dilipbalaut@gmail.com
In reply to: Peter Eisentraut (#10)
Re: Potential problem in commit f777d773878 and 4f7f7b03758

On Wed, Aug 27, 2025 at 7:47 PM Peter Eisentraut <peter@eisentraut.org> wrote:

Please find the revised patch with improved comments and commit messages.

Thanks for the improvements! LGTM.

committed, thanks

Thanks Peter.

--
Regards,
Dilip Kumar
Google

#12Álvaro Herrera
alvherre@kurilemu.de
In reply to: Peter Eisentraut (#10)
Re: Potential problem in commit f777d773878 and 4f7f7b03758

On 2025-Aug-27, Peter Eisentraut wrote:

On Sun, Aug 24, 2025 at 5:59 PM Srinath Reddy Sadipiralla
<srinath2133@gmail.com> wrote:

Thanks Dilip and Matheus for working on this , i reviewed the
latest patch given [by] Matheus and it LGTM but i have doubt
that in f777d773878 commit the $libdir was moved out from
expand_dynamic_library_name into load_external_function
because if someone specifies LOAD '$libdir/foo' explicitly
they want to get the foo.so from $libdir not from other paths
given in dynamic_library_path

committed, thanks

Was the above-quoted complaint addressed in some way? I see that Dilip
and Matheus disregarded as not important, but I'm not so sure that
that's really true.

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"Hay quien adquiere la mala costumbre de ser infeliz" (M. A. Evans)

#13Dilip Kumar
dilipbalaut@gmail.com
In reply to: Álvaro Herrera (#12)
Re: Potential problem in commit f777d773878 and 4f7f7b03758

On Thu, Sep 4, 2025 at 5:13 PM Álvaro Herrera <alvherre@kurilemu.de> wrote:

On 2025-Aug-27, Peter Eisentraut wrote:

On Sun, Aug 24, 2025 at 5:59 PM Srinath Reddy Sadipiralla
<srinath2133@gmail.com> wrote:

Thanks Dilip and Matheus for working on this , i reviewed the
latest patch given [by] Matheus and it LGTM but i have doubt
that in f777d773878 commit the $libdir was moved out from
expand_dynamic_library_name into load_external_function
because if someone specifies LOAD '$libdir/foo' explicitly
they want to get the foo.so from $libdir not from other paths
given in dynamic_library_path

committed, thanks

Was the above-quoted complaint addressed in some way? I see that Dilip
and Matheus disregarded as not important, but I'm not so sure that
that's really true.

What's the complain, Srinath complained that if you try to LOAD
'$libdir/foo' explicitly then it should load this from $libdir path
and it will indeed do so because it will directly call
expand_dynamic_library_name() and in this function if there is '/' it
will replace the $libdir macro with pkglib_path which is right thing
to do so it would behave as expected, am I missing something?

--
Regards,
Dilip Kumar
Google

#14Álvaro Herrera
alvherre@kurilemu.de
In reply to: Dilip Kumar (#13)
Re: Potential problem in commit f777d773878 and 4f7f7b03758

On 2025-Sep-04, Dilip Kumar wrote:

What's the complain, Srinath complained that if you try to LOAD
'$libdir/foo' explicitly then it should load this from $libdir path
and it will indeed do so because it will directly call
expand_dynamic_library_name() and in this function if there is '/' it
will replace the $libdir macro with pkglib_path which is right thing
to do so it would behave as expected, am I missing something?

Ah, I see, and I agree that this is probably the desired result. It's
not what you said on your first response to him though, or at least I
didn't (and still don't) understand it that way.

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/

#15Dilip Kumar
dilipbalaut@gmail.com
In reply to: Álvaro Herrera (#14)
Re: Potential problem in commit f777d773878 and 4f7f7b03758

On Mon, Sep 8, 2025 at 10:36 PM Álvaro Herrera <alvherre@kurilemu.de> wrote:

On 2025-Sep-04, Dilip Kumar wrote:

What's the complain, Srinath complained that if you try to LOAD
'$libdir/foo' explicitly then it should load this from $libdir path
and it will indeed do so because it will directly call
expand_dynamic_library_name() and in this function if there is '/' it
will replace the $libdir macro with pkglib_path which is right thing
to do so it would behave as expected, am I missing something?

Ah, I see, and I agree that this is probably the desired result. It's
not what you said on your first response to him though, or at least I
didn't (and still don't) understand it that way.

I agree, when I read my response again, I realized, in that response,
I did not exactly reply to the problem he raised.

--
Regards,
Dilip Kumar
Google