Horton

Thread

Commits

  1. Fix: Don't strip $libdir from nested module_pathnames

  2. Don't strip $libdir from LOAD command

  1. Potential problem in commit f777d773878 and 4f7f7b03758

    Dilip Kumar <dilipbalaut@gmail.com> — 2025-08-22T10:34:33Z

    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

  2. Re: Potential problem in commit f777d773878 and 4f7f7b03758

    Dilip Kumar <dilipbalaut@gmail.com> — 2025-08-22T11:02:34Z

    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

  3. Re: Potential problem in commit f777d773878 and 4f7f7b03758

    Matheus Alcantara <matheusssilv97@gmail.com> — 2025-08-22T13:33:52Z

    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

  4. Re: Potential problem in commit f777d773878 and 4f7f7b03758

    Dilip Kumar <dilipbalaut@gmail.com> — 2025-08-23T11:05:55Z

    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

  5. Re: Potential problem in commit f777d773878 and 4f7f7b03758

    Srinath Reddy Sadipiralla <srinath2133@gmail.com> — 2025-08-24T12:28:44Z

    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/

  6. Re: Potential problem in commit f777d773878 and 4f7f7b03758

    Srinath Reddy Sadipiralla <srinath2133@gmail.com> — 2025-08-24T23:46:15Z

    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/

  7. Re: Potential problem in commit f777d773878 and 4f7f7b03758

    Dilip Kumar <dilipbalaut@gmail.com> — 2025-08-26T06:01:40Z

    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

  8. Re: Potential problem in commit f777d773878 and 4f7f7b03758

    Dilip Kumar <dilipbalaut@gmail.com> — 2025-08-26T06:26:53Z

    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

  9. Re: Potential problem in commit f777d773878 and 4f7f7b03758

    Matheus Alcantara <matheusssilv97@gmail.com> — 2025-08-27T12:39:33Z

    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

  10. Re: Potential problem in commit f777d773878 and 4f7f7b03758

    Peter Eisentraut <peter@eisentraut.org> — 2025-08-27T14:17:42Z

    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

  11. Re: Potential problem in commit f777d773878 and 4f7f7b03758

    Dilip Kumar <dilipbalaut@gmail.com> — 2025-08-28T04:14:34Z

    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. Re: Potential problem in commit f777d773878 and 4f7f7b03758

    Álvaro Herrera <alvherre@kurilemu.de> — 2025-09-04T11:43:33Z

    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)

  13. Re: Potential problem in commit f777d773878 and 4f7f7b03758

    Dilip Kumar <dilipbalaut@gmail.com> — 2025-09-04T12:02:27Z

    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. Re: Potential problem in commit f777d773878 and 4f7f7b03758

    Álvaro Herrera <alvherre@kurilemu.de> — 2025-09-08T17:06:03Z

    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/

  15. Re: Potential problem in commit f777d773878 and 4f7f7b03758

    Dilip Kumar <dilipbalaut@gmail.com> — 2025-09-09T04:40:42Z

    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