Re: Potential problem in commit f777d773878 and 4f7f7b03758

On Fri, Aug 22, 2025 at 7:04 PM Matheus Alcantara
<matheusssilv97(at)gmail(dot)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