Thread: BUG #18920: LOAD '$libdir/plugins' no longer works in 18beta1
The following bug has been logged on the website: Bug reference: 18920 Logged by: Evan Si Email address: evsi@amazon.com PostgreSQL version: 18beta1 Operating system: Amazon Linux 2 Description: Hi, The 18beta1 documentation for the LOAD command has remained the same since previous major versions. https://www.postgresql.org/docs/18/sql-load.html In the postgres installation, I can copy something e.g. auto_explain into a plugins subdirectory of `pg_config --pkglibdir` ``` cp $(pg_config --pkglibdir)/auto_explain.so $(pg_config --pkglibdir)/plugins/auto_explain.so ``` then from the server start: ``` psql (18beta1) Type "help" for help. postgres=# LOAD '$libdir/plugins/auto_explain.so'; ERROR: could not access file "plugins/auto_explain.so": No such file or directory postgres=# \q ``` This behavior differs from documentation and previous majors. It seems to me that $libdir is getting stripped out from this commit: https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=4f7f7b0 -- Evan
On Mon, Jun 02, 2025 at 11:05:12AM -0300, Matheus Alcantara wrote: > The problem is how we are sripping the $libdir from module_pathname. I > think that we should still strip the $libdir from module_pathname when > expanding from a .control file, but we should not expand via LOAD > command, since the user may explicitly specify the $libdir prefix as it > is the case here. > > Having this logic on expand_dynamic_library_name() force us to srip the > $libdir for all scenarios, so I think that a better place to have this > is on load_external_function() that is called when executing a SQL > script from an extension. Thanks for the patch. This seems generally reasonable to me. -- nathan
Hi,
Thank you both for the patch and the explanation regarding the fix.
Another approach would be to change the
signature of expand_dynamic_library_name() to receive a boolean and only
srip the $libdir if it's true, but I think that the expand function
should not handle the strip logic, it should only expand the dynamic
library path and nothing else.
The reasoning makes sense to me.
I'm also attaching a second patch that has a TAP test case that
reproduce this issue, it has some hacks to move dynamic library files
around to exercice the problem so I'm not sure if it could be good to
also include these tests on the final patch.
The TAP test is useful in testing various paths of extension files.
It will be cleaner if the dummy_index_am.so is deleted at the end of the
tests from the $libdir.
Currently, the tests seem to be failing as follows for windows:
Currently, the tests seem to be failing as follows for windows:
[19:07:13.107] # Failed test 'create extension from custom extension control path' [19:07:13.107] # at C:/cirrus/src/test/modules/test_extensions/t/002_strip_modulepathname.pl line 102. [19:07:13.107] # got: '2' [19:07:13.107] # expected: '0' [19:07:13.107] # Looks like you failed 1 test of 7. [19:07:13.107] [19:07:13.107] (test program exited with status code 1)
Rahila Syed
On Mon, Jun 02, 2025 at 07:56:01PM -0300, Matheus Alcantara wrote: > On 02/06/25 16:56, Rahila Syed wrote: >> Currently, the tests seem to be failing as follows for windows: >> >> [19:07:13.107] # Failed test 'create extension from custom extension >> control path'[19:07:13.107] # at >> C:/cirrus/src/test/modules/test_extensions/t/002_strip_modulepathname.pl >> line 102.[19:07:13.107] # got: '2'[19:07:13.107] # >> expected: '0'[19:07:13.107] # Looks like you failed 1 test of >> 7.[19:07:13.107] [19:07:13.107] (test program exited with status code >> 1) >> >> Details here: Windows - Server 2019, VS 2019 - Meson & ninja - Cirrus CI >> <https://cirrus-ci.com/task/5241991606304768> > > Fixed, v2 attached. The 0001 patch remains the same as v1. I am adding an open item for Peter Eisentraut, for 4f7f7b037585. The patch looks sensible at quick glance, I did not check it in details regarding the internals of dfmgr.c. I don't like much the tests you are adding here. First, the cross-platform requirements to copy a library to the plugins/ folder is annoying. The actual issue is that we don't have an installation rule to be able to install a library to a plugins/ path to allow non-superusers to load it, no? I mean, as 4f7f7b037585 states, we have a `make install prefix=/else/where` but the TAP tests cannot shape a temporary installation with it. If we had something like that for meson and makefiles, we could then reuse EXTRA_INSTALL to force a library to exist where we want it to be, for the sake of testing coverage. I am not completely sure that the tests are completely waterproof, either. Some distributions like fancy installation folder layers, like Debian, and such things have proven to break these folk's tests. Having a centralized rule could be also useful for out-of-core extensions, to give these a way to install something inside a plugins/ folder. At least that may be better than requiring pg_config to get the basic library install path. Second, requiring dummy_index_am inside the tests test_extensions is adding unwelcome complexity across the test modules. -- Michael
Attachment
On Tue, Jun 03, 2025 at 09:31:17AM -0300, Matheus Alcantara wrote: > I just wrote these tests to make it > possible to reproduce the issue on my local machine and make the fix > easier, and I just shared here so that folks can also have a way to > reproduce the issue and maybe share other ideas to test this. +1. These are useful to check the patch, there's no denying that. My apologies if my previous message was confusing. -- Michael
Attachment
On 04.06.25 02:44, Michael Paquier wrote: > On Tue, Jun 03, 2025 at 09:31:17AM -0300, Matheus Alcantara wrote: >> I just wrote these tests to make it >> possible to reproduce the issue on my local machine and make the fix >> easier, and I just shared here so that folks can also have a way to >> reproduce the issue and maybe share other ideas to test this. > > +1. These are useful to check the patch, there's no denying that. My > apologies if my previous message was confusing. I have committed the patch without the additional tests, as discussed.
On Tue, Jun 3, 2025 at 9:44 PM Michael Paquier <michael@paquier.xyz> wrote: > > On Tue, Jun 03, 2025 at 09:31:17AM -0300, Matheus Alcantara wrote: > > I just wrote these tests to make it > > possible to reproduce the issue on my local machine and make the fix > > easier, and I just shared here so that folks can also have a way to > > reproduce the issue and maybe share other ideas to test this. > > +1. These are useful to check the patch, there's no denying that. My > apologies if my previous message was confusing. > No worries, thanks very much for the comments and taking time to review! -- Matheus Alcantara
On Wed, Jun 4, 2025 at 6:42 AM Peter Eisentraut <peter@eisentraut.org> wrote: > > On 04.06.25 02:44, Michael Paquier wrote: > > On Tue, Jun 03, 2025 at 09:31:17AM -0300, Matheus Alcantara wrote: > >> I just wrote these tests to make it > >> possible to reproduce the issue on my local machine and make the fix > >> easier, and I just shared here so that folks can also have a way to > >> reproduce the issue and maybe share other ideas to test this. > > > > +1. These are useful to check the patch, there's no denying that. My > > apologies if my previous message was confusing. > > I have committed the patch without the additional tests, as discussed. > Thanks! -- Matheus Alcantara