Обсуждение: Potential problem in commit f777d773878 and 4f7f7b03758
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?
[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
--
Regards,
Dilip Kumar
Google
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
Вложения
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
Вложения
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
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 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.
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 "
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 doubtthat in f777d773878 commit the $libdir was moved out from expand_dynamic_library_name into load_external_function becauseif someone specifies LOAD '$libdir/foo' explicitly they want to get the foo.so from $libdir not from other paths givenin dynamic_library_path ,i think same should go for the case when we do "create extension" will try to execute the sqlscript which will replace the MODULE_PATHNAME with module_pathname from .control file lets say which is $libdir/foo ,nowduring the sql functions execution this calls the load_external_function where we strip $libdir and we are going to loadthe foo.so from other paths specified in dynamic_library_path, isn't that a problem , i think this case is also sameas 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
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 doubtthat in f777d773878 commit the $libdir was moved out from expand_dynamic_library_name into load_external_function becauseif someone specifies LOAD '$libdir/foo' explicitly they want to get the foo.so from $libdir not from other paths givenin dynamic_library_path ,i think same should go for the case when we do "create extension" will try to execute the sqlscript which will replace the MODULE_PATHNAME with module_pathname from .control file lets say which is $libdir/foo ,nowduring the sql functions execution this calls the load_external_function where we strip $libdir and we are going to loadthe foo.so from other paths specified in dynamic_library_path, isn't that a problem , i think this case is also sameas 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
Вложения
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
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
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
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)
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
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/
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