RFC: Additional Directory for Extensions

Hackers,

In the Security lessons from liblzma thread[1], walther broached the subject of an extension directory path[1]:

> Also a configurable directoy to look up extensions, possibly even to be
> changed at run-time like [2]. The patch says this:
>
>> This directory is prepended to paths when loading extensions (control and SQL files), and to the '$libdir' directive when loading modules that back functions. The location is made configurable to allow build-time testing of extensions that do not have been installed to their proper location yet.
>
> This seems like a great thing to have. This might also be relevant in
> light of recent discussions in the ecosystem around extension management.

That quotation comes from this Debian patch[2] maintained by Christoph Berg. I’d like to formally propose integrating this patch into the core. And not only because it’s overhead for package maintainers like Christoph, but because a number of use cases have emerged since we originally discussed something like this back in 2013[3]:

Docker Immutability
-------------------

Docker images must be immutable. In order for users of a Docker image to install extensions that persist, they must create a persistent volume, map it to SHAREDIR/extensions, and copy over all the core extensions (or muck with symlink magic[4]). This makes upgrades trickier, because the core extensions are mixed in with third party extensions.

By supporting a second directory pretended to the list of directories to search, as the Debian patch does, users of Docker images can keep extensions they install separate from core extensions, in a directory mounted to a persistent volume with none of the core extensions. Along with tweaking dynamic_library_path to support additional directories for shared object libraries, which can also be mounted to a separate path, we can have a persistent and clean separation of immutable core extensions and extensions installed at runtime.

Postgres.app
------------

The Postgres.app project also supports installing extensions. However, because they must go into the SHAREDIR/extensions, once a user installs one the package has been modified and the Apple bundle signature will be broken. The OS will no longer be able to validate that the app is legit.

If the core supported an additional extension (and PKGLIBDIR), it would allow an immutable PostgreSQL base package and still allow extensions to be installed into directories outside the app bundle, and thus preserve bundle signing on macOS (and presumably other systems --- is this the nix issue, too?)

RFC
---

I know there was some objection to changes like this in the past, but the support I’m seeing in the liblzma thread for making pkglibdir configurable me optimistic that this might be the right time to support additional configuration for the extension directory, as well, starting with the Debian patch, perhaps.

Thoughts?

I would be happy to submit a clean version of the Debian patch[2].

Best,

David

[1] https://www.postgresql.org/message-id/99c41b46-616e-49d0-9ffd-a29432cec818%40technowledgy.de
[2] https://salsa.debian.org/postgresql/postgresql/-/blob/17/debian/patches/extension_destdir?ref_type=heads
[3] https://www.postgresql.org/message-id/flat/51AE0845(dot)8010600(at)ocharles(dot)org(dot)uk
[4] https://speakerdeck.com/ongres/postgres-extensions-in-kubernetes?slide=14

On 2024-Apr-02, David E. Wheeler wrote:

> That quotation comes from this Debian patch[2] maintained by Christoph
> Berg. I’d like to formally propose integrating this patch into the
> core. And not only because it’s overhead for package maintainers like
> Christoph, but because a number of use cases have emerged since we
> originally discussed something like this back in 2013[3]:

I support the idea of there being a second location from where to load
shared libraries ... but I don't like the idea of making it
runtime-configurable. If we want to continue to tighten up what
superuser can do, then one of the things that has to go away is the
ability to load shared libraries from arbitrary locations
(dynamic_library_path). I think we should instead look at making those
locations hardcoded at compile time. The packager can then decide where
those things go, and the superuser no longer has the ability to load
arbitrary code from arbitrary locations.

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
Al principio era UNIX, y UNIX habló y dijo: "Hello world\n".
No dijo "Hello New Jersey\n", ni "Hello USA\n".

Alvaro Herrera:
> I support the idea of there being a second location from where to load
> shared libraries ... but I don't like the idea of making it
> runtime-configurable. If we want to continue to tighten up what
> superuser can do, then one of the things that has to go away is the
> ability to load shared libraries from arbitrary locations
> (dynamic_library_path). I think we should instead look at making those
> locations hardcoded at compile time. The packager can then decide where
> those things go, and the superuser no longer has the ability to load
> arbitrary code from arbitrary locations.

The use-case for runtime configuration of this seems to be build-time
testing of extensions against an already installed server. For this
purpose it should be enough to be able to set this directory at startup
- it doesn't need to be changed while the server is actually running.
Then you could spin up a temporary postgres instance with the extension
directory pointing a the build directory and test.

Would startup-configurable be better than runtime-configurable regarding
your concerns?

I can also imagine that it would be very helpful in a container setup to
be able to set an environment variable with this path instead of having
to recompile all of postgres to change it.

Best,

Wolfgang

> On 3 Apr 2024, at 09:13, Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
>
> On 2024-Apr-02, David E. Wheeler wrote:
>
>> That quotation comes from this Debian patch[2] maintained by Christoph
>> Berg. I’d like to formally propose integrating this patch into the
>> core. And not only because it’s overhead for package maintainers like
>> Christoph, but because a number of use cases have emerged since we
>> originally discussed something like this back in 2013[3]:
>
> I support the idea of there being a second location from where to load
> shared libraries

Agreed, the case made upthread that installing an extension breaks the app
signing seems like a compelling reason to do this.

The implementation of this need to make sure the directory is properly set up
however to avoid similar problems that CVE 2019-10211 showed.

--
Daniel Gustafsson

On Apr 3, 2024, at 3:57 AM, walther(at)technowledgy(dot)de wrote:

> I can also imagine that it would be very helpful in a container setup to be able to set an environment variable with this path instead of having to recompile all of postgres to change it.

Yes, I like the suggestion to make it require a restart, which lets the sysadmin control it and not limited to whatever the person who compiled it thought would make sense.

Best,

David

On Apr 3, 2024, at 8:54 AM, David E. Wheeler <david(at)justatheory(dot)com> wrote:

> Yes, I like the suggestion to make it require a restart, which lets the sysadmin control it and not limited to whatever the person who compiled it thought would make sense.

Or SIGHUP?

D

On Apr 3, 2024, at 8:54 AM, David E. Wheeler <david(at)justatheory(dot)com> wrote:

> Yes, I like the suggestion to make it require a restart, which lets the sysadmin control it and not limited to whatever the person who compiled it thought would make sense.

Here’s a revision of the Debian patch that requires a server start.

However, in studying the patch, it appears that the `extension_directory` is searched for *all* shared libraries, not just those being loaded for an extension. Am I reading the `expand_dynamic_library_name()` function right?

If so, this seems like a good way for a bad actor to muck with things, by putting an exploited libpgtypes library into the extension directory, where it would be loaded in preference to the core libpgtypes library, if they couldn’t exploit the original.

I’m thinking it would be better to have the dynamic library lookup for extension libraries (and LOAD libraries?) separate, so that the `extension_directory` would not be used for core libraries.

This would also allow the lookup of extension libraries prefixed by the directory field from the control file, which would enable much tidier extension installation: The control file, SQL scripts, and DSOs could all be in a single directory for an extension.

Thoughts?

Best,

David

Re: David E. Wheeler
> > Yes, I like the suggestion to make it require a restart, which lets the sysadmin control it and not limited to whatever the person who compiled it thought would make sense.
>
> Here’s a revision of the Debian patch that requires a server start.

Thanks for bringing this up, I should have submitted this years ago.
(The patch is originally from September 2020.)

I designed the patch to require a superuser to set it, so it doesn't
matter very much by which mechanism it gets updated. There should be
little reason to vary it at run-time, so I'd be fine with requiring a
restart, but otoh, why restrict the superuser from reloading it if
they know what they are doing?

> However, in studying the patch, it appears that the `extension_directory` is searched for *all* shared libraries, not just those being loaded for an extension. Am I reading the `expand_dynamic_library_name()` function right?
>
> If so, this seems like a good way for a bad actor to muck with things, by putting an exploited libpgtypes library into the extension directory, where it would be loaded in preference to the core libpgtypes library, if they couldn’t exploit the original.
>
> I’m thinking it would be better to have the dynamic library lookup for extension libraries (and LOAD libraries?) separate, so that the `extension_directory` would not be used for core libraries.

I'm not sure the concept of "core libraries" exists. PG happens to
dlopen things at run time, and it doesn't know/care if they were
installed by users or by the original PG server. Also, an exploited
libpgtypes library is not worse than any other untrusted "user"
library, so you really don't want to allow users to provide their own
.so files, no matter by what mechanism.

> This would also allow the lookup of extension libraries prefixed by the directory field from the control file, which would enable much tidier extension installation: The control file, SQL scripts, and DSOs could all be in a single directory for an extension.

Nice idea, but that would mean installing .so files into PGSHAREDIR.
Perhaps the whole extension stuff would have to move to PKGLIBDIR
instead.

Fwiw, I wrote this patch to solve the problem of testing extensions at
build-time where the build process does not have write access to
PGSHAREDIR. It solves that problem quite well, almost all PG
extensions have build-time test coverage now (where there was
basically 0 before).

Security is not a concern at this point as everything is running as
the same user, and the test cluster will be wiped right after the
test. I figured marking the setting as "super user" only was enough
security at that point, but I would recommend another audit before
using it together with "trusted" extensions and other things in
production.

Christoph

> Fwiw, I wrote this patch to solve the problem of testing extensions at
> build-time where the build process does not have write access to
> PGSHAREDIR. It solves that problem quite well, almost all PG
> extensions have build-time test coverage now (where there was
> basically 0 before).

Also, it's called extension "destdir" because it behaves like DESTDIR
in Makefiles: It prepends the given path to the path that PG is trying
to open when set. So it doesn't allow arbitrary new locations as of
now, just /home/build/foo-1/debian/foo/usr/share/postgresql/17/extension
in addition to /usr/share/postgresql/17/extension. (That is what the
Debian package build process needs, so that restriction/design choice
made sense.)

> Security is not a concern at this point as everything is running as
> the same user, and the test cluster will be wiped right after the
> test. I figured marking the setting as "super user" only was enough
> security at that point, but I would recommend another audit before
> using it together with "trusted" extensions and other things in
> production.

That's also included in the current GUC description:

This directory is prepended to paths when loading extensions
(control and SQL files), and to the '$libdir' directive when
loading modules that back functions. The location is made
configurable to allow build-time testing of extensions that do not
have been installed to their proper location yet.

Perhaps I should have included a more verbose "NOT FOR PRODUCTION"
there.

As for compatibility, the patch has been part of the PG 9.5..17 now
for several years, and I'm very happy with extra test coverage it
provides, especially on the Debian architectures that don't have
"autopkgtest" runners yet.

Christoph

On Apr 3, 2024, at 12:46 PM, Christoph Berg <myon(at)debian(dot)org> wrote:

> Thanks for bringing this up, I should have submitted this years ago.
> (The patch is originally from September 2020.)

That’s okay, it’s still 2020 in some ways. 😂

> I designed the patch to require a superuser to set it, so it doesn't
> matter very much by which mechanism it gets updated. There should be
> little reason to vary it at run-time, so I'd be fine with requiring a
> restart, but otoh, why restrict the superuser from reloading it if
> they know what they are doing?

I think that’s fair. I’ll keep it requiring a restart now, on the theory it would be easier to loosen it later than have to tighten it later.

> I'm not sure the concept of "core libraries" exists. PG happens to
> dlopen things at run time, and it doesn't know/care if they were
> installed by users or by the original PG server. Also, an exploited
> libpgtypes library is not worse than any other untrusted "user"
> library, so you really don't want to allow users to provide their own
> .so files, no matter by what mechanism.

Yes, I guess my concern is whether it could be used to “shadow” core libraries. Maybe it’s no different, really.

>> This would also allow the lookup of extension libraries prefixed by the directory field from the control file, which would enable much tidier extension installation: The control file, SQL scripts, and DSOs could all be in a single directory for an extension.
>
> Nice idea, but that would mean installing .so files into PGSHAREDIR.
> Perhaps the whole extension stuff would have to move to PKGLIBDIR
> instead.

Yes, I was just poking around the code, and realized that, when extension functions are created they may or may not not use `MODULE_PATHNAME`, but in any event, there is nothing different about loading an extension DSO than any other DSO. I was hoping to find a path where it knows it’s opening a DSO for the purpose of an extension, so we could limit the lookup there. But that does not (currently) exist.

Maybe we could add an `$extensiondir` variable to complement `$libdir`?

Or is PGKLIBDIR is the way to go? I’m not familiar with it. It looks like extension JIT files are put there already.

> Fwiw, I wrote this patch to solve the problem of testing extensions at
> build-time where the build process does not have write access to
> PGSHAREDIR. It solves that problem quite well, almost all PG
> extensions have build-time test coverage now (where there was
> basically 0 before).

Yeah, good additional use case.

On Apr 3, 2024, at 1:03 PM, Christoph Berg <myon(at)debian(dot)org> wrote:

> Also, it's called extension "destdir" because it behaves like DESTDIR
> in Makefiles: It prepends the given path to the path that PG is trying
> to open when set. So it doesn't allow arbitrary new locations as of
> now, just /home/build/foo-1/debian/foo/usr/share/postgresql/17/extension
> in addition to /usr/share/postgresql/17/extension. (That is what the
> Debian package build process needs, so that restriction/design choice
> made sense.

Right, this makes perfect sense, in that you don’t have to copy all the extension files from the destdir to the SHAREDIR to test them, which I imagine could be a PITA.

> That's also included in the current GUC description:
>
> This directory is prepended to paths when loading extensions
> (control and SQL files), and to the '$libdir' directive when
> loading modules that back functions. The location is made
> configurable to allow build-time testing of extensions that do not
> have been installed to their proper location yet.
>
> Perhaps I should have included a more verbose "NOT FOR PRODUCTION"
> there.

The use cases I described upthread are very much production use cases. Do you think it’s not for production just because we need to really think it through?

I’ve added some docs based on your GUC description; updated patch attached.

Best,

David

On Apr 4, 2024, at 1:20 PM, David E. Wheeler <david(at)justatheory(dot)com> wrote:

> I’ve added some docs based on your GUC description; updated patch attached.

Here’s a rebase.

I realize this probably isn’t going to happen for 17, given the freeze, but I would very much welcome feedback and pointers to address concerns about providing a second directory for extensions and DSOs. Quite a few people have talked about the need for this in the Extension Mini Summits[1], so I’m sure I could get some collaborators to make improvements or look at a different approach.

Best,

David

[1] https://justatheory.com/2024/02/extension-ecosystem-summit/#extension-ecosystem-mini-summit

On Thu, Apr 11, 2024 at 01:52:26PM -0400, David E. Wheeler wrote:
> I realize this probably isn´t going to happen for 17, given the freeze,
> but I would very much welcome feedback and pointers to address concerns
> about providing a second directory for extensions and DSOs. Quite a few
> people have talked about the need for this in the Extension Mini
> Summits[1], so I´m sure I could get some collaborators to make
> improvements or look at a different approach.

At first glance, the general idea seems reasonable to me. I'm wondering
whether there is a requirement for this directory to be prepended or if it
could be appended to the end. That way, the existing ones would take
priority, which might be desirable from a security standpoint.

--
nathan

On Wed, Apr 3, 2024 at 3:13 AM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
> I support the idea of there being a second location from where to load
> shared libraries ... but I don't like the idea of making it
> runtime-configurable. If we want to continue to tighten up what
> superuser can do, then one of the things that has to go away is the
> ability to load shared libraries from arbitrary locations
> (dynamic_library_path). I think we should instead look at making those
> locations hardcoded at compile time. The packager can then decide where
> those things go, and the superuser no longer has the ability to load
> arbitrary code from arbitrary locations.

Is "tighten up what the superuser can do" on our list of objectives?
Personally, I think we should be focusing mostly, and maybe entirely,
on letting non-superusers do more things, with appropriate security
controls. The superuser can ultimately do anything, since they can
cause shell commands to be run and load arbitrary code into the
backend and write code in untrusted procedural languages and mutilate
the system catalogs and lots of other terrible things.

Now, I think there are environments where people have used things like
containers to try to lock down the superuser, and while I'm not sure
that can ever be particularly water-tight, if it were the case that
this patch would make it a whole lot easier for a superuser to bypass
the kinds of controls that people are imposing today, that might be an
argument against this patch. But ... off-hand, I'm not seeing such an
exposure.

On the patch itself, I find the documentation for this to be fairly
hard to understand. I think it could benefit from an example. I'm
confused about whether this is intended to let me search for
extensions in /my/temp/root/usr/lib/postgresql/... by setting
extension_directory=/my/temp/dir, or whether it's intended me to
search both /usr/lib/postgresql as I normally would and also
/some/other/place. If the latter, I wonder why we don't handle shared
libraries by setting dynamic_library_path and then just have an
analogue of that for control files.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Jun 24, 2024, at 1:53 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> Is "tighten up what the superuser can do" on our list of objectives?
> Personally, I think we should be focusing mostly, and maybe entirely,
> on letting non-superusers do more things, with appropriate security
> controls. The superuser can ultimately do anything, since they can
> cause shell commands to be run and load arbitrary code into the
> backend and write code in untrusted procedural languages and mutilate
> the system catalogs and lots of other terrible things.

I guess the question then is what security controls are appropriate for this feature, which after all tells the postmaster what directories to read files from. It feels a little outside the scope of a regular user to even be aware of the file system undergirding the service. But perhaps there’s a non-superuser role for whom it is appropriate?

> Now, I think there are environments where people have used things like
> containers to try to lock down the superuser, and while I'm not sure
> that can ever be particularly water-tight, if it were the case that
> this patch would make it a whole lot easier for a superuser to bypass
> the kinds of controls that people are imposing today, that might be an
> argument against this patch. But ... off-hand, I'm not seeing such an
> exposure.

Yeah I’m not even sure I follow. Containers are immutable, other than mutable mounted volumes --- which is one use case this patch is attempting to enable.

> On the patch itself, I find the documentation for this to be fairly
> hard to understand. I think it could benefit from an example. I'm
> confused about whether this is intended to let me search for
> extensions in /my/temp/root/usr/lib/postgresql/... by setting
> extension_directory=/my/temp/dir, or whether it's intended me to
> search both /usr/lib/postgresql as I normally would and also
> /some/other/place.

I sketched them quickly, so agree they can be better. Reading the code, I now see that it appears to be the former case. I’d like to advocate for the latter.

> If the latter, I wonder why we don't handle shared
> libraries by setting dynamic_library_path and then just have an
> analogue of that for control files.

The challenge is that it applies not just to shared object libraries and control files, but also extension SQL files and any other SHAREDIR files an extension might include. But also, I think it should support all the pg_config installation targets that extensions might use, including:

BINDIR
DOCDIR
HTMLDIR
PKGINCLUDEDIR
LOCALEDIR
MANDIR

I can imagine an extension wanting or needing to use any and all of these.

Best,

David

On Mon, Jun 24, 2024 at 3:37 PM David E. Wheeler <david(at)justatheory(dot)com> wrote:
> I guess the question then is what security controls are appropriate for this feature, which after all tells the postmaster what directories to read files from. It feels a little outside the scope of a regular user to even be aware of the file system undergirding the service. But perhaps there’s a non-superuser role for whom it is appropriate?

As long as the GUC is superuser-only, I'm not sure what else there is
to do here. The only question is whether there's some reason to
disallow this even from the superuser, but I'm not quite seeing such a
reason.

> > On the patch itself, I find the documentation for this to be fairly
> > hard to understand. I think it could benefit from an example. I'm
> > confused about whether this is intended to let me search for
> > extensions in /my/temp/root/usr/lib/postgresql/... by setting
> > extension_directory=/my/temp/dir, or whether it's intended me to
> > search both /usr/lib/postgresql as I normally would and also
> > /some/other/place.
>
> I sketched them quickly, so agree they can be better. Reading the code, I now see that it appears to be the former case. I’d like to advocate for the latter.

Sounds good.

> > If the latter, I wonder why we don't handle shared
> > libraries by setting dynamic_library_path and then just have an
> > analogue of that for control files.
>
> The challenge is that it applies not just to shared object libraries and control files, but also extension SQL files and any other SHAREDIR files an extension might include. But also, I think it should support all the pg_config installation targets that extensions might use, including:
>
> BINDIR
> DOCDIR
> HTMLDIR
> PKGINCLUDEDIR
> LOCALEDIR
> MANDIR
>
> I can imagine an extension wanting or needing to use any and all of these.

Are these really all relevant to backend code?

--
Robert Haas
EDB: http://www.enterprisedb.com

On Jun 24, 2024, at 4:28 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> As long as the GUC is superuser-only, I'm not sure what else there is
> to do here. The only question is whether there's some reason to
> disallow this even from the superuser, but I'm not quite seeing such a
> reason.

I can switch it back from requiring a restart to allowing a superuser to set it.

>> I sketched them quickly, so agree they can be better. Reading the code, I now see that it appears to be the former case. I’d like to advocate for the latter.
>
> Sounds good.

Yeah, though then I have a harder time deciding how it should work. pg_config’s paths are absolute. With your first example, we just use them exactly as they are, but prefix them with the destination directory. So if it’s set to `/my/temp/root/`, then files go into

/my/temp/root/$(pg_conifg --sharedir)
/my/temp/root/$(pg_conifg --pkglibdir)
/my/temp/root/$(pg_conifg --bindir)
# etc.

Which is exactly how RPM and Apt packages are built, but seems like an odd configuration for general use.

>> BINDIR
>> DOCDIR
>> HTMLDIR
>> PKGINCLUDEDIR
>> LOCALEDIR
>> MANDIR
>>
>> I can imagine an extension wanting or needing to use any and all of these.
>
> Are these really all relevant to backend code?

Oh I think so. Especially BINDIR; lots of extensions ship with binary applications. And most ship with docs, too (PGXS puts items listed in DOCS into DOCDIR). Some might also produce man pages (for their binaries), HTML docs, and other stuff. Maybe an FTE extension would include locale files?

I find it pretty easy to imagine use cases for all of them. So much so that I wrote an extension binary distribution RFC[1] and its POC[2] around them.

Best,

David

[1]: https://github.com/orgs/pgxn/discussions/2
[1]: https://justatheory.com/2024/06/trunk-poc/

On Thu, 11 Apr 2024 at 19:52, David E. Wheeler <david(at)justatheory(dot)com> wrote:
> I realize this probably isn’t going to happen for 17, given the freeze, but I would very much welcome feedback and pointers to address concerns about providing a second directory for extensions and DSOs. Quite a few people have talked about the need for this in the Extension Mini Summits[1], so I’m sure I could get some collaborators to make improvements or look at a different approach.

Overall +1 for the idea. We're running into this same limitation (only
a single place to put extension files) at Microsoft at the moment.

+ and to the '$libdir' directive when loading modules
+ that back functions.

I feel like this is a bit strange. Either its impact is too wide, or
it's not wide enough depending on your intent.

If you want to only change $libdir during CREATE EXTENSION (or ALTER
EXTENSION UPDATE), then why not just change it there. And really you'd
only want to change it when creating an extension from which the
control file is coming from extension_destdir.

However, I can also see a case for really always changing $libdir.
Because some extensions in shared_preload_libraries, might want to
trigger loading other libraries that they ship with dynamically. And
these libraries are probably also in extension_destdir.

On Mon, 24 Jun 2024 at 18:11, Nathan Bossart <nathandbossart(at)gmail(dot)com> wrote:
> At first glance, the general idea seems reasonable to me. I'm wondering
> whether there is a requirement for this directory to be prepended or if it
> could be appended to the end. That way, the existing ones would take
> priority, which might be desirable from a security standpoint.

Citus does ship with some override library for pgoutput to make
logical replication/CDC work correctly with sharded tables. Right now
using this override library requires changing dynamic_library_path. It
would be nice if that wasn't necessary. But this is obviously a small
thing. And I definitely agree that there's a security angle to this as
well, but honestly that seems rather small too. If an attacker can put
shared libraries into the extension_destdir, I'm pretty sure you've
lost already, no matter if extension_destdir is prepended or appended
to the existing $libdir.

On Jun 24, 2024, at 17:17, Jelte Fennema-Nio <postgres(at)jeltef(dot)nl> wrote:

> If you want to only change $libdir during CREATE EXTENSION (or ALTER
> EXTENSION UPDATE), then why not just change it there. And really you'd
> only want to change it when creating an extension from which the
> control file is coming from extension_destdir.

IIUC, the postmaster needs to load an extension on first use in every session unless it’s in shared_preload_libraries.

> However, I can also see a case for really always changing $libdir.
> Because some extensions in shared_preload_libraries, might want to
> trigger loading other libraries that they ship with dynamically. And
> these libraries are probably also in extension_destdir.

Right, it can be more than just the DSOs for the extension itself.

Best,

David

On Mon, 24 Jun 2024 at 22:42, David E. Wheeler <david(at)justatheory(dot)com> wrote:
> >> BINDIR
> >> DOCDIR
> >> HTMLDIR
> >> PKGINCLUDEDIR
> >> LOCALEDIR
> >> MANDIR
> >>
> >> I can imagine an extension wanting or needing to use any and all of these.
> >
> > Are these really all relevant to backend code?
>
> Oh I think so. Especially BINDIR; lots of extensions ship with binary applications. And most ship with docs, too (PGXS puts items listed in DOCS into DOCDIR). Some might also produce man pages (for their binaries), HTML docs, and other stuff. Maybe an FTE extension would include locale files?
>
> I find it pretty easy to imagine use cases for all of them. So much so that I wrote an extension binary distribution RFC[1] and its POC[2] around them.

Definitely agreed on BINDIR needing to be supported.

And while lots of extensions ship with docs, I expect this feature to
mostly be used in production environments to make deploying extensions
easier. And I'm not sure that many people care about deploying docs to
production (honestly lots of people would probably want to strip
them).

Still, for the sake of completeness it might make sense to support
this whole list in extension_destdir. (assuming it's easy to do)

Re: Nathan Bossart
> At first glance, the general idea seems reasonable to me. I'm wondering
> whether there is a requirement for this directory to be prepended or if it
> could be appended to the end. That way, the existing ones would take
> priority, which might be desirable from a security standpoint.

My use case for this is to test things at compile time (where I can't
write to /usr/share/postgresql/). If installed things would take
priority over the things that I'm trying to test, I'd be disappointed.

Christoph

On 2024-Jun-24, Robert Haas wrote:

> Is "tighten up what the superuser can do" on our list of objectives?
> Personally, I think we should be focusing mostly, and maybe entirely,
> on letting non-superusers do more things, with appropriate security
> controls. The superuser can ultimately do anything, since they can
> cause shell commands to be run and load arbitrary code into the
> backend and write code in untrusted procedural languages and mutilate
> the system catalogs and lots of other terrible things.

I don't agree that we should focus _solely_ on allowing non-superusers
to do more things. Sure, it's a good thing to do -- but we shouldn't
completely close the option of securing superuser itself. I think it's
not completely impossible to have a future where superuser is just so
within the database, i.e. that it can't escape to the operating system.
I'm sure that would be useful in many environments. On this list, many
people frequently make the argument that it is impossible to secure, but
I'm not convinced.

They can mutilate the system catalogs: yes, they can TRUNCATE pg_type.
So what? They've just destroyed their own ability to do anything else.
The real issue here is that they can edit pg_proc to cause SQL function
calls to call arbitrary code. But what if we limit functions so that
the C code that they can call is located in specific places that are
known to only contain secure code? This is easy: make sure the
OS-installation only contains safe code in $libdir.

I hear you say: ah, but they can modify dynamic_library_path, which is a
GUC, to load code from anywhere -- especially /tmp, where the newest
bitcoin-mining library was just written. This is true. I suggest, to
solve this problem, that we should make dynamic_library_path no longer a
GUC. It should be a setting that comes from a different origin, one
that even superuser cannot write to. Only the OS-installation can
modify that file; that way, superuser cannot load arbitrary code that
way.

This is where the new GUC setting being proposed in this thread rubs me
the wrong way: it's adding yet another avenue for this to be exploited.
I would like this new directory not to be a GUC either, just like
dynamic_library_path.

I hear you argue: ah, but they can use COPY to write a new file to
$libdir. Yes, they can, and I think that's foolish. We could have
another non-GUC setting which takes a list of directories where COPY can
write files into. Problem solved. Do people really need the ability to
write files on arbitrary locations?

Untrusted extensions: well, just don't have those in the OS-installation
and you'll be fine. I'm okay with saying that a superuser-restricted
system is incompatible with plpython.

archive_command and so on: we could disable these too. Nathan did some
work to implement those using dynamic libraries, so it shouldn't be too
much of a loss; anything that is done with a shell script can also be
done with a small library. Those libraries can be made safe.
If there are other ways to invoke shell commands from GUCs, let's add
the ability to use libraries for those too.

What other exploits do we know about? How can we close them?

Now, I'm not saying that this is an easy journey. But if we don't
start, we're not going to get there.

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"Doing what he did amounts to sticking his fingers under the hood of the
implementation; if he gets his fingers burnt, it's his problem." (Tom Lane)

On Tue, 25 Jun 2024 at 12:12, Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
> They can mutilate the system catalogs: yes, they can TRUNCATE pg_type.
> So what? They've just destroyed their own ability to do anything else.
> The real issue here is that they can edit pg_proc to cause SQL function
> calls to call arbitrary code. But what if we limit functions so that
> the C code that they can call is located in specific places that are
> known to only contain secure code? This is easy: make sure the
> OS-installation only contains safe code in $libdir.

I wouldn't call it "easy" but I totally agree that changing pg_proc is
the main security issue that we have no easy way to tackle.

> I hear you say: ah, but they can modify dynamic_library_path, which is a
> GUC, to load code from anywhere -- especially /tmp, where the newest
> bitcoin-mining library was just written. This is true. I suggest, to
> solve this problem, that we should make dynamic_library_path no longer a
> GUC. It should be a setting that comes from a different origin, one
> that even superuser cannot write to. Only the OS-installation can
> modify that file; that way, superuser cannot load arbitrary code that
> way.

I don't think that needs a whole new file. Making this GUC be
PGC_SIGHUP/PGC_POSTMASTER + GUC_DISALLOW_IN_AUTO_FILE should be
enough. Just like was done for the new allow_alter_system GUC in PG17.

> This is where the new GUC setting being proposed in this thread rubs me
> the wrong way: it's adding yet another avenue for this to be exploited.
> I would like this new directory not to be a GUC either, just like
> dynamic_library_path.

We can make it PGC_SIGHUP/PGC_POSTMASTER + GUC_DISALLOW_IN_AUTO_FILE
too, either now or in the future.

> Now, I'm not saying that this is an easy journey. But if we don't
> start, we're not going to get there.

Sure, but it sounds like you're suggesting that you want to "start" by
not adding new features that have equivalent security holes as the
ones that we already have. I don't think that is a very helpful way to
get to a better place. It seems much more useful to tackle the current
problems that we have first, and almost certainly the same solutions
to those problems can be applied to any new features with security
issues.

It at least definitely seems the case for the proposal in this thread:
i.e. we already have a GUC that allows loading libraries from an
arbitrary location. This proposal adds another such GUC. If we solve
the security problem in that first GUC, either by
GUC_DISALLOW_IN_AUTO_FILE, or by creating a whole new mechanism for
the setting, then I see no reason why we cannot use that exact same
solution for the newly proposed GUC. So the required work to secure
postgres will not be meaningfully harder by adding this GUC.

On Tue, Jun 25, 2024 at 6:12 AM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
> Now, I'm not saying that this is an easy journey. But if we don't
> start, we're not going to get there.

I actually kind of agree with you. I think I've said something similar
in a previous email to the list somewhere. But I don't agree that this
patch should be burdened with taking the first step. We seem to often
find reasons why patches that packagers for prominent distributions
are carrying shouldn't be put into core, and I think that's a bad
habit. They're not going to stop applying those packages because we
refuse to put suitable functionality in core; we're just creating a
situation where lots of people are running slightly patched versions
of PostgreSQL instead of straight-up PostgreSQL. That's not improving
anything. If we want to work on making the sorts of changes that
you're proposing, let's do it on a separate thread. It's not going to
be meaningfully harder to move in that direction after some patch like
this than it is today.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Jun 24, 2024, at 5:32 PM, Jelte Fennema-Nio <postgres(at)jeltef(dot)nl> wrote:

> Still, for the sake of completeness it might make sense to support
> this whole list in extension_destdir. (assuming it's easy to do)

It should be with the current patch, which just uses a prefix to paths in `pg_config`. So if SHAREDIR is set to /usr/share/postgresql/16 and extension_destdir is set to /mount/ext, then Postgres will look for files in /mount/ext/usr/share/postgresql/16. The same rule applies (or should apply) for all other pg_config directory configs and where the postmaster looks for specific files. And PGXS already supports installing files in these locations, thanks to its DESTDIR param.

(I don’t know how it works on Windows, though.)

That said, this is very much a pattern designed for RPM and Debian package management patterns, and not for actually installing and managing extensions. And maybe that’s fine for now, as it can still be used to address the immutability problems descried in the original post in this thread.

Ultimately, I’d like to figure out a way to more tidily organize installed extension files, but I think that, too, might be a separate thread.

Best,

David

On Jun 25, 2024, at 10:43 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> If we want to work on making the sorts of changes that
> you're proposing, let's do it on a separate thread. It's not going to
> be meaningfully harder to move in that direction after some patch like
> this than it is today.

I appreciate this separation of concerns, Robert.

In other news, here’s an updated patch that expands the documentation to record that the destination directory is a prefix, and full paths should be used under it. Also take the opportunity to document the PGXS DESTDIR variable as the thing to use to install files under the destination directory.

It still requires a server restart; I can change it back to superuser-only if that’s the consensus.

For those who prefer a GitHub patch review experience, see this PR:

https://github.com/theory/postgres/pull/3/files

Best,

David

On Wed, 26 Jun 2024 at 00:32, David E. Wheeler <david(at)justatheory(dot)com> wrote:
> In other news, here’s an updated patch that expands the documentation to record that the destination directory is a prefix, and full paths should be used under it. Also take the opportunity to document the PGXS DESTDIR variable as the thing to use to install files under the destination directory.

Docs are much clearer now thanks.

full = substitute_libpath_macro(name);
+ /*
+ * If extension_destdir is set, try to find the file there first
+ */
+ if (*extension_destdir != '\0')
+ {
+ full2 = psprintf("%s%s", extension_destdir, full);
+ if (pg_file_exists(full2))
+ {
+ pfree(full);
+ return full2;
+ }
+ pfree(full2);
+ }

I think this should be done differently. For two reasons:
1. I don't think extension_destdir should be searched when $libdir is
not part of the name.
2. find_in_dynamic_libpath currently doesn't use extension_destdir at
all, so if there is no slash in the filename we do not search
extension_destdir.

I feel like changing the substitute_libpath_macro function a bit (or
adding a new similar function) is probably the best way to achieve
that.

We should also check somewhere (probably GUC check hook) that
extension_destdir is an absolute path.

> It still requires a server restart;

When reading the code I see no reason why this cannot be PGC_SIGHUP.
Even though it's probably not needed to change on a running server, I
think it's better to allow that. Even just so people can disable it if
necessary for some reason without restarting the process.

> I can change it back to superuser-only if that’s the consensus.

It still is GUC_SUPERUSER_ONLY, right?

> For those who prefer a GitHub patch review experience, see this PR:
>
> https://github.com/theory/postgres/pull/3/files

Sidenote: The "D" link for each patch on cfbot[1] now gives a similar
link for all commitfest entries[2].

[1]: http://cfbot.cputube.org/
[2]: https://github.com/postgresql-cfbot/postgresql/compare/cf/4913~1...cf/4913

On Tue, 25 Jun 2024 at 19:33, David E. Wheeler <david(at)justatheory(dot)com> wrote:
>
> On Jun 24, 2024, at 5:32 PM, Jelte Fennema-Nio <postgres(at)jeltef(dot)nl> wrote:
>
> > Still, for the sake of completeness it might make sense to support
> > this whole list in extension_destdir. (assuming it's easy to do)
>
> It should be with the current patch, which just uses a prefix to paths in `pg_config`.

Ah alright, I think it confused me because I never saw bindir being
used. But as it turns out the current backend code never uses bindir.
So that makes sense. I guess to actually use the binaries from the
extension_destdir/$BINDIR the operator needs to set PATH accordingly,
or the extension needs to be changed to support extension_destdir.

It might be nice to add a helper function to find binaries in BINDIR,
now that the resolution logic is more complex. Even if postgres itself
doesn't use it. That would make it easier for extensions to be
modified to support extension_distdir. Something like
find_bindir_executable(char *name)

On Jun 25, 2024, at 18:31, David E. Wheeler <david(at)justatheory(dot)com> wrote:

> For those who prefer a GitHub patch review experience, see this PR:
>
> https://github.com/theory/postgres/pull/3/files

Rebased and restored PGC_SUSET in the attached v5 patch, plus noted the required privileges in the docs.

Best,

David

Hi everyone,

Apologies for only starting to look into this now. Thanks, David, for
pushing this forward.

I want to emphasize the importance of this patch for the broader adoption
of extensions in immutable container environments, such as those used by
the CloudNativePG operator in Kubernetes.

To provide some context, one of the key principles of CloudNativePG is that
containers, once started, cannot be modified—this includes the installation
of Postgres extensions and their libraries. This restriction prevents us
from adding extensions on the fly, requiring them to be included in the
main PostgreSQL operand image. As a result, users who need specific
extensions must build custom images through automated pipelines (see:
https://cloudnative-pg.io/blog/creating-container-images/)

We’ve been considering ways to improve this process for some time. The
direction we're exploring involves mounting an ephemeral volume that
contains the necessary extensions (namely $sharedir and $pkglibdir from
pg_config). These volumes would be created and populated with the required
extensions when the container starts and destroyed when it shuts down. To
make this work, each extension must be independently packaged as a
container image containing the appropriate files for a specific extension
version, tailored to the architecture, distribution, OS version, and
Postgres version.

I’m committed to thoroughly reviewing this patch, testing it with
CloudNativePG and a few extensions, and providing feedback as soon as
possible.

Best,
Gabriele

On Mon, 8 Jul 2024 at 18:02, David E. Wheeler <david(at)justatheory(dot)com> wrote:

> On Jun 25, 2024, at 18:31, David E. Wheeler <david(at)justatheory(dot)com> wrote:
>
> > For those who prefer a GitHub patch review experience, see this PR:
> >
> > https://github.com/theory/postgres/pull/3/files
>
> Rebased and restored PGC_SUSET in the attached v5 patch, plus noted the
> required privileges in the docs.
>
> Best,
>
> David
>
>
>
>

--
Gabriele Bartolini
Vice President, Cloud Native at EDB
enterprisedb.com

On Tue, Jun 25, 2024 at 12:12:33PM +0200, Alvaro Herrera wrote:
> archive_command and so on: we could disable these too. Nathan did some
> work to implement those using dynamic libraries, so it shouldn't be too
> much of a loss; anything that is done with a shell script can also be
> done with a small library. Those libraries can be made safe.
> If there are other ways to invoke shell commands from GUCs, let's add
> the ability to use libraries for those too.

Sorry, I just noticed this message. I recently withdrew my patch set [0]
for using a library instead of shell commands for restore_command,
archive_cleanup_command, and recovery_end_command, as it had sat idle for a
very long time. If/when there's interest, I'd be happy to pick it up
again.

[0] https://postgr.es/m/ZkwLqichtySV5kF3%40nathan-air.lan

--
nathan

On Thu, 22 Aug 2024 at 08:00, Gabriele Bartolini
<gabriele(dot)bartolini(at)enterprisedb(dot)com> wrote:
>
> Hi everyone,
>
> Apologies for only starting to look into this now. Thanks, David, for pushing this forward.

100%. I've wanted this for some time but never had time to cook up a patch.

> I want to emphasize the importance of this patch for the broader adoption of extensions in immutable container environments, such as those used by the CloudNativePG operator in Kubernetes.

It's also very relevant for local development and testing.

Right now postgres makes it impossible to locally compile and install
an extension for a distro-packaged postgres (whether from upstream
repos or PGDG repos) without dirtying the distro-managed filesystem
subtrees with local changes under /usr etc, because it cannot be
configured to look for locally installed extensions on non-default
paths.

> To provide some context, one of the key principles of CloudNativePG is that containers, once started, cannot be modified—this includes the installation of Postgres extensions and their libraries. This restriction prevents us from adding extensions on the fly, requiring them to be included in the main PostgreSQL operand image. As a result, users who need specific extensions must build custom images through automated pipelines (see: https://cloudnative-pg.io/blog/creating-container-images/)

It may be possible to weaken this restriction somewhat thanks to the
upcoming https://kubernetes.io/blog/2024/08/16/kubernetes-1-31-image-volume-source/
feature that permits additional OCI images to be mounted as read-only
volumes on a workload. This would still only permit mounting at
Pod-creation time, not runtime mounting and unmonuting, but means the
base postgres image could be supplemented by mounting additional
images for extensions.

For example, one might mount image "postgis-vX.Y.Z" image onto base
image "postgresql-16" if support for PostGIS is desired, without then
having to bake every possible extension anyone might ever want into
the base image. This solves all sorts of messy issues with upgrades
and new version releases.

But for it to work, it must be possible to tell postgres to look in
_multiple places_ for extension .sql scripts and control files. This
is presently possible for modules (dynamic libraries, .so / .dylib /
.dll) but without a way to also configure the path for extensions it's
of very limited utility.

> We’ve been considering ways to improve this process for some time. The direction we're exploring involves mounting an ephemeral volume that contains the necessary extensions (namely $sharedir and $pkglibdir from pg_config). These volumes would be created and populated with the required extensions when the container starts and destroyed when it shuts down. To make this work, each extension must be independently packaged as a container image containing the appropriate files for a specific extension version, tailored to the architecture, distribution, OS version, and Postgres version.

Right. And there might be more than one of them.

So IMO this should be a _path_ to search for extension control files
and SQL scripts.

If the current built-in default extension dir was exposed as a var
$extdir like we do for $libdir, this might look something like this
for local development and testing while working with a packaged
postgres build:

SET extension_search_path = $extsdir, /opt/myapp/extensions,
/usr/local/postgres/my-custom-extension/extensions;
SET dynamic_library_path = $libdir, /opt/myapp/lib,
/usr/local/postgres/my-custom-extension/lib

or in the container extensions case, something like:

SET extension_search_path = $extsdir,
/mnt/extensions/pg16/postgis-vX.Y/extensions,
/mnt/extensions/pg16/gosuperfast/extensions;
SET dynamic_library_path = $libdir,
/mnt/extensions/pg16/postgis-vX.Y/lib,
/mnt/extensions/pg16/gosuperfast/lib;

For safety, it might make sense to impose the restriction that if an
extension control file is found in a given directory, SQL scripts will
also only be looked for in that same directory. That way there's no
chance of accidentally mixing and matching SQL scripts from different
versions of an extension if it appears twice on the extension search
path in different places. The rule for loading SQL scripts would be:

* locate first directory on path contianing matching extension control file
* use this directory as the extension directory for all subsequent SQL
script loading and running actions

--
Craig Ringer
EnterpriseDB

On Thu, 22 Aug 2024 at 01:08, Craig Ringer
<craig(dot)ringer(at)enterprisedb(dot)com> wrote:
> SET extension_search_path = $extsdir,
> /mnt/extensions/pg16/postgis-vX.Y/extensions,
> /mnt/extensions/pg16/gosuperfast/extensions;

It looks like you want one directory per extension, so that list would
get pretty long if you have multiple extensions. Maybe (as a follow up
change), we should start to support a * as a wildcard in both of these
GUCs. So you could say:

SET extension_search_path = /mnt/extensions/pg16/*

To mean effectively the same as you're describing above.

Hi Craig,

On Thu, 22 Aug 2024 at 01:07, Craig Ringer <craig(dot)ringer(at)enterprisedb(dot)com>
wrote:

> It's also very relevant for local development and testing.
>

Yep, which is the original goal of Christoph IIRC.

> It may be possible to weaken this restriction somewhat thanks to the
> upcoming
> https://kubernetes.io/blog/2024/08/16/kubernetes-1-31-image-volume-source/
> feature that permits additional OCI images to be mounted as read-only
> volumes on a workload. This would still only permit mounting at
> Pod-creation time, not runtime mounting and unmonuting, but means the
> base postgres image could be supplemented by mounting additional
> images for extensions.
>

I'm really excited about that feature, but it's still in the alpha stage.
However, I don't anticipate any issues for the future general availability
(GA) release. Regardless, we may need to consider a temporary solution that
is compatible with existing Kubernetes and possibly Postgres versions (but
that's beyond the purpose of this thread).

For example, one might mount image "postgis-vX.Y.Z" image onto base
> image "postgresql-16" if support for PostGIS is desired, without then
> having to bake every possible extension anyone might ever want into
> the base image. This solves all sorts of messy issues with upgrades
> and new version releases.
>

Yep.

> But for it to work, it must be possible to tell postgres to look in
> _multiple places_ for extension .sql scripts and control files. This
> is presently possible for modules (dynamic libraries, .so / .dylib /
> .dll) but without a way to also configure the path for extensions it's
> of very limited utility.
>

Agree.

> So IMO this should be a _path_ to search for extension control files
> and SQL scripts.
>

I like this. I also prefer the name `extension_search_path`.

For safety, it might make sense to impose the restriction that if an
> extension control file is found in a given directory, SQL scripts will
> also only be looked for in that same directory. That way there's no
> chance of accidentally mixing and matching SQL scripts from different
> versions of an extension if it appears twice on the extension search
> path in different places. The rule for loading SQL scripts would be:
>
> * locate first directory on path contianing matching extension control file
> * use this directory as the extension directory for all subsequent SQL
> script loading and running actions
>

It could work, but it requires some prototyping and exploration. I'm
willing to participate and use CloudNativePG as a test bed.

Cheers,
Gabriele
--
Gabriele Bartolini
Vice President, Cloud Native at EDB
enterprisedb.com

Hi Jelte,

On Thu, 22 Aug 2024 at 09:32, Jelte Fennema-Nio <postgres(at)jeltef(dot)nl> wrote:

> It looks like you want one directory per extension, so that list would
> get pretty long if you have multiple extensions. Maybe (as a follow up
> change), we should start to support a * as a wildcard in both of these
> GUCs. So you could say:
>
> SET extension_search_path = /mnt/extensions/pg16/*
>
> To mean effectively the same as you're describing above.
>

That'd be great. +1.

--
Gabriele Bartolini
Vice President, Cloud Native at EDB
enterprisedb.com

On Thu, 22 Aug 2024 at 21:00, Gabriele Bartolini
<gabriele(dot)bartolini(at)enterprisedb(dot)com> wrote:
> On Thu, 22 Aug 2024 at 09:32, Jelte Fennema-Nio <postgres(at)jeltef(dot)nl> wrote:
>> SET extension_search_path = /mnt/extensions/pg16/*
>
> That'd be great. +1.

Agreed, that'd be handy, but not worth blocking the underlying capability for.

Except possibly to the degree that the feature should reserve wildcard
characters and require them to be escaped if they appear on a path, so
there's no BC break if it's added later.

On Thu, 22 Aug 2024 at 21:00, Gabriele Bartolini
<gabriele(dot)bartolini(at)enterprisedb(dot)com> wrote:
>
> Hi Jelte,
>
> On Thu, 22 Aug 2024 at 09:32, Jelte Fennema-Nio <postgres(at)jeltef(dot)nl> wrote:
>>
>> It looks like you want one directory per extension, so that list would
>> get pretty long if you have multiple extensions. Maybe (as a follow up
>> change), we should start to support a * as a wildcard in both of these
>> GUCs. So you could say:
>>
>> SET extension_search_path = /mnt/extensions/pg16/*
>>
>> To mean effectively the same as you're describing above.
>
>
> That'd be great. +1.
>
> --
> Gabriele Bartolini
> Vice President, Cloud Native at EDB
> enterprisedb.com

On Fri, 23 Aug 2024 at 10:14, Craig Ringer
<craig(dot)ringer(at)enterprisedb(dot)com> wrote:
> On Thu, 22 Aug 2024 at 21:00, Gabriele Bartolini
> <gabriele(dot)bartolini(at)enterprisedb(dot)com> wrote:
> > On Thu, 22 Aug 2024 at 09:32, Jelte Fennema-Nio <postgres(at)jeltef(dot)nl> wrote:
> >> SET extension_search_path = /mnt/extensions/pg16/*
> >
> > That'd be great. +1.
>
> Agreed, that'd be handy, but not worth blocking the underlying capability for.
>
> Except possibly to the degree that the feature should reserve wildcard
> characters and require them to be escaped if they appear on a path, so
> there's no BC break if it's added later.

... though on second thoughts, it might make more sense to just
recursively search directories found under each path entry. Rules like
'search if a redundant trailing / is present' can be an option.

That way there's no icky path escaping needed for normal configuration.

Hi Hackers,

Apologies for the delay in reply; I’ve been at the XOXO Festival and almost completely unplugged for the first time in ages. Happy to see this thread coming alive, though. Thank you Gabriele, Craig, and Jelte!

On Aug 21, 2024, at 19:07, Craig Ringer <craig(dot)ringer(at)enterprisedb(dot)com> wrote:

> So IMO this should be a _path_ to search for extension control files
> and SQL scripts.
>
> If the current built-in default extension dir was exposed as a var
> $extdir like we do for $libdir, this might look something like this
> for local development and testing while working with a packaged
> postgres build:
>
> SET extension_search_path = $extsdir, /opt/myapp/extensions,
> /usr/local/postgres/my-custom-extension/extensions;
> SET dynamic_library_path = $libdir, /opt/myapp/lib,
> /usr/local/postgres/my-custom-extension/lib

I would very much like something like this, but I’m not sure how feasible it is for a few reasons. The first, and most important, is that extensions are not limited to just a control file and SQL file. They also very often include:

* one or more shared library files
* documentation files
* binary files

And maybe more? How many of these directories might an extension install files into:

✦ ❯ pg_config | grep DIR | awk '{print $1}'
BINDIR
DOCDIR
HTMLDIR
INCLUDEDIR
PKGINCLUDEDIR
INCLUDEDIR-SERVER
LIBDIR
PKGLIBDIR
LOCALEDIR
MANDIR
SHAREDIR
SYSCONFDIR

I would assume BINDIR, DOCDIR, HTMLDIR, PKGLIBDIR, MANDIR, SHAREDIR, and perhaps LOCALEDIR.

But even if it’s just one or two, the only proper way an extension directory would work, IME, is to define a directory-based structure for extensions, where every file for an extension is in a directory named for the extension, and subdirectories are defined for each of the above requisite file types. Something like:

extension_name
├── control.ini
├── bin
├── doc
├── html
├── lib
├── local
├── man
└── share

This would allow multiple paths to work and keep all the files for an extension bundled together. It could also potentially allow for multiple versions of an extension to be installed at once, if we required the version to be part of the directory name.

I think this would be a much nicer layout for packaging, installing, and managing extensions versus the current method of strewing files around to a slew of different directories. But it would come at some cost, in terms of backward with the existing layout (or migration to it), significant modification of the server to use the new layout (and extension_search_path), and other annoyances like PATH and MANPATH management.

Long term I think it would be worthwhile, but the current patch feels like a decent interim step we could live with, solving most of the integration problems (immutable servers, packaging testing, etc.) at the cost of a slightly unexpected directory layout. What I mean by that is that the current patch is pretty much just using extension_destdir as a prefix to all of those directories from pg_config, so they never have to change, but it does mean that you end up installing extensions in something like

/mnt/extensions/pg16/usr/share/postgresql/16
/mnt/extensions/pg16/usr/include/postgresql

etc.

Best,

David

On Tue, 27 Aug 2024 at 02:07, David E. Wheeler <david(at)justatheory(dot)com> wrote:
> On Aug 21, 2024, at 19:07, Craig Ringer <craig(dot)ringer(at)enterprisedb(dot)com> wrote:

> But even if it’s just one or two, the only proper way an extension directory would work, IME, is to define a directory-based structure for extensions, where every file for an extension is in a directory named for the extension, and subdirectories are defined for each of the above requisite file types.
> [...]
> I think this would be a much nicer layout for packaging, installing, and managing extensions versus the current method of strewing files around to a slew of different directories.

This looks like a good suggestion to me, it would make the packaging,
distribution and integration of 3rd party extensions significantly
easier without any obvious large or long term cost.

> But it would come at some cost, in terms of backward with the existing layout (or migration to it), significant modification of the server to use the new layout (and extension_search_path), and other annoyances like PATH and MANPATH management.

Also PGXS, the windows extension build support, and 3rd party cmake
builds etc. But not by the looks a drastic change.

> Long term I think it would be worthwhile, but the current patch feels like a decent interim step we could live with, solving most of the integration problems (immutable servers, packaging testing, etc.) at the cost of a slightly unexpected directory layout. What I mean by that is that the current patch is pretty much just using extension_destdir as a prefix to all of those directories from pg_config, so they never have to change, but it does mean that you end up installing extensions in something like:
>
> /mnt/extensions/pg16/usr/share/postgresql/16
> /mnt/extensions/pg16/usr/include/postgresql

My only real concern with the current patch is that it limits
searching for extensions to one additional configurable location,
which is inconsistent with how things like the dynamic_library_path
works. Once in, it'll be difficult to change or extend for BC, and if
someone wants to add a search path capability it'll break existing
configurations.

Would it be feasible to define its configuration syntax as accepting a
list of paths, but only implement the semantics for single-entry lists
and ERROR on multiple paths? That way it could be extended w/o
breaking existing configurations later.

With that said, I'm not the one doing the work at the moment, and the
functionality would definitely be helpful. If there's agreement on
supporting a search-path or recursing into subdirectories I'd be
willing to have a go at it, but I'm a bit stale on Pg's codebase now
so I'd want to be fairly confident the work wouldn't just be thrown
out.

--
Craig Ringer

Hi David,

Thanks for your email.

On Mon, 26 Aug 2024 at 16:07, David E. Wheeler <david(at)justatheory(dot)com>
wrote:

> I would assume BINDIR, DOCDIR, HTMLDIR, PKGLIBDIR, MANDIR, SHAREDIR, and
> perhaps LOCALEDIR.
>
> But even if it’s just one or two, the only proper way an extension
> directory would work, IME, is to define a directory-based structure for
> extensions, where every file for an extension is in a directory named for
> the extension, and subdirectories are defined for each of the above
> requisite file types. Something like:
>
> extension_name
> ├── control.ini
> ├── bin
> ├── doc
> ├── html
> ├── lib
> ├── local
> ├── man
> └── share
>

I'm really glad you proposed this publicly. I reached the same conclusion
the other day when digging deeper into the problem with a few folks from
CloudNativePG. Setting aside multi-arch images for now, if we could
reorganize the content of a single image (identified by OS distro,
PostgreSQL major version, and extension version) with a top-level directory
structure as you described, we could easily mount each image as a separate
volume.

The extension image could follow a naming convention like this (order can
be adjusted): `<extension name>-<pg major>-<extension
version>-<distro>(-<seq>)`. For example, `pgvector-16-0.7.4-bookworm-1`
would represent the first image built in a repository for pgvector 0.7.4
for PostgreSQL 16 on Debian Bookworm. If multi-arch images aren't desired,
we could incorporate the architecture somewhere in the naming convention.

This would allow multiple paths to work and keep all the files for an
> extension bundled together. It could also potentially allow for multiple
> versions of an extension to be installed at once, if we required the
> version to be part of the directory name.
>

If we wanted to install multiple versions of an extension, we could mount
them in different directories, with the version included in the folder
name—for example, `pgvector-0.7.4` instead of just `pgvector`. However, I'm
a bit rusty with the extensions framework, so I'll need to check if this
approach is feasible and makes sense.

Thanks,
Gabriele
--
Gabriele Bartolini
Vice President, Cloud Native at EDB
enterprisedb.com

On Aug 27, 2024, at 04:56, Gabriele Bartolini <gabriele(dot)bartolini(at)enterprisedb(dot)com> wrote:

> The extension image could follow a naming convention like this (order can be adjusted): `<extension name>-<pg major>-<extension version>-<distro>(-<seq>)`. For example, `pgvector-16-0.7.4-bookworm-1` would represent the first image built in a repository for pgvector 0.7.4 for PostgreSQL 16 on Debian Bookworm. If multi-arch images aren't desired, we could incorporate the architecture somewhere in the naming convention.

Well now you’re just describing the binary distribution format RFC[1] (POC[2]) and multi-platform OCI distribution POC[3] :-)

> If we wanted to install multiple versions of an extension, we could mount them in different directories, with the version included in the folder name—for example, `pgvector-0.7.4` instead of just `pgvector`. However, I'm a bit rusty with the extensions framework, so I'll need to check if this approach is feasible and makes sense.

Right, if we decided to adopt this proposal, it might make sense to include the “default version” as part of the directory name. But there’s quite a lot of work between here and there.

Best,

David

[1]: https://github.com/pgxn/rfcs/pull/2
[2]: https://justatheory.com/2024/06/trunk-poc/
[3]: https://justatheory.com/2024/06/trunk-oci-poc/

On Aug 26, 2024, at 17:35, Craig Ringer <craig(dot)ringer(at)enterprisedb(dot)com> wrote:

> This looks like a good suggestion to me, it would make the packaging,
> distribution and integration of 3rd party extensions significantly
> easier without any obvious large or long term cost.

Yes!

> Also PGXS, the windows extension build support, and 3rd party cmake
> builds etc. But not by the looks a drastic change.

Right. ISTM it could complicate PGXS quite a bit. If we set, say,

SET extension_search_path = $extsdir, /mnt/extensions/pg16, /mnt/extensions/pg16/gosuperfast/extensions;

What should be the output of `pg_config --sharedir`?

> My only real concern with the current patch is that it limits
> searching for extensions to one additional configurable location,
> which is inconsistent with how things like the dynamic_library_path
> works. Once in, it'll be difficult to change or extend for BC, and if
> someone wants to add a search path capability it'll break existing
> configurations.

Agreed.

> Would it be feasible to define its configuration syntax as accepting a
> list of paths, but only implement the semantics for single-entry lists
> and ERROR on multiple paths? That way it could be extended w/o
> breaking existing configurations later.

I imagine it’s a simple matter of programming :-) But that leaves the issue of directory organization. The current patch is just a prefix for various PGXS/pg_config directories; the longer-term proposal I’ve made here is not a prefix for sharedir, mandir, etc., but a directory that contains directories named for extensions. So even if we were to take this approach, the directory structure would vary.

I suspect we’d have to name it differently and support both long-term. That, too me, is the main issue with this patch.

OTOH, we have this patch now, and this other stuff is just a proposal. Actual code trumps ideas in my mind.

> With that said, I'm not the one doing the work at the moment, and the
> functionality would definitely be helpful. If there's agreement on
> supporting a search-path or recursing into subdirectories I'd be
> willing to have a go at it, but I'm a bit stale on Pg's codebase now
> so I'd want to be fairly confident the work wouldn't just be thrown
> out.

I think we should get some clarity on the proposal, and then consensus, as you say. I say “get some clarity” because my proposal doesn’t require recursing, and I’m not sure why it’d be needed.

Best,

David

On Wed, 28 Aug 2024 at 03:26, David E. Wheeler <david(at)justatheory(dot)com> wrote:
> Right. ISTM it could complicate PGXS quite a bit. If we set, say,
>
> SET extension_search_path = $extsdir, /mnt/extensions/pg16, /mnt/extensions/pg16/gosuperfast/extensions;
>
> What should be the output of `pg_config --sharedir`?

`pg_config` only cares about compile-time settings, so I would not
expect its output to change.

I suspect we'd have to add PGXS extension-path awareness if going for
per-extension subdir support. I'm not sure it makes sense to teach
`pg_config` about this, since it'd need to have a different mode like

pg_config --extension myextname --extension-sharedir

since the extension's "sharedir" is
$basedir/extensions/myextension/share or whatever.

Supporting this looks to be a bit intrusive in the makefiles,
requiring them to differentiate between "share dir for extensions" and
"share dir for !extensions", etc. I'm not immediately sure if it can
be done in a way that transparently converts unmodified extension PGXS
makefiles to target the new paths; it might require an additional
define, or use of new variables and an ifdef block to add
backwards-compat to the extension makefile for building on older
postgres.

> But that leaves the issue of directory organization. The current patch is just a prefix for various PGXS/pg_config directories; the longer-term proposal I’ve made here is not a prefix for sharedir, mandir, etc., but a directory that contains directories named for extensions. So even if we were to take this approach, the directory structure would vary.

Right. The proposed structure is rather a bigger change than I was
thinking when I suggested supporting an extension search path not just
a single extra path. But it's also a cleaner proposal; the
per-extension directory would make it easier to ensure that the
extension control file, sql scripts, and dynamic library all match the
same extension and version if multiple ones are on the path. Which is
desirable when doing things like testing a new version of an in-core
extension.

> OTOH, we have this patch now, and this other stuff is just a proposal. Actual code trumps ideas in my mind.

Right. And I've been on the receiving end of having a small, focused
patch derailed by others jumping in and scope-exploding it into
something completely different to solve a much wider but related
problem.

I'm definitely not trying to stand in the way of progress with this; I
just want to make sure that it doesn't paint us into a corner that
prevents a more general solution from being adopted later. That's why
I'm suggesting making the config a multi-value string (list of paths)
and raising a runtime "ERROR: searching multiple paths for extensions
not yet supported" or something if >1 path configured.

If that doesn't work, no problem.

> I think we should get some clarity on the proposal, and then consensus, as you say. I say “get some clarity” because my proposal doesn’t require recursing, and I’m not sure why it’d be needed.

From what you and Gabriele are discussing (which I agree with), it wouldn't.

On Aug 27, 2024, at 22:24, Craig Ringer <craig(dot)ringer(at)enterprisedb(dot)com> wrote:

> `pg_config` only cares about compile-time settings, so I would not
> expect its output to change.

Right, of course that’s its original purpose, but extensions depend on it to determine where to install extensions. Not just PGXS, but also pgrx and various Makefile customizations I’ve seen in the wild.

> I suspect we'd have to add PGXS extension-path awareness if going for
> per-extension subdir support. I'm not sure it makes sense to teach
> `pg_config` about this, since it'd need to have a different mode like
>
> pg_config --extension myextname --extension-sharedir
>
> since the extension's "sharedir" is
> $basedir/extensions/myextension/share or whatever.

Right. PGXS would just need to know where to put the directory for an extension. There should be a default for the project, and then it can be overridden with something like DESTDIR (but without full paths under that prefix).

> Supporting this looks to be a bit intrusive in the makefiles,
> requiring them to differentiate between "share dir for extensions" and
> "share dir for !extensions", etc. I'm not immediately sure if it can
> be done in a way that transparently converts unmodified extension PGXS
> makefiles to target the new paths; it might require an additional
> define, or use of new variables and an ifdef block to add
> backwards-compat to the extension makefile for building on older
> postgres.

Yeah, might just have to be an entirely new thing, though it sure would be nice for existing PGXS-using Makefiles to do the right thing. Maybe for the new version of the server with the proposed new pattern it would dispatch to the new thing somehow without modifying all the rest of its logic.

> Right. The proposed structure is rather a bigger change than I was
> thinking when I suggested supporting an extension search path not just
> a single extra path. But it's also a cleaner proposal; the
> per-extension directory would make it easier to ensure that the
> extension control file, sql scripts, and dynamic library all match the
> same extension and version if multiple ones are on the path. Which is
> desirable when doing things like testing a new version of an in-core
> extension.

💯

> Right. And I've been on the receiving end of having a small, focused
> patch derailed by others jumping in and scope-exploding it into
> something completely different to solve a much wider but related
> problem.

I’m not complaining, I would definitely prefer to see consensus on a cleaner proposal along the lines we’ve discussed and a commitment to time from parties able to get it done in time for v18. I’m willing to help where I can with my baby C! Failing that, we can fall back on the destdir patch.

> I'm definitely not trying to stand in the way of progress with this; I
> just want to make sure that it doesn't paint us into a corner that
> prevents a more general solution from being adopted later. That's why
> I'm suggesting making the config a multi-value string (list of paths)
> and raising a runtime "ERROR: searching multiple paths for extensions
> not yet supported" or something if >1 path configured.
>
> If that doesn't work, no problem.

I think the logic would have to be different, so they’d be different GUCs with their own semantics. But if the core team and committers are on board with the general idea of search paths and per-extension directory organization, it would be best to avoid getting stuck with maintaining the current patch’s GUC.

OTOH, we could get it committed now and revert it later if we get the better thing done and committed.

>> I think we should get some clarity on the proposal, and then consensus, as you say. I say “get some clarity” because my proposal doesn’t require recursing, and I’m not sure why it’d be needed.
>
> From what you and Gabriele are discussing (which I agree with), it wouldn’t.

Ah, great.

I’ll try to put some thought into a more formal proposal in a new thread next week. Unless your Gabriele beats me to it 😂.

Best,

David

Thanks for your efforts, a secondary directory for extensions is a very
useful feature.

Is there any updates on the patch?

-Ebru

On Thu, Aug 29, 2024 at 6:55 PM David E. Wheeler <david(at)justatheory(dot)com>
wrote:

> On Aug 27, 2024, at 22:24, Craig Ringer <craig(dot)ringer(at)enterprisedb(dot)com>
> wrote:
>
> > `pg_config` only cares about compile-time settings, so I would not
> > expect its output to change.
>
> Right, of course that’s its original purpose, but extensions depend on it
> to determine where to install extensions. Not just PGXS, but also pgrx and
> various Makefile customizations I’ve seen in the wild.
>
> > I suspect we'd have to add PGXS extension-path awareness if going for
> > per-extension subdir support. I'm not sure it makes sense to teach
> > `pg_config` about this, since it'd need to have a different mode like
> >
> > pg_config --extension myextname --extension-sharedir
> >
> > since the extension's "sharedir" is
> > $basedir/extensions/myextension/share or whatever.
>
> Right. PGXS would just need to know where to put the directory for an
> extension. There should be a default for the project, and then it can be
> overridden with something like DESTDIR (but without full paths under that
> prefix).
>
> > Supporting this looks to be a bit intrusive in the makefiles,
> > requiring them to differentiate between "share dir for extensions" and
> > "share dir for !extensions", etc. I'm not immediately sure if it can
> > be done in a way that transparently converts unmodified extension PGXS
> > makefiles to target the new paths; it might require an additional
> > define, or use of new variables and an ifdef block to add
> > backwards-compat to the extension makefile for building on older
> > postgres.
>
> Yeah, might just have to be an entirely new thing, though it sure would be
> nice for existing PGXS-using Makefiles to do the right thing. Maybe for the
> new version of the server with the proposed new pattern it would dispatch
> to the new thing somehow without modifying all the rest of its logic.
>
> > Right. The proposed structure is rather a bigger change than I was
> > thinking when I suggested supporting an extension search path not just
> > a single extra path. But it's also a cleaner proposal; the
> > per-extension directory would make it easier to ensure that the
> > extension control file, sql scripts, and dynamic library all match the
> > same extension and version if multiple ones are on the path. Which is
> > desirable when doing things like testing a new version of an in-core
> > extension.
>
> 💯
>
> > Right. And I've been on the receiving end of having a small, focused
> > patch derailed by others jumping in and scope-exploding it into
> > something completely different to solve a much wider but related
> > problem.
>
> I’m not complaining, I would definitely prefer to see consensus on a
> cleaner proposal along the lines we’ve discussed and a commitment to time
> from parties able to get it done in time for v18. I’m willing to help where
> I can with my baby C! Failing that, we can fall back on the destdir patch.
>
> > I'm definitely not trying to stand in the way of progress with this; I
> > just want to make sure that it doesn't paint us into a corner that
> > prevents a more general solution from being adopted later. That's why
> > I'm suggesting making the config a multi-value string (list of paths)
> > and raising a runtime "ERROR: searching multiple paths for extensions
> > not yet supported" or something if >1 path configured.
> >
> > If that doesn't work, no problem.
>
> I think the logic would have to be different, so they’d be different GUCs
> with their own semantics. But if the core team and committers are on board
> with the general idea of search paths and per-extension directory
> organization, it would be best to avoid getting stuck with maintaining the
> current patch’s GUC.
>
> OTOH, we could get it committed now and revert it later if we get the
> better thing done and committed.
>
> >> I think we should get some clarity on the proposal, and then consensus,
> as you say. I say “get some clarity” because my proposal doesn’t require
> recursing, and I’m not sure why it’d be needed.
> >
> > From what you and Gabriele are discussing (which I agree with), it
> wouldn’t.
>
> Ah, great.
>
> I’ll try to put some thought into a more formal proposal in a new thread
> next week. Unless your Gabriele beats me to it 😂.
>
> Best,
>
> David
>
>
>
>

On Oct 10, 2024, at 13:20, Ebru Aydin Gol <ebruaydin(at)gmail(dot)com> wrote:

> Thanks for your efforts, a secondary directory for extensions is a very useful feature.
>
> Is there any updates on the patch?

There haven't been, no, but your reply has chastened me! I’ve now started a separate thread[1] proposing directory-based extension packaging, as promised up-thread.

Best,

David

[1]: https://www.postgresql.org/message-id/2CAD6FA7-DC25-48FC-80F2-8F203DECAE6A%40justatheory.com

❤️

Ebru Aydin Gol reacted via Gmail
<https://www.google.com/gmail/about/?utm_source=gmail-in-product&utm_medium=et&utm_campaign=emojireactionemail#app>

On Thu, Oct 10, 2024 at 11:35 PM David E. Wheeler <david(at)justatheory(dot)com>
wrote:

> On Oct 10, 2024, at 13:20, Ebru Aydin Gol <ebruaydin(at)gmail(dot)com> wrote:
>
> > Thanks for your efforts, a secondary directory for extensions is a very
> useful feature.
> >
> > Is there any updates on the patch?
>
> There haven't been, no, but your reply has chastened me! I’ve now started
> a separate thread[1] proposing directory-based extension packaging, as
> promised up-thread.
>
> Best,
>
> David
>
> [1]:
> https://www.postgresql.org/message-id/2CAD6FA7-DC25-48FC-80F2-8F203DECAE6A%40justatheory.com
>
>

I implemented a patch along the lines Craig had suggested. It's a new
GUC variable that is a path for extension control files. It's called
extension_control_path, and it works exactly the same way as
dynamic_library_path. Except that the magic token is called $system
instead of $libdir. In fact, most of the patch is refactoring the
routines in dfmgr.c to not hardcode dynamic_library_path but allow
searching for any file in any path. Once a control file is found, the
other extension support files (script files and auxiliary control files)
are looked for in the same directory.

This works pretty much fine for the use cases that have been presented
here, including installing extensions outside of the core installation
tree (for CNPG and Postgres.app) and for testing uninstalled extensions
(for Debian).

There are some TODOs in the patch. Some of those are for documentation
that needs to be completed. Others are for functions like
pg_available_extensions() that need to be rewritten to be aware of the
path. I think this would be pretty straightforward.

Some open problems or discussion points:

- You can install extensions into alternative directories using PGXS like

make install datadir=/else/where/share pkglibdir=/else/where/lib

This works. I was hoping it would work to use

make install prefix=/else/where

but that doesn't because of some details in Makefile.global. I think we
can tweak that a little bit to make that work too.

- With the current patch, if you install into datadir=/else/where/share,
then you need to set extension_control_path=/else/where/share/extension.
This is a bit confusing. Maybe we want to make the "extension" part
implicit.

- The biggest problem is that many extensions set in their control file

module_pathname = '$libdir/foo'

This disables the use of dynamic_library_path, so this whole idea of
installing an extension elsewhere won't work that way. The obvious
solution is that extensions change this to just 'foo'. But this will
require a lot updating work for many extensions, or a lot of patching by
packagers.

Maybe we could devise some sort of rule that if the extension control
file is found via the path outside of $system, then the leading $libdir
is ignored.

On Nov 11, 2024, at 02:16, Peter Eisentraut <peter(at)eisentraut(dot)org> wrote:

> I implemented a patch along the lines Craig had suggested.

Oh, nice, thank you.

> It's a new GUC variable that is a path for extension control files. It's called extension_control_path, and it works exactly the same way as dynamic_library_path. Except that the magic token is called $system instead of $libdir.

I assume we can bikeshed these names later. :-)

> In fact, most of the patch is refactoring the routines in dfmgr.c to not hardcode dynamic_library_path but allow searching for any file in any path. Once a control file is found, the other extension support files (script files and auxiliary control files) are looked for in the same directory.

What about shared libraries files?

> This works pretty much fine for the use cases that have been presented here, including installing extensions outside of the core installation tree (for CNPG and Postgres.app) and for testing uninstalled extensions (for Debian).

If I understand correctly, shared modules still lie in dynamic_library_path, yes? That makes things a bit more complicated, as the CNPG use case has to set up multiple persistent volumes to persist files put into various directories, notably sharedir and pkglibdir.

> - You can install extensions into alternative directories using PGXS like
>
> make install datadir=/else/where/share pkglibdir=/else/where/lib
>
> This works. I was hoping it would work to use
>
> make install prefix=/else/where
>
> but that doesn't because of some details in Makefile.global. I think we can tweak that a little bit to make that work too.

In the broader extension organization RFC I’ve been working up[1], I propose a new Makefile prefix for the destination directory for an extension. It hinges on the idea that an extension has all of its files organized in a directory with the extension name, rather than separate params for data, pkglib, bin, etc.

> - With the current patch, if you install into datadir=/else/where/share, then you need to set extension_control_path=/else/where/share/extension. This is a bit confusing. Maybe we want to make the "extension" part implicit.

+1

> - The biggest problem is that many extensions set in their control file
>
> module_pathname = '$libdir/foo'
>
> This disables the use of dynamic_library_path, so this whole idea of installing an extension elsewhere won't work that way. The obvious solution is that extensions change this to just 'foo'. But this will require a lot updating work for many extensions, or a lot of patching by packagers.

Since that’s set at build/install time, couldn’t the definition of `$libdir` here be changed to mean “the directory into which it’s being installed right now?”. Doesn’t seem necessary to search a path if the specific location is set at install time.

> Maybe we could devise some sort of rule that if the extension control file is found via the path outside of $system, then the leading $libdir is ignored.
> <v0-0001-extension_control_path.patch>

This is what I propose,[1] yes. If we redefine the organization of extension files to live in a single directory named for an extension, then once you’ve found the control files all the other files are there, too. But `$libdir` is presumably still meaningful, since the first time you call an extension function in a new connection, it just tries to load that location, it doesn’t go through the control file search process, AFAIK.

Perhaps I misunderstand, but I would like to talk through the implications of a more radical rethinking of extension file location along the lines of the other thread[2] and the RFC I’m working up based on them both[1], especially since there are a few other use cases that inform it.

A notable one is the idea Gabriele shared with me in Athens to be able to add an extension to a running CNPG pod by simply mounting a read-only volume with all the files it requires.

Best,

David

[1]: https://github.com/theory/justatheory/pull/7/files

On 11.11.24 19:15, David E. Wheeler wrote:
>> In fact, most of the patch is refactoring the routines in dfmgr.c to not hardcode dynamic_library_path but allow searching for any file in any path. Once a control file is found, the other extension support files (script files and auxiliary control files) are looked for in the same directory.
>
> What about shared libraries files?

Nothing changes about shared library files. They are looked up in
dynamic_library_path or any hardcoded file name.

>> This works pretty much fine for the use cases that have been presented here, including installing extensions outside of the core installation tree (for CNPG and Postgres.app) and for testing uninstalled extensions (for Debian).
>
> If I understand correctly, shared modules still lie in dynamic_library_path, yes? That makes things a bit more complicated, as the CNPG use case has to set up multiple persistent volumes to persist files put into various directories, notably sharedir and pkglibdir.

No, you can also install them into a common directory and mount that
one. For example, you install the extension at build time into
/tmp/foo/{lib,share/extension}, you package that up as a disk image,
mount it at /opt/extensions/myext, and then you can point
extension_control_path at /opt/extensions/myext/lib and
dynamic_library_path at /opt/extensions/myext/share/extension.

>> - The biggest problem is that many extensions set in their control file
>>
>> module_pathname = '$libdir/foo'
>>
>> This disables the use of dynamic_library_path, so this whole idea of installing an extension elsewhere won't work that way. The obvious solution is that extensions change this to just 'foo'. But this will require a lot updating work for many extensions, or a lot of patching by packagers.
>
> Since that’s set at build/install time, couldn’t the definition of `$libdir` here be changed to mean “the directory into which it’s being installed right now?”. Doesn’t seem necessary to search a path if the specific location is set at install time.

No, this is not set at build or install time. This is for typical
extensions hardcoded, and $libdir is resolved by the PostgreSQL server
at run time.

> Perhaps I misunderstand, but I would like to talk through the implications of a more radical rethinking of extension file location along the lines of the other thread[2] and the RFC I’m working up based on them both[1], especially since there are a few other use cases that inform it.

I'm aware of that thread, but I think that is looking like a much larger
project than what I'm proposing here.

On Nov 12, 2024, at 08:25, Peter Eisentraut <peter(at)eisentraut(dot)org> wrote:

> No, you can also install them into a common directory and mount that one. For example, you install the extension at build time into /tmp/foo/{lib,share/extension}, you package that up as a disk image, mount it at /opt/extensions/myext, and then you can point extension_control_path at /opt/extensions/myext/lib and dynamic_library_path at /opt/extensions/myext/share/extension.

Ah, I see, then you just have to set both GUCs to subdirectories of the one volume.

>> Since that’s set at build/install time, couldn’t the definition of `$libdir` here be changed to mean “the directory into which it’s being installed right now?”. Doesn’t seem necessary to search a path if the specific location is set at install time.
>
> No, this is not set at build or install time. This is for typical extensions hardcoded, and $libdir is resolved by the PostgreSQL server at run time.

I see, so that they could be moved and, as long as dynamic_library_path is updated, would still be findable.

So back to your original caveat:

>>> - The biggest problem is that many extensions set in their control file
>>>
>>> module_pathname = '$libdir/foo'
>>>
>>> This disables the use of dynamic_library_path, so this whole idea of installing an extension elsewhere won't work that way. The obvious solution is that extensions change this to just 'foo'. But this will require a lot updating work for many extensions, or a lot of patching by packagers.

Yeah, '$libdir/foo' has been the documented way to do it for quite some time, as I recall. Perhaps the behavior of the MODULE_PATHNAME replacement function could be changed to omit $libdir when writing the SQL files?

>> Perhaps I misunderstand, but I would like to talk through the implications of a more radical rethinking of extension file location along the lines of the other thread[2] and the RFC I’m working up based on them both[1], especially since there are a few other use cases that inform it.
>
> I'm aware of that thread, but I think that is looking like a much larger project than what I'm proposing here.

Fair enough. Once we get to some consensus on a design there (and I’ve continued to iterate on it elsewhere[1]), I doubt it’d take much to use this patch as the first step toward it.

Best,

David

[1]: https://github.com/theory/justatheory/pull/7/files

Hi Peter,

Making another pass at this proposal, I’m a bit confused by this issue:

On Nov 12, 2024, at 09:44, David E. Wheeler <david(at)justatheory(dot)com> wrote:

>>>> - The biggest problem is that many extensions set in their control file
>>>>
>>>> module_pathname = '$libdir/foo'
>>>>
>>>> This disables the use of dynamic_library_path, so this whole idea of installing an extension elsewhere won't work that way. The obvious solution is that extensions change this to just 'foo'. But this will require a lot updating work for many extensions, or a lot of patching by packagers.
>
> Yeah, '$libdir/foo' has been the documented way to do it for quite some time, as I recall. Perhaps the behavior of the MODULE_PATHNAME replacement function could be changed to omit $libdir when writing the SQL files?

Elsewhere you write:

> Nothing changes about shared library files. They are looked up in dynamic_library_path or any hardcoded file name.

And also point out that the way to install them is:

```
make install datadir=/else/where/share pkglibdir=/else/where/lib
```

So as long as dynamic_library_path includes /else/where/lib it should work, just as before, no?

Best,

David

On 18.11.24 20:19, David E. Wheeler wrote:
>>>>> - The biggest problem is that many extensions set in their control file
>>>>>
>>>>> module_pathname = '$libdir/foo'
>>>>>
>>>>> This disables the use of dynamic_library_path, so this whole idea of installing an extension elsewhere won't work that way. The obvious solution is that extensions change this to just 'foo'. But this will require a lot updating work for many extensions, or a lot of patching by packagers.
>>
>> Yeah, '$libdir/foo' has been the documented way to do it for quite some time, as I recall. Perhaps the behavior of the MODULE_PATHNAME replacement function could be changed to omit $libdir when writing the SQL files?
>
> Elsewhere you write:
>
>> Nothing changes about shared library files. They are looked up in dynamic_library_path or any hardcoded file name.
>
> And also point out that the way to install them is:
>
> ```
> make install datadir=/else/where/share pkglibdir=/else/where/lib
> ```
>
> So as long as dynamic_library_path includes /else/where/lib it should work, just as before, no?

The path is only consulted if the specified name does not contain a
slash. So if you do LOAD 'foo', the path is consulted, but if you do
LOAD '$libdir/foo', it is not. The problem I'm describing is that most
extensions use the latter style, per current recommendation in the
documentation.

On Nov 20, 2024, at 04:05, Peter Eisentraut <peter(at)eisentraut(dot)org> wrote:

> The path is only consulted if the specified name does not contain a slash. So if you do LOAD 'foo', the path is consulted, but if you do LOAD '$libdir/foo', it is not. The problem I'm describing is that most extensions use the latter style, per current recommendation in the documentation.

I see; some details here:

https://www.postgresql.org/docs/current/xfunc-c.html#XFUNC-C-DYNLOAD

And I suppose the `directory` control file variable and `MODULEDIR` make variable make that necessary.

Maybe $libdir should be stripped out when installing extensions to work with this patch?

Best,

David

On 11.11.24 08:16, Peter Eisentraut wrote:
> I implemented a patch along the lines Craig had suggested.  It's a new
> GUC variable that is a path for extension control files.  It's called
> extension_control_path, and it works exactly the same way as
> dynamic_library_path.  Except that the magic token is called $system
> instead of $libdir.  In fact, most of the patch is refactoring the
> routines in dfmgr.c to not hardcode dynamic_library_path but allow
> searching for any file in any path.  Once a control file is found, the
> other extension support files (script files and auxiliary control files)
> are looked for in the same directory.

> There are some TODOs in the patch.  Some of those are for documentation
> that needs to be completed.  Others are for functions like
> pg_available_extensions() that need to be rewritten to be aware of the
> path.  I think this would be pretty straightforward.

I've made a bit of progress on this patch, filled in some documentation
and resolved some TODO markers. Also:

> Some open problems or discussion points:
>
> - You can install extensions into alternative directories using PGXS like
>
>     make install datadir=/else/where/share pkglibdir=/else/where/lib
>
> This works.  I was hoping it would work to use
>
>     make install prefix=/else/where
>
> but that doesn't because of some details in Makefile.global.  I think we
> can tweak that a little bit to make that work too.

This works now.

> - The biggest problem is that many extensions set in their control file
>
>     module_pathname = '$libdir/foo'
>
> This disables the use of dynamic_library_path, so this whole idea of
> installing an extension elsewhere won't work that way.  The obvious
> solution is that extensions change this to just 'foo'.  But this will
> require a lot updating work for many extensions, or a lot of patching by
> packagers.

I have solved this by just stripping off "$libdir/" from the front of
the filename. This works for now. We can think about other ways to
tweak this, perhaps, but I don't see any drawback to this in practice.

This patch is now complete enough for testing, I think. As I mentioned
earlier, I haven't updated pg_available_extensions() etc. to support the
path, but that shouldn't prevent some testing.

Hello Peter & Co.

On Dec 5, 2024, at 06:07, Peter Eisentraut <peter(at)eisentraut(dot)org> wrote:

> I've made a bit of progress on this patch, filled in some documentation and resolved some TODO markers. Also:

Finally getting around to reviewing this patch. Should it be considered part of the previous patch[1] for purposes of commitfest tracking?

I’ve also created a GitHub PR[2] for anyone who’d prefer to look it over that way.

>> Some open problems or discussion points:
>> - You can install extensions into alternative directories using PGXS like
>> make install datadir=/else/where/share pkglibdir=/else/where/lib
>> This works. I was hoping it would work to use
>> make install prefix=/else/where
>> but that doesn't because of some details in Makefile.global. I think we can tweak that a little bit to make that work too.
>
> This works now.

I tried `prefix` with semver[3] and it did not work:

``` console
❯ make PG_CONFIG=~/dev/misc/postgres/pgsql-devel/bin/pg_config prefix=/Users/david/pgsql-test
gcc -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Werror=vla -Werror=unguarded-availability-new -Wendif-labels -Wmissing-format-attribute -Wcast-function-type -Wformat-security -Wmissing-variable-declarations -fno-strict-aliasing -fwrapv -fexcess-precision=standard -Wno-unused-command-line-argument -Wno-compound-token-split-by-macro -Wno-cast-function-type-strict -I/opt/homebrew/opt/readline/include -I/opt/homebrew/opt/openssl/include -I/opt/homebrew/opt/libxml2/include -I/opt/homebrew/opt/icu4c/include -fvisibility=hidden -I. -I./ -I/Users/david/pgsql-test/include/server -I/Users/david/pgsql-test/include/internal -I/opt/homebrew/Cellar/icu4c(at)76/76.1_1/include -isysroot /Library/Developer/CommandLineTools/SDKs/MacOSX15.1.sdk -I/opt/homebrew/opt/readline/include -I/opt/homebrew/opt/openssl/include -I/opt/homebrew/opt/libxml2/include -I/opt/homebrew/opt/icu4c/include -c -o src/semver.o src/semver.c
src/semver.c:13:10: fatal error: 'postgres.h' file not found
13 | #include "postgres.h"
| ^~~~~~~~~~~~
1 error generated.
make: *** [src/semver.o] Error 1
```

It works fine without `prefix` to install into the default directories as before. It also installs fine with `datadir` and `pkglibdir`:

``` console
❯ make PG_CONFIG=~/dev/misc/postgres/pgsql-devel/bin/pg_config datadir=/Users/david/pgsql-test/share pkglibdir=/Users/david/pgsql-test/lib install
/opt/homebrew/bin/gmkdir -p '/Users/david/pgsql-test/share/extension'
/opt/homebrew/bin/gmkdir -p '/Users/david/pgsql-test/share/semver'
/opt/homebrew/bin/gmkdir -p '/Users/david/pgsql-test/lib'
/opt/homebrew/bin/gmkdir -p '/Users/david/dev/misc/postgres/pgsql-devel/share/doc/semver'
/opt/homebrew/bin/ginstall -c -m 644 .//semver.control '/Users/david/pgsql-test/share/extension/'
/opt/homebrew/bin/ginstall -c -m 644 .//sql/semver--0.10.0--0.11.0.sql .//sql/semver--0.11.0--0.12.0.sql .//sql/semver--0.12.0--0.13.0.sql .//sql/semver--0.13.0--0.15.0.sql .//sql/semver--0.15.0--0.16.0.sql .//sql/semver--0.16.0--0.17.0.sql .//sql/semver--0.17.0--0.20.0.sql .//sql/semver--0.2.1--0.2.4.sql .//sql/semver--0.2.4--0.3.0.sql .//sql/semver--0.20.0--0.21.0.sql .//sql/semver--0.21.0--0.22.0.sql .//sql/semver--0.22.0--0.30.0.sql .//sql/semver--0.3.0--0.4.0.sql .//sql/semver--0.30.0--0.31.0.sql .//sql/semver--0.31.0--0.31.1.sql .//sql/semver--0.31.1--0.31.2.sql .//sql/semver--0.31.2--0.32.0.sql .//sql/semver--0.5.0--0.10.0.sql .//sql/semver--unpackaged--0.2.1.sql '/Users/david/pgsql-test/share/semver/'
/opt/homebrew/bin/ginstall -c -m 755 src/semver.dylib '/Users/david/pgsql-test/lib/'
/opt/homebrew/bin/gmkdir -p '/Users/david/pgsql-test/lib/bitcode/src/semver'
/opt/homebrew/bin/gmkdir -p '/Users/david/pgsql-test/lib/bitcode'/src/semver/src/
/opt/homebrew/bin/ginstall -c -m 644 src/semver.bc '/Users/david/pgsql-test/lib/bitcode'/src/semver/src/
cd '/Users/david/pgsql-test/lib/bitcode' && /opt/homebrew/Cellar/llvm/19.1.6/bin/llvm-lto -thinlto -thinlto-action=thinlink -o src/semver.index.bc src/semver/src/semver.bc
/opt/homebrew/bin/ginstall -c -m 644 .//doc/semver.mmd '/Users/david/dev/misc/postgres/pgsql-devel/share/doc/semver/'
```

But then it won’t load:

```psql
postgres=# create extension semver;
ERROR: extension "semver" has no installation script nor update path for version “0.40.0"
```

Since `semver` uses the directory parameter[4], I decided to try another C extension that doesn’t use it, envvar[5], which worked:

``` psql
postgres=# create extension envvar;
CREATE EXTENSION
```

So I suspect the issue is that, when looking for SQL files, the patch needs to use the directory parameter[4] when it’s set --- and it can be an absolute path! Honestly I think there’s a case to be made for eliminating that parameter.

The `prefix` param works with a pure SQL extension like pair[6]:

```console
❯ make PG_CONFIG=~/dev/misc/postgres/pgsql-devel/bin/pg_config prefix=/Users/david/pgsql-test
cp sql/pair.sql sql/pair--0.1.2.sql

❯ make PG_CONFIG=~/dev/misc/postgres/pgsql-devel/bin/pg_config prefix=/Users/david/pgsql-test install
/opt/homebrew/bin/gmkdir -p '/Users/david/pgsql-test/share/extension'
/opt/homebrew/bin/gmkdir -p '/Users/david/pgsql-test/share/extension'
/opt/homebrew/bin/gmkdir -p '/Users/david/pgsql-test/share/doc//extension'
/opt/homebrew/bin/ginstall -c -m 644 .//pair.control '/Users/david/pgsql-test/share/extension/'
/opt/homebrew/bin/ginstall -c -m 644 .//sql/pair--0.1.2.sql .//sql/pair--unpackaged--0.1.2.sql '/Users/david/pgsql-test/share/extension/'
/opt/homebrew/bin/ginstall -c -m 644 .//doc/pair.md '/Users/david/pgsql-test/share/doc//extension/‘
```

I thought it would put the files into /Users/david/pgsql-test/extension, not /Users/david/pgsql-test/share/extension? I guess that makes sense; but then perhaps the search path should be for prefixes, in which case I’d use a config like:

``` ini
extension_control_path = '/Users/david/pgsql-test:$system'
dynamic_library_path = '/Users/david/pgsql-test/lib:$libdir'
```

And the search path would append `share/extension` to each path. But then it varies from the behavior of `dynamic_library_path`. :-(

Not at all sure where the doc files should go unless, again, prefix is truly used as a prefix for all the things, which frankly seems reasonable.

I’m wondering whether there should be formal documentation of prefix, datadir, pkglibdir, etc. I haven’t noticed them in the PGXS docs[7].

>> This disables the use of dynamic_library_path, so this whole idea of installing an extension elsewhere won't work that way. The obvious solution is that extensions change this to just 'foo'. But this will require a lot updating work for many extensions, or a lot of patching by packagers.
>
>
> I have solved this by just stripping off "$libdir/" from the front of the filename. This works for now. We can think about other ways to tweak this, perhaps, but I don't see any drawback to this in practice.

I think this makes perfect sense. I presume it’s stripped out *before* replacing the MODULE_PATHNAME string in SQL files, yes?

# Patch Review

Compiles fine. All tests pass. Some comments on the docs:

> <primary><varname>extension_control_path</varname> configuration parameter</primary>

Although the current path explicitly searches for control files, I’d like to suggest a more generic name, since the point is to find extensions; control files are just the (current) method for identifying them. I suggest `extension_path`. Although given the above, maybe it should be all about prefixes.

> The value for <varname>extension_control_path</varname> must be a
> list of absolute directory paths separated by colons (or semi-colons
> on Windows). If a list element starts
> with the special string <literal>$system</literal>, the

I like this idea, though I quibble with the term `$system`, which seems like it would identify SYSCONFDIR, not SHAREDIR/extension. How about `$extdir` or, if using prefixes, `$coreprefix` or something.

> Note that the path elements should typically end in
> <literal>extension</literal> if the normal installation layouts are
> followed. (The value for <literal>$system</literal> already includes
> the <literal>extension</literal> suffix.)

In fact, if we’re using `prefix` as currently implemented, it should end in `share/extension`, no?

> Note that if you set this parameter to be able to load extensions from
> nonstandard locations, you will most likely also need to set <xref
> linkend="guc-dynamic-library-path"/> to a correspondent location, for

s/correspondent/corresponding/

> example,
> <programlisting>
> extension_control_path = '/usr/local/share/postgresql/extension:$system'
> dynamic_library_path = '/usr/local/lib/postgresql:$libdir'
> </programlisting>

This makes sense in the context of this patch, but I sure would like to see these features decoupled as much as possible.

<digression type="brief">

Hence my proposal that extensions each have their own directory. For example, via Slack Gabriele sent Peter and me a POC for loading individual extensions as mounted volumes in an OCI container. The Kubernetes config looks like this:

``` yaml
postgresql:
shared_preload_libraries:
- pg_squeeze
parameters:
extension_control_path: '/extensions/pgvector/18/share:/extensions/pgsqueeze/18/share:$system'
dynamic_library_path: '/extensions/pgvector/18/lib:/extensions/pgsqueeze/18/lib:$libdir'

extensions:
- name: pgvector
image:
reference: ghcr.io/cloudnative-pg/pgvector-testing:0.8.0
- name: pgsqueeze
image:
reference: ghcr.io/cloudnative-pg/pgsqueeze-testing:1.7
```

Which works! But it also means that new directories need to be appended to `extension_control_path` and, often `dynamic_library_path` for every extension added. It would be much nicer to just have 2-3 paths that contain extensions identified by a directory name. Then, to add a new extension, one just installs it to the appropriate prefix. (This is also yet another reason to eliminate the directory parameter[4].)

This is only additional wish I had for this feature, but I also believe we can iterate in that direction based on your patch.

</digression>

> the installation's <literal>SHAREDIR</literal> directory. By default,
> the script files are looked for in the same directory where the
> control file was found.

This could use a pointer to the impact of the directory parameter[4]. Bit of a wildcard for DBAs who want to use this feature, TBH.

Best,

David

[1]: https://commitfest.postgresql.org/51/4913/
[2]: https://github.com/theory/postgres/pull/9/files
[3]: https://github.com/theory/pg-semver
[4]: https://www.postgresql.org/docs/current/extend-extensions.html#EXTEND-EXTENSIONS-FILES-DIRECTORY
[5]: https://github.com/theory/pg-envvar
[6]: https://github.com/theory/kv-pair
[7]: https://www.postgresql.org/docs/current/extend-pgxs.html

On 14.01.25 21:01, David E. Wheeler wrote:
> I tried `prefix` with semver[3] and it did not work:
>
> ``` console
> ❯ make PG_CONFIG=~/dev/misc/postgres/pgsql-devel/bin/pg_config prefix=/Users/david/pgsql-test
> gcc -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Werror=vla -Werror=unguarded-availability-new -Wendif-labels -Wmissing-format-attribute -Wcast-function-type -Wformat-security -Wmissing-variable-declarations -fno-strict-aliasing -fwrapv -fexcess-precision=standard -Wno-unused-command-line-argument -Wno-compound-token-split-by-macro -Wno-cast-function-type-strict -I/opt/homebrew/opt/readline/include -I/opt/homebrew/opt/openssl/include -I/opt/homebrew/opt/libxml2/include -I/opt/homebrew/opt/icu4c/include -fvisibility=hidden -I. -I./ -I/Users/david/pgsql-test/include/server -I/Users/david/pgsql-test/include/internal -I/opt/homebrew/Cellar/icu4c(at)76/76.1_1/include -isysroot /Library/Developer/CommandLineTools/SDKs/MacOSX15.1.sdk -I/opt/homebrew/opt/readline/include -I/opt/homebrew/opt/openssl/include -I/opt/homebrew/opt/libxml2/include -I/opt/homebrew/opt/icu4c/include -c -o src/semver.o src/semver.c
> src/semver.c:13:10: fatal error: 'postgres.h' file not found
> 13 | #include "postgres.h"
> | ^~~~~~~~~~~~
> 1 error generated.
> make: *** [src/semver.o] Error 1
> ```

prefix= should only be set when running the "install" target, not when
building (make all).

> But then it won’t load:
>
> ```psql
> postgres=# create extension semver;
> ERROR: extension "semver" has no installation script nor update path for version “0.40.0"
> ```
>
> Since `semver` uses the directory parameter[4],

Yeah, this is the problem. I'm not sure what to do about it. Setting
the directory parameter is a bit like setting an absolute rpath. You're
then stuck with that particular directory location.

> So I suspect the issue is that, when looking for SQL files, the patch needs to use the directory parameter[4] when it’s set --- and it can be an absolute path! Honestly I think there’s a case to be made for eliminating that parameter.

Possibly. I didn't know why extensions would use that parameter, before
you showed an example.

> I thought it would put the files into /Users/david/pgsql-test/extension, not /Users/david/pgsql-test/share/extension? I guess that makes sense; but then perhaps the search path should be for prefixes, in which case I’d use a config like:
>
> ``` ini
> extension_control_path = '/Users/david/pgsql-test:$system'
> dynamic_library_path = '/Users/david/pgsql-test/lib:$libdir'
> ```
>
> And the search path would append `share/extension` to each path. But then it varies from the behavior of `dynamic_library_path`. :-(

Yes exactly. This is meant to be symmetrical.

We could have a setting that just sets the top-level prefixes to search
and would thus supersede both of these settings. But then you introduce
another lay of complications, such as, the subdirectory structure under
the prefix is not necessarily fixed. It could be lib, lib/postgresql,
lib64, etc., similar under share. It's not even required that there is
a common prefix.

>>> This disables the use of dynamic_library_path, so this whole idea of installing an extension elsewhere won't work that way. The obvious solution is that extensions change this to just 'foo'. But this will require a lot updating work for many extensions, or a lot of patching by packagers.
>>
>>
>> I have solved this by just stripping off "$libdir/" from the front of the filename. This works for now. We can think about other ways to tweak this, perhaps, but I don't see any drawback to this in practice.
>
> I think this makes perfect sense. I presume it’s stripped out *before* replacing the MODULE_PATHNAME string in SQL files, yes?

No, actually it's done after. Does it make a difference?

> # Patch Review
>
> Compiles fine. All tests pass. Some comments on the docs:
>
>> <primary><varname>extension_control_path</varname> configuration parameter</primary>
>
> Although the current path explicitly searches for control files, I’d like to suggest a more generic name, since the point is to find extensions; control files are just the (current) method for identifying them. I suggest `extension_path`. Although given the above, maybe it should be all about prefixes.

If we implemented a prefix approach, then 'extension_path' could be a
good name. But right now we're not.

>> The value for <varname>extension_control_path</varname> must be a
>> list of absolute directory paths separated by colons (or semi-colons
>> on Windows). If a list element starts
>> with the special string <literal>$system</literal>, the
>
> I like this idea, though I quibble with the term `$system`, which seems like it would identify SYSCONFDIR, not SHAREDIR/extension. How about `$extdir` or, if using prefixes, `$coreprefix` or something.

I'm not attached to '$system', but I don't see how you get from that to
SYSCONFDIR. The other suggestions also have various ways of
misinterpreting them. We can keep thinking about this one.

>> Note that the path elements should typically end in
>> <literal>extension</literal> if the normal installation layouts are
>> followed. (The value for <literal>$system</literal> already includes
>> the <literal>extension</literal> suffix.)
>
> In fact, if we’re using `prefix` as currently implemented, it should end in `share/extension`, no?

It could be share/postgresql/extension, too. So I didn't want to
overdocument this, because it could vary. The examples just above this
are hopefully helpful.

Hi Peter,

> prefix= should only be set when running the "install" target, not when building (make all).

I see. I confirm that works. Still, don’t all the other parameters work when passed to any/all targets? Should this one have an extension-specific name?

>> So I suspect the issue is that, when looking for SQL files, the patch needs to use the directory parameter[4] when it’s set --- and it can be an absolute path! Honestly I think there’s a case to be made for eliminating that parameter.
>
> Possibly. I didn't know why extensions would use that parameter, before you showed an example.

ISTM it does more harm than good. The location of extension files should be highly predictable. I think the search path functionality mitigates the need for this parameter, and it should be dropped.

>> I thought it would put the files into /Users/david/pgsql-test/extension, not /Users/david/pgsql-test/share/extension? I guess that makes sense; but then perhaps the search path should be for prefixes, in which case I’d use a config like:
>> ``` ini
>> extension_control_path = '/Users/david/pgsql-test:$system'
>> dynamic_library_path = '/Users/david/pgsql-test/lib:$libdir'
>> ```
>> And the search path would append `share/extension` to each path. But then it varies from the behavior of `dynamic_library_path`. :-(
>
> Yes exactly. This is meant to be symmetrical.

IOW, `extension_control_path`s should always end in `share/extension` and `dynamic_library_path`s should always end in `lib`, yes?

> We could have a setting that just sets the top-level prefixes to search and would thus supersede both of these settings. But then you introduce another lay of complications, such as, the subdirectory structure under the prefix is not necessarily fixed. It could be lib, lib/postgresql, lib64, etc., similar under share. It's not even required that there is a common prefix.

I would say that, under those prefixes, the directory names have to be defined by PostgreSQL, not by compile-time options. It seems to me that by providing search paths there is less of a need to monkey with directory names.

>>> I have solved this by just stripping off "$libdir/" from the front of the filename. This works for now. We can think about other ways to tweak this, perhaps, but I don't see any drawback to this in practice.
>>
>> I think this makes perfect sense. I presume it’s stripped out *before* replacing the MODULE_PATHNAME string in SQL files, yes?
>
> No, actually it's done after. Does it make a difference?

If you mean it strips it out at runtime, it just feels a little less clean to me than if it was formatted properly before `make install`ing.

>> Although the current path explicitly searches for control files, I’d like to suggest a more generic name, since the point is to find extensions; control files are just the (current) method for identifying them. I suggest `extension_path`. Although given the above, maybe it should be all about prefixes.
>
> If we implemented a prefix approach, then 'extension_path' could be a good name. But right now we're not.

I’m not sure what difference it makes, especially since each directory in the path is expected to end in `share/extension`, not `share/control`. Feels more symmetrical to me.

>> I like this idea, though I quibble with the term `$system`, which seems like it would identify SYSCONFDIR, not SHAREDIR/extension. How about `$extdir` or, if using prefixes, `$coreprefix` or something.
>
> I'm not attached to '$system', but I don't see how you get from that to SYSCONFDIR.

Because it’s the only pg_config item that includes the word “Sys” in it, meaning the operating system.

> The other suggestions also have various ways of misinterpreting them. We can keep thinking about this one.

I like $extdir. Short, clear, and not conflicting with existing pg_config options. I guess it could be misinterpreted as “extra directory” or something, but since there is no such thing it seems like a minor risk. I am likely overlooking something though.

>>> Note that the path elements should typically end in
>>> <literal>extension</literal> if the normal installation layouts are
>>> followed. (The value for <literal>$system</literal> already includes
>>> the <literal>extension</literal> suffix.)
>> In fact, if we’re using `prefix` as currently implemented, it should end in `share/extension`, no?
>
> It could be share/postgresql/extension, too. So I didn't want to overdocument this, because it could vary. The examples just above this are hopefully helpful.

Yeah, but if we can define it specifically, and disallow its modification, it simplifies things. And if understand correctly, paths like SHAREDIR defined at compile time are absolute paths, not suffixes to a prefix, yes?

But perhaps our packaging friends object to disallowing customization of this suffix?

Best,

David

On 2025-02-03 Mo 3:42 PM, David E. Wheeler wrote:
> Hi Peter,
>
>> prefix= should only be set when running the "install" target, not when building (make all).
> I see. I confirm that works. Still, don’t all the other parameters work when passed to any/all targets? Should this one have an extension-specific name?

IDK, I don't understand what you think you're saying when you specify
--prefix to an extension build (as opposed to an install).

>
>>> So I suspect the issue is that, when looking for SQL files, the patch needs to use the directory parameter[4] when it’s set --- and it can be an absolute path! Honestly I think there’s a case to be made for eliminating that parameter.
>> Possibly. I didn't know why extensions would use that parameter, before you showed an example.
> ISTM it does more harm than good. The location of extension files should be highly predictable. I think the search path functionality mitigates the need for this parameter, and it should be dropped.

I agree that we should either drop the "directory" directive or fix this
patch so it doesn't break it. I have never used the directive, not sure
I was even aware of its existence, so I have no objection to dropping it.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Hi Andrew,

On Feb 4, 2025, at 15:34, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:

>> I see. I confirm that works. Still, don’t all the other parameters work when passed to any/all targets? Should this one have an extension-specific name?
>
> IDK, I don't understand what you think you're saying when you specify --prefix to an extension build (as opposed to an install).

I am unfamiliar with that option. Although `prefix` is mentioned in the PGXS docs[1] in the context of other variables, it is not itself documented, neither as `prefix=` as Peter suggests, nor as `--prefix`.

At any rate, all the other PGXS variables I’ve used have worked with all the make targets, though they obviously don’t necessarily change the behavior of all of the targets.

>> ISTM it does more harm than good. The location of extension files should be highly predictable. I think the search path functionality mitigates the need for this parameter, and it should be dropped.
>
> I agree that we should either drop the "directory" directive or fix this patch so it doesn't break it. I have never used the directive, not sure I was even aware of its existence, so I have no objection to dropping it.

I only just started using it, thinking it keeps things better organized. But it’s honestly a bit confusing, in that one must set both the `MODULEDIR` variable in the `Makefile` and the `directory` variable in the control file.

Best,

David

[1]: https://www.postgresql.org/docs/current/extend-pgxs.html

Hi,

On Thu, Dec 5, 2024 at 8:07 AM Peter Eisentraut <peter(at)eisentraut(dot)org> wrote:
>
> This patch is now complete enough for testing, I think. As I mentioned
> earlier, I haven't updated pg_available_extensions() etc. to support the
> path, but that shouldn't prevent some testing.

To help with this patch I'm attaching a new version with the remaining TODOs
fixed and also with a new TAP test.

Thoughts?

--
Matheus Alcantara

On 2025-02-24 Mo 8:33 AM, Matheus Alcantara wrote:
> Hi,
>
> On Thu, Dec 5, 2024 at 8:07 AM Peter Eisentraut <peter(at)eisentraut(dot)org> wrote:
>> This patch is now complete enough for testing, I think. As I mentioned
>> earlier, I haven't updated pg_available_extensions() etc. to support the
>> path, but that shouldn't prevent some testing.
> To help with this patch I'm attaching a new version with the remaining TODOs
> fixed and also with a new TAP test.
>
> Thoughts?
>

I think your additions generally look good. We should be able to
simplify this:

+    system_dir = psprintf("%s/extension", sharepath);
+    ecp = system_dir;
+
+    if (strlen(Extension_control_path) == 0)
+    {
+        paths = lappend(paths, ecp);
+    }

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Thanks for reviewing!

On Tue, Feb 25, 2025 at 9:45 AM Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
> I think your additions generally look good. We should be able to
> simplify this:
>
>
> + system_dir = psprintf("%s/extension", sharepath);
> + ecp = system_dir;
> +
> + if (strlen(Extension_control_path) == 0)
> + {
> + paths = lappend(paths, ecp);
> + }
>
Fixed on the attached v3.

--
Matheus Alcantara

Hi

On Tue, Feb 25, 2025 at 5:29 PM Matheus Alcantara
<matheusssilv97(at)gmail(dot)com> wrote:
> Fixed on the attached v3.
>
After I've sent the v3 patch I noticed that the tests were failing on windows.
The problem was on TAP test that was using ":" as a separator on
extension_control_path and also the path was not being escaped correctly
resulting in a wrong path configuration.

Attached v4 with all these fixes.

--
Matheus Alcantara

Hi everyone,

I have finally been able to test the patch in a Kubernetes environment with
CloudNativePG, thanks to Niccolò Fei and Marco Nenciarini, who created a
pilot patch for CloudNativePG (
https://github.com/cloudnative-pg/cloudnative-pg/pull/6546)

In the meantime, Kubernetes is likely adding the ImageVolume feature
starting from the upcoming version 1.33. I will write a blog post soon
about how CloudNativePG will benefit from this feature. See
https://github.com/kubernetes/enhancements/issues/4639.

Although the steps are not easily reproducible by everyone, I can confirm
that I successfully mounted a volume in the Postgres pod using a container
image that includes only pgvector (with a size of 1.6MB - see
https://github.com/EnterpriseDB/pgvector/blob/dev/5645/Dockerfile.cnpg)

By setting:

dynamic_library_path = '$libdir:/extensions/pgvector/lib'
extension_control_path = '$system:/extensions/pgvector/share'

I was able to run the following queries:

postgres=# SELECT * FROM pg_available_extensions WHERE name = 'vector';
-[ RECORD 1 ]-----+-----------------------------------------------------
name | vector
default_version | 0.8.0
installed_version |
comment | vector data type and ivfflat and hnsw access methods

postgres=# SELECT * FROM pg_available_extensions WHERE name = 'vector';
-[ RECORD 1 ]-----+-----------------------------------------------------
name | vector
default_version | 0.8.0
installed_version |
comment | vector data type and ivfflat and hnsw access methods

I also successfully ran the following:

postgres=# SELECT * FROM pg_extension_update_paths('vector');

By emptying the content of `extension_control_path`, the vector extension
disappeared from the list.

postgres=# SHOW extension_control_path ;
extension_control_path
------------------------
$system
(1 row)

postgres=# SELECT * FROM pg_available_extensions WHERE name = 'vector';
name | default_version | installed_version | comment
------+-----------------+-------------------+---------
(0 rows)

postgres=# SELECT * FROM pg_available_extension_versions WHERE name =
'vector';
name | version | installed | superuser | trusted | relocatable | schema |
requires | comment
------+---------+-----------+-----------+---------+-------------+--------+----------+---------
(0 rows)

In my opinion, the patch already helps a lot and does what I can reasonably
expect from a first iteration of improvements in enabling immutable
container images that ship a self-contained extension to be dynamically
loaded and unloaded from a Postgres cluster in Kubernetes. From here, it is
all about learning how to improve things with an exploratory mindset with
future versions of Postgres and Kubernetes. It's a long journey but this is
a fundamental step in the right direction.

Let me know if there's more testing for me to do. The documentation looks
clear to me.

Thank you to everyone who contributed to this patch, from the initial
discussions to the development phase. I sincerely hope this is included in
Postgres 18.

Ciao,
Gabriele

On Fri, 28 Feb 2025 at 16:36, Matheus Alcantara <matheusssilv97(at)gmail(dot)com>
wrote:

> Hi
>
> On Tue, Feb 25, 2025 at 5:29 PM Matheus Alcantara
> <matheusssilv97(at)gmail(dot)com> wrote:
> > Fixed on the attached v3.
> >
> After I've sent the v3 patch I noticed that the tests were failing on
> windows.
> The problem was on TAP test that was using ":" as a separator on
> extension_control_path and also the path was not being escaped correctly
> resulting in a wrong path configuration.
>
> Attached v4 with all these fixes.
>
> --
> Matheus Alcantara
>

--
Gabriele Bartolini
VP, Chief Architect, Kubernetes
enterprisedb.com

As promised, here is a blog article that provides more context and
information about what this feature will mean in Kubernetes with
CloudNativePG:
https://www.gabrielebartolini.it/articles/2025/03/the-immutable-future-of-postgresql-extensions-in-kubernetes-with-cloudnativepg/

Thanks,
Gabriele

On Sat, 1 Mar 2025 at 10:15, Gabriele Bartolini <
gabriele(dot)bartolini(at)enterprisedb(dot)com> wrote:

> Hi everyone,
>
> I have finally been able to test the patch in a Kubernetes environment
> with CloudNativePG, thanks to Niccolò Fei and Marco Nenciarini, who created
> a pilot patch for CloudNativePG (
> https://github.com/cloudnative-pg/cloudnative-pg/pull/6546)
>
> In the meantime, Kubernetes is likely adding the ImageVolume feature
> starting from the upcoming version 1.33. I will write a blog post soon
> about how CloudNativePG will benefit from this feature. See
> https://github.com/kubernetes/enhancements/issues/4639.
>
> Although the steps are not easily reproducible by everyone, I can confirm
> that I successfully mounted a volume in the Postgres pod using a container
> image that includes only pgvector (with a size of 1.6MB - see
> https://github.com/EnterpriseDB/pgvector/blob/dev/5645/Dockerfile.cnpg)
>
> By setting:
>
> dynamic_library_path = '$libdir:/extensions/pgvector/lib'
> extension_control_path = '$system:/extensions/pgvector/share'
>
> I was able to run the following queries:
>
> postgres=# SELECT * FROM pg_available_extensions WHERE name = 'vector';
> -[ RECORD 1 ]-----+-----------------------------------------------------
> name | vector
> default_version | 0.8.0
> installed_version |
> comment | vector data type and ivfflat and hnsw access methods
>
> postgres=# SELECT * FROM pg_available_extensions WHERE name = 'vector';
> -[ RECORD 1 ]-----+-----------------------------------------------------
> name | vector
> default_version | 0.8.0
> installed_version |
> comment | vector data type and ivfflat and hnsw access methods
>
> I also successfully ran the following:
>
> postgres=# SELECT * FROM pg_extension_update_paths('vector');
>
> By emptying the content of `extension_control_path`, the vector extension
> disappeared from the list.
>
> postgres=# SHOW extension_control_path ;
> extension_control_path
> ------------------------
> $system
> (1 row)
>
> postgres=# SELECT * FROM pg_available_extensions WHERE name = 'vector';
> name | default_version | installed_version | comment
> ------+-----------------+-------------------+---------
> (0 rows)
>
> postgres=# SELECT * FROM pg_available_extension_versions WHERE name =
> 'vector';
> name | version | installed | superuser | trusted | relocatable | schema |
> requires | comment
>
> ------+---------+-----------+-----------+---------+-------------+--------+----------+---------
> (0 rows)
>
> In my opinion, the patch already helps a lot and does what I can
> reasonably expect from a first iteration of improvements in enabling
> immutable container images that ship a self-contained extension to be
> dynamically loaded and unloaded from a Postgres cluster in Kubernetes. From
> here, it is all about learning how to improve things with an exploratory
> mindset with future versions of Postgres and Kubernetes. It's a long
> journey but this is a fundamental step in the right direction.
>
> Let me know if there's more testing for me to do. The documentation looks
> clear to me.
>
> Thank you to everyone who contributed to this patch, from the initial
> discussions to the development phase. I sincerely hope this is included in
> Postgres 18.
>
> Ciao,
> Gabriele
>
> On Fri, 28 Feb 2025 at 16:36, Matheus Alcantara <matheusssilv97(at)gmail(dot)com>
> wrote:
>
>> Hi
>>
>> On Tue, Feb 25, 2025 at 5:29 PM Matheus Alcantara
>> <matheusssilv97(at)gmail(dot)com> wrote:
>> > Fixed on the attached v3.
>> >
>> After I've sent the v3 patch I noticed that the tests were failing on
>> windows.
>> The problem was on TAP test that was using ":" as a separator on
>> extension_control_path and also the path was not being escaped correctly
>> resulting in a wrong path configuration.
>>
>> Attached v4 with all these fixes.
>>
>> --
>> Matheus Alcantara
>>
>
>
> --
> Gabriele Bartolini
> VP, Chief Architect, Kubernetes
> enterprisedb.com
>

--
Gabriele Bartolini
VP, Chief Architect, Kubernetes
enterprisedb.com

On Mar 3, 2025, at 08:39, Gabriele Bartolini <gabriele(dot)bartolini(at)enterprisedb(dot)com> wrote:

> As promised, here is a blog article that provides more context and information about what this feature will mean in Kubernetes with CloudNativePG: https://www.gabrielebartolini.it/articles/2025/03/the-immutable-future-of-postgresql-extensions-in-kubernetes-with-cloudnativepg/

Very nice writeup, thank you. Makes me wish for the bandwidth to get back to and start refining the PGXN OCI RFC to specify tags and/or metadata for the distribution of different contents, including:

* Full extension distribution (as currently written)
* Distribution with only pkglibdir and sharedir contents
* Distribution with only pkglibdir contents
* Distribution with only sharedir contents

I think we could come up with a standard that would work very nicely for a variety of use cases.

Will have to see if I can scrounge up the time for it.

Best,

David

On Mar 3, 2025, at 10:05, David E. Wheeler <david(at)justatheory(dot)com> wrote:

> Very nice writeup, thank you. Makes me wish for the bandwidth to get back to and start refining the PGXN OCI RFC

Forgot to link to the POC[1]. The RFC[2] is not OCI-specific, but the POC demonstrates the “full content” version. Will likely want to modify the binary distribution RFC to adopt a standard layout that allows for volume mounting to “install” an extension.

Best,

David

1: https://justatheory.com/2024/06/trunk-oci-poc/
2: https://github.com/pgxn/rfcs/pull/2

Hi, attached a new v5 with some minor improvements on TAP tests:

- Add a proper test name for all test cases
- Add CREATE EXTENSION command execution
- Change the assert on pg_available_extensions and
pg_available_extension_versions to validate the row content

Also rebased with master

--
Matheus Alcantara

On 03.03.25 19:45, Matheus Alcantara wrote:
> Hi, attached a new v5 with some minor improvements on TAP tests:
>
> - Add a proper test name for all test cases
> - Add CREATE EXTENSION command execution
> - Change the assert on pg_available_extensions and
> pg_available_extension_versions to validate the row content
>
> Also rebased with master

This looks very good to me. I have one issue to point out: The logic
in get_extension_control_directories() needs to be a little bit more
careful to align with the rules in find_in_path(). For example, it
should use first_path_var_separator() to get the platform-specific path
separator, and probably also substitute_path_macro() and
canonicalize_path() etc., to keep everything consistent. (Maybe it
would be ok to move the function to dfmgr.c to avoid having to export
too many things from there.)

Independent of that, attached is a small patch that suggests to use the
newer foreach_ptr() macro in some places.

Hi,

Thanks for reviewing and suggestions!

On Thu, Mar 6, 2025 at 10:46 AM Peter Eisentraut <peter(at)eisentraut(dot)org> wrote:
> This looks very good to me. I have one issue to point out: The logic
> in get_extension_control_directories() needs to be a little bit more
> careful to align with the rules in find_in_path(). For example, it
> should use first_path_var_separator() to get the platform-specific path
> separator, and probably also substitute_path_macro() and
> canonicalize_path() etc., to keep everything consistent.
>
I fixed this hardcoded path separator issue on the TAP test and forgot
to fix it also on code, sorry, fixed on this new version.

I also spent some time investigating why the tests on Windows were still
passing even using a wrong path separator.

Consider extension_control_path = '$system;C:\custom\path'

When running on Windows, the get_extension_control_directories was
returning [$system;C:, \custom\path] and for somehow the \custom\path
was successfully being read and since the tests was only referencing the
extension on this custom path everything was passing, but querying for
an extension that is only on $system was resulting in an empty query
result. In the attached patch I also included a new test case to query
on pg_available_extensions for an extension that is installed on the
$system, so we can ensure that extensions in both paths can be used
correctly.

> (Maybe it would be ok to move the function to dfmgr.c to avoid having
> to export too many things from there.)
>
I've exported substitute_path_macro because adding a new function on
dfmgr would require #include nodes/pg_list.h and I'm not sure what
approach would be better, please let me know what you think.

--
Matheus Alcantara

On 10.03.25 21:25, Matheus Alcantara wrote:
> On Thu, Mar 6, 2025 at 10:46 AM Peter Eisentraut <peter(at)eisentraut(dot)org> wrote:
>> This looks very good to me. I have one issue to point out: The logic
>> in get_extension_control_directories() needs to be a little bit more
>> careful to align with the rules in find_in_path(). For example, it
>> should use first_path_var_separator() to get the platform-specific path
>> separator, and probably also substitute_path_macro() and
>> canonicalize_path() etc., to keep everything consistent.
>>
> I fixed this hardcoded path separator issue on the TAP test and forgot
> to fix it also on code, sorry, fixed on this new version.

>> (Maybe it would be ok to move the function to dfmgr.c to avoid having
>> to export too many things from there.)
>>
> I've exported substitute_path_macro because adding a new function on
> dfmgr would require #include nodes/pg_list.h and I'm not sure what
> approach would be better, please let me know what you think.

Yes, that structure looks ok. But you can remove one level of block in
get_extension_control_directories().

I found a bug that was already present in my earlier patch versions:

@@ -423,7 +424,7 @@ find_extension_control_filename(ExtensionControlFile
*control)
ecp = Extension_control_path;
if (strlen(ecp) == 0)
ecp = "$system";
- result = find_in_path(basename, Extension_control_path,
"extension_control_path", "$system", system_dir);
+ result = find_in_path(basename, ecp, "extension_control_path",
"$system", system_dir);

Without this, it won't work if you set extension_control_path empty.
(Maybe add a test for that?)

I think this all works now, but I think the way
pg_available_extensions() works is a bit strange and inefficient. After
it finds a candidate control file, it calls
read_extension_control_file() with the extension name, that calls
parse_extension_control_file(), that calls
find_extension_control_filename(), and that calls find_in_path(), which
searches that path again!

There should be a simpler way into this. Maybe
pg_available_extensions() should fill out the ExtensionControlFile
structure itself, set ->control_dir with where it found it, then call
directly to parse_extension_control_file(), and that should skip the
finding if the directory is already set. Or something similar.

On Tue, Mar 11, 2025 at 12:59 PM Peter Eisentraut <peter(at)eisentraut(dot)org> wrote:
> Yes, that structure looks ok. But you can remove one level of block in
> get_extension_control_directories().
>
Sorry, missed during debugging. Fixed

> I found a bug that was already present in my earlier patch versions:
>
> @@ -423,7 +424,7 @@ find_extension_control_filename(ExtensionControlFile
> *control)
> ecp = Extension_control_path;
> if (strlen(ecp) == 0)
> ecp = "$system";
> - result = find_in_path(basename, Extension_control_path,
> "extension_control_path", "$system", system_dir);
> + result = find_in_path(basename, ecp, "extension_control_path",
> "$system", system_dir);
>
> Without this, it won't work if you set extension_control_path empty.
> (Maybe add a test for that?)
>
Fixed, and also added a new test case for this.

> I think this all works now, but I think the way
> pg_available_extensions() works is a bit strange and inefficient. After
> it finds a candidate control file, it calls
> read_extension_control_file() with the extension name, that calls
> parse_extension_control_file(), that calls
> find_extension_control_filename(), and that calls find_in_path(), which
> searches that path again!
>
> There should be a simpler way into this. Maybe
> pg_available_extensions() should fill out the ExtensionControlFile
> structure itself, set ->control_dir with where it found it, then call
> directly to parse_extension_control_file(), and that should skip the
> finding if the directory is already set. Or something similar.
>
Good catch. I fixed this by creating a new function to construct the
ExtensionControlFile and changed the pg_available_extensions to set the
control_dir. The read_extension_control_file was also changed to just
call this new function constructor. I implemented the logic to check if
the control_dir is already set on parse_extension_control_file because
it seems to me that make more sense to not call
find_extension_control_filename instead of putting this logic there
since we already set the control_dir when we find the control file, and
having the logic to set the control_dir or skip the find_in_path seems
more confusing on this function instead of on
parse_extension_control_file. Please let me know what you think.

--
Matheus Alcantara

On 12.03.25 14:17, Matheus Alcantara wrote:
>> There should be a simpler way into this. Maybe
>> pg_available_extensions() should fill out the ExtensionControlFile
>> structure itself, set ->control_dir with where it found it, then call
>> directly to parse_extension_control_file(), and that should skip the
>> finding if the directory is already set. Or something similar.
>>
> Good catch. I fixed this by creating a new function to construct the
> ExtensionControlFile and changed the pg_available_extensions to set the
> control_dir. The read_extension_control_file was also changed to just
> call this new function constructor. I implemented the logic to check if
> the control_dir is already set on parse_extension_control_file because
> it seems to me that make more sense to not call
> find_extension_control_filename instead of putting this logic there
> since we already set the control_dir when we find the control file, and
> having the logic to set the control_dir or skip the find_in_path seems
> more confusing on this function instead of on
> parse_extension_control_file. Please let me know what you think.

Committed that, thanks.

A small tweak I made was to replace palloc+snprintf by psprintf. Maybe
you were not aware that that function exists.

I also simplified the error handling in parse_extension_control_file() a
bit. If we pass in a control directory (which is the new code we're
adding), then we can assume that we already found the file earlier, and
then if we now don't find it, then we should just report the file system
error instead of the "you should install this extension first" error.
It's kind of a "can't happen" error anyway, so the different is small.

Thanks everyone for making this happen.

Ciao,
Gabriele

On Wed, 19 Mar 2025 at 07:42, Peter Eisentraut <peter(at)eisentraut(dot)org> wrote:

> On 12.03.25 14:17, Matheus Alcantara wrote:
> >> There should be a simpler way into this. Maybe
> >> pg_available_extensions() should fill out the ExtensionControlFile
> >> structure itself, set ->control_dir with where it found it, then call
> >> directly to parse_extension_control_file(), and that should skip the
> >> finding if the directory is already set. Or something similar.
> >>
> > Good catch. I fixed this by creating a new function to construct the
> > ExtensionControlFile and changed the pg_available_extensions to set the
> > control_dir. The read_extension_control_file was also changed to just
> > call this new function constructor. I implemented the logic to check if
> > the control_dir is already set on parse_extension_control_file because
> > it seems to me that make more sense to not call
> > find_extension_control_filename instead of putting this logic there
> > since we already set the control_dir when we find the control file, and
> > having the logic to set the control_dir or skip the find_in_path seems
> > more confusing on this function instead of on
> > parse_extension_control_file. Please let me know what you think.
>
> Committed that, thanks.
>
> A small tweak I made was to replace palloc+snprintf by psprintf. Maybe
> you were not aware that that function exists.
>
> I also simplified the error handling in parse_extension_control_file() a
> bit. If we pass in a control directory (which is the new code we're
> adding), then we can assume that we already found the file earlier, and
> then if we now don't find it, then we should just report the file system
> error instead of the "you should install this extension first" error.
> It's kind of a "can't happen" error anyway, so the different is small.
>
>

--
Gabriele Bartolini
VP, Chief Architect, Kubernetes
enterprisedb.com

Re: Peter Eisentraut
> Committed that, thanks.

Awesome, thanks!

It works perfectly for the Debian "test extension packages at build
time" use case, replacing our old extension_destdir patch.

PKGARGS="--pgoption extension_control_path=$PWD/debian/$PACKAGE/usr/share/postgresql/$v/extension:\$system --pgoption dynamic_library_path=$PWD/debian/$PACKAGE/usr/lib/postgresql/$v/lib:/usr/lib/postgresql/$v/lib"

https://salsa.debian.org/postgresql/postgresql-common/-/commit/3792eea42e4dcef39b5c8d99f63deb8091ef9c46

Christoph

On Mar 19, 2025, at 02:42, Peter Eisentraut <peter(at)eisentraut(dot)org> wrote:

> Committed that, thanks.

🎉

I’ve been meaning to test the patch again, so here goes.

First thing I notice is that prefix= uses the magic to insert “postgresql” into the path if it’s not already there:

``` console
❯ make PG_CONFIG=~/dev/c/postgres/pgsql-devel/bin/pg_config prefix=/Users/david/Downloads install
/opt/homebrew/bin/gmkdir -p '/Users/david/Downloads/share/postgresql/extension'
/opt/homebrew/bin/gmkdir -p '/Users/david/Downloads/share/postgresql/extension'
/opt/homebrew/bin/gmkdir -p '/Users/david/Downloads/share/doc//postgresql/extension'
/opt/homebrew/bin/ginstall -c -m 644 .//pair.control '/Users/david/Downloads/share/postgresql/extension/'
/opt/homebrew/bin/ginstall -c -m 644 .//sql/pair--0.1.2.sql .//sql/pair--unpackaged--0.1.2.sql '/Users/david/Downloads/share/postgresql/extension/'
/opt/homebrew/bin/ginstall -c -m 644 .//doc/pair.md '/Users/david/Downloads/share/doc//postgresql/extension/‘
```

I think this should at least be documented, but generally feels unexpected to me. I’ve attached a patch that fleshes out the docs, along with an example of setting `extension_control_path` and `dynamic_library_path` to use the locations. It might not have the information right about the need for “postgresql” or “pgsql” in the path. Back in 2003[1] it was just “postgres”, but I couldn’t find the logic for it just now.

Everything else works very nicely except for extensions that use the Makefile `MODULEDIR` variable to install all of the share files except the control file into a particular directory, and the `directory` in the control file so that the files can be found. Here’s semver[2], which has both:

```console
❯ make PG_CONFIG=~/dev/c/postgres/pgsql-devel/bin/pg_config prefix=/Users/david/Downloads/postgresql install
/opt/homebrew/bin/gmkdir -p '/Users/david/Downloads/postgresql/share/extension'
/opt/homebrew/bin/gmkdir -p '/Users/david/Downloads/postgresql/share/semver'
/opt/homebrew/bin/gmkdir -p '/Users/david/Downloads/postgresql/lib'
/opt/homebrew/bin/gmkdir -p '/Users/david/Downloads/postgresql/share/doc//semver'
/opt/homebrew/bin/ginstall -c -m 644 .//semver.control '/Users/david/Downloads/postgresql/share/extension/'
/opt/homebrew/bin/ginstall -c -m 644 .//sql/semver--0.10.0--0.11.0.sql .//sql/semver--0.11.0--0.12.0.sql .//sql/semver--0.12.0--0.13.0.sql .//sql/semver--0.13.0--0.15.0.sql .//sql/semver--0.15.0--0.16.0.sql .//sql/semver--0.16.0--0.17.0.sql .//sql/semver--0.17.0--0.20.0.sql .//sql/semver--0.2.1--0.2.4.sql .//sql/semver--0.2.4--0.3.0.sql .//sql/semver--0.20.0--0.21.0.sql .//sql/semver--0.21.0--0.22.0.sql .//sql/semver--0.22.0--0.30.0.sql .//sql/semver--0.3.0--0.4.0.sql .//sql/semver--0.30.0--0.31.0.sql .//sql/semver--0.31.0--0.31.1.sql .//sql/semver--0.31.1--0.31.2.sql .//sql/semver--0.31.2--0.32.0.sql .//sql/semver--0.32.0--0.32.1.sql .//sql/semver--0.32.1--0.40.0.sql .//sql/semver--0.32.1.sql .//sql/semver--0.40.0.sql .//sql/semver--0.5.0--0.10.0.sql .//sql/semver--unpackaged--0.2.1.sql .//sql/semver.sql '/Users/david/Downloads/postgresql/share/semver/'
/opt/homebrew/bin/ginstall -c -m 755 src/semver.dylib '/Users/david/Downloads/postgresql/lib/'
/opt/homebrew/bin/gmkdir -p '/Users/david/Downloads/postgresql/lib/bitcode/src/semver'
/opt/homebrew/bin/gmkdir -p '/Users/david/Downloads/postgresql/lib/bitcode'/src/semver/src/
/opt/homebrew/bin/ginstall -c -m 644 src/semver.bc '/Users/david/Downloads/postgresql/lib/bitcode'/src/semver/src/
cd '/Users/david/Downloads/postgresql/lib/bitcode' && /opt/homebrew/Cellar/llvm/19.1.7_1/bin/llvm-lto -thinlto -thinlto-action=thinlink -o src/semver.index.bc src/semver/src/semver.bc
/opt/homebrew/bin/ginstall -c -m 644 .//doc/semver.mmd '/Users/david/Downloads/postgresql/share/doc//semver/‘
```

Following `MODULEDIR=semver`, it puts the SQL files into `share/semver/` instead of `share/extension/`, as expected, but then, even though the control file has `directory=semver`, it can’t load them:

```pgsql
david=# create extension semver;
ERROR: could not open directory "/Users/david/dev/c/postgres/pgsql-devel/share/semver": No such file or directory
```

Looks like it’s only looking in the `semver` subdirectory under $libdir and not the whole path.

But given that the `directory` variable in the control file can be a full path, I don’t see that there’s much of a way to generalize a solution. I guess there are three options:

1. If directory is a full path, try to load the files there. It probably already works that way, though I haven’t tired it.

2. If the directory is not a full path, check for it under each directory in `extension_control_path`? But no, that points to `share/extension`, not `share`, so it can’t really searched unless it also lops off `extension` from the end of each path.

3. Drop support for MODULEDIR and directory.

I think I’d opt for #3, personally, just to simplify things.

Anyway, I then built envvar, a C extension with no `directory` configuration, and it worked perfectly.

I will say, though, that I will kind of miss being able to run `make install` without first running `make`, as the `prefix` variable does not work with `make`.

Best,

David

[1]: https://postgr.es/m/[email protected]
[2]: https://github.com/theory/pg-semver/
[3]: https://github.com/theory/pg-envvar

Peter Eisentraut <peter(at)eisentraut(dot)org> writes:
> Committed that, thanks.

Buildfarm member snakefly doesn't like this too much. Since no other
animals have failed, I guess it must be about local conditions on
that machine, but the report is pretty opaque:

# +++ tap check in src/test/modules/test_extensions +++

# Failed test '$system extension is installed correctly on pg_available_extensions'
# at t/001_extension_control_path.pl line 69.
# got: 'f'
# expected: 't'

# Failed test '$system extension is installed correctly on pg_available_extensions with empty extension_control_path'
# at t/001_extension_control_path.pl line 76.
# got: 'f'
# expected: 't'
# Looks like you failed 2 tests of 5.
[06:43:53] t/001_extension_control_path.pl ..
Dubious, test returned 2 (wstat 512, 0x200)
Failed 2/5 subtests

Looking at the test, it presupposes that "amcheck" must be an
available extension. I do not see anything that guarantees
that that's so, though. It'd fail if contrib hasn't been
installed. Is there a reason to use "amcheck" rather than
something more certainly available, like "plpgsql"?

regards, tom lane

On Wed, Mar 19, 2025 at 3:56 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Peter Eisentraut <peter(at)eisentraut(dot)org> writes:
> > Committed that, thanks.
>
> Buildfarm member snakefly doesn't like this too much. Since no other
> animals have failed, I guess it must be about local conditions on
> that machine, but the report is pretty opaque:
>
> # +++ tap check in src/test/modules/test_extensions +++
>
> # Failed test '$system extension is installed correctly on pg_available_extensions'
> # at t/001_extension_control_path.pl line 69.
> # got: 'f'
> # expected: 't'
>
> # Failed test '$system extension is installed correctly on pg_available_extensions with empty extension_control_path'
> # at t/001_extension_control_path.pl line 76.
> # got: 'f'
> # expected: 't'
> # Looks like you failed 2 tests of 5.
> [06:43:53] t/001_extension_control_path.pl ..
> Dubious, test returned 2 (wstat 512, 0x200)
> Failed 2/5 subtests
>
> Looking at the test, it presupposes that "amcheck" must be an
> available extension. I do not see anything that guarantees
> that that's so, though. It'd fail if contrib hasn't been
> installed. Is there a reason to use "amcheck" rather than
> something more certainly available, like "plpgsql"?

There is no specific reason to use "amcheck" instead of "plpgsql". Attached a
patch with this change, sorry about that.

(Not sure if we should also improve the message to make the test failure less
opaque?)

--
Matheus Alcantara

Matheus Alcantara <matheusssilv97(at)gmail(dot)com> writes:
> (Not sure if we should also improve the message to make the test failure less
> opaque?)

Yeah, I was wondering how to do that. The earlier tests in that
script show the whole row from pg_available_extensions, not just a
bool ... but that doesn't help if the problem is we don't find a row.

regards, tom lane

On 19.03.25 20:25, Matheus Alcantara wrote:
> On Wed, Mar 19, 2025 at 3:56 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>
>> Peter Eisentraut <peter(at)eisentraut(dot)org> writes:
>>> Committed that, thanks.
>>
>> Buildfarm member snakefly doesn't like this too much. Since no other
>> animals have failed, I guess it must be about local conditions on
>> that machine, but the report is pretty opaque:
>>
>> # +++ tap check in src/test/modules/test_extensions +++
>>
>> # Failed test '$system extension is installed correctly on pg_available_extensions'
>> # at t/001_extension_control_path.pl line 69.
>> # got: 'f'
>> # expected: 't'
>>
>> # Failed test '$system extension is installed correctly on pg_available_extensions with empty extension_control_path'
>> # at t/001_extension_control_path.pl line 76.
>> # got: 'f'
>> # expected: 't'
>> # Looks like you failed 2 tests of 5.
>> [06:43:53] t/001_extension_control_path.pl ..
>> Dubious, test returned 2 (wstat 512, 0x200)
>> Failed 2/5 subtests
>>
>> Looking at the test, it presupposes that "amcheck" must be an
>> available extension. I do not see anything that guarantees
>> that that's so, though. It'd fail if contrib hasn't been
>> installed. Is there a reason to use "amcheck" rather than
>> something more certainly available, like "plpgsql"?
>
> There is no specific reason to use "amcheck" instead of "plpgsql". Attached a
> patch with this change, sorry about that.

Committed.

I was able to reproduce the problem from scratch using:

./configure ...
make # no contrib
make -C src/test/modules/test_extensions check

So it depended on in which order you build the various components.

Peter Eisentraut <peter(at)eisentraut(dot)org> writes:
> On 19.03.25 20:25, Matheus Alcantara wrote:
>> On Wed, Mar 19, 2025 at 3:56 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> Buildfarm member snakefly doesn't like this too much.

> I was able to reproduce the problem from scratch using:

> ./configure ...
> make # no contrib
> make -C src/test/modules/test_extensions check

> So it depended on in which order you build the various components.

That makes sense, but I wonder how snakefly hit it while other BF
animals did not. It's running a reasonably up-to-date BF client
version and there's nothing odd-looking about its configuration.

Anyway, I see snakefly is green now so that tweak did fix it.

regards, tom lane

On 2025-03-19 We 2:55 PM, Tom Lane wrote:
> Peter Eisentraut<peter(at)eisentraut(dot)org> writes:
>> Committed that, thanks.
> Buildfarm member snakefly doesn't like this too much. Since no other
> animals have failed, I guess it must be about local conditions on
> that machine, but the report is pretty opaque:
>
> # +++ tap check in src/test/modules/test_extensions +++
>
> # Failed test '$system extension is installed correctly on pg_available_extensions'
> # at t/001_extension_control_path.pl line 69.
> # got: 'f'
> # expected: 't'
>
> # Failed test '$system extension is installed correctly on pg_available_extensions with empty extension_control_path'
> # at t/001_extension_control_path.pl line 76.
> # got: 'f'
> # expected: 't'
> # Looks like you failed 2 tests of 5.
> [06:43:53] t/001_extension_control_path.pl ..
> Dubious, test returned 2 (wstat 512, 0x200)
> Failed 2/5 subtests
>
> Looking at the test, it presupposes that "amcheck" must be an
> available extension. I do not see anything that guarantees
> that that's so, though. It'd fail if contrib hasn't been
> installed. Is there a reason to use "amcheck" rather than
> something more certainly available, like "plpgsql"?

I think something else must be going on. The failure in question came
after the step "install-contrib" succeeded, and the log file for that shows:

make[1]: Entering directory `/opt/postgres/build-farm-18/HEAD/pgsql.build/contrib/amcheck'
/usr/bin/mkdir -p '/opt/postgres/build-farm-18/HEAD/pgsql.build/tmp_install/opt/postgres/build-farm-18/HEAD/inst/lib'
/usr/bin/mkdir -p '/opt/postgres/build-farm-18/HEAD/pgsql.build/tmp_install/opt/postgres/build-farm-18/HEAD/inst/share/extension'
/usr/bin/mkdir -p '/opt/postgres/build-farm-18/HEAD/pgsql.build/tmp_install/opt/postgres/build-farm-18/HEAD/inst/share/extension'
/usr/bin/install -c -m 755 amcheck.so '/opt/postgres/build-farm-18/HEAD/pgsql.build/tmp_install/opt/postgres/build-farm-18/HEAD/inst/lib/amcheck.so'
/usr/bin/install -c -m 644 ./amcheck.control '/opt/postgres/build-farm-18/HEAD/pgsql.build/tmp_install/opt/postgres/build-farm-18/HEAD/inst/share/extension/'
/usr/bin/install -c -m 644 ./amcheck--1.3--1.4.sql ./amcheck--1.2--1.3.sql ./amcheck--1.1--1.2.sql ./amcheck--1.0--1.1.sql ./amcheck--1.0.sql '/opt/postgres/build-farm-18/HEAD/pgsql.build/tmp_install/opt/postgres/build-farm-18/HEAD/inst/share/extension/'
make[1]: Leaving directory `/opt/postgres/build-farm-18/HEAD/pgsql.build/contrib/amcheck'

(wondering if this another of these cases where the "path includes postgres" thing bites us, and we're looking in the wrong place)

cheers

andrew

--
Andrew Dunstan
EDB:https://www.enterprisedb.com

On 2025-03-20 Th 10:53 AM, Andrew Dunstan wrote:
>
>
> On 2025-03-19 We 2:55 PM, Tom Lane wrote:
>> Peter Eisentraut<peter(at)eisentraut(dot)org> writes:
>>> Committed that, thanks.
>> Buildfarm member snakefly doesn't like this too much. Since no other
>> animals have failed, I guess it must be about local conditions on
>> that machine, but the report is pretty opaque:
>>
>> # +++ tap check in src/test/modules/test_extensions +++
>>
>> # Failed test '$system extension is installed correctly on pg_available_extensions'
>> # at t/001_extension_control_path.pl line 69.
>> # got: 'f'
>> # expected: 't'
>>
>> # Failed test '$system extension is installed correctly on pg_available_extensions with empty extension_control_path'
>> # at t/001_extension_control_path.pl line 76.
>> # got: 'f'
>> # expected: 't'
>> # Looks like you failed 2 tests of 5.
>> [06:43:53] t/001_extension_control_path.pl ..
>> Dubious, test returned 2 (wstat 512, 0x200)
>> Failed 2/5 subtests
>>
>> Looking at the test, it presupposes that "amcheck" must be an
>> available extension. I do not see anything that guarantees
>> that that's so, though. It'd fail if contrib hasn't been
>> installed. Is there a reason to use "amcheck" rather than
>> something more certainly available, like "plpgsql"?
>
>
>
> I think something else must be going on. The failure in question came
> after the step "install-contrib" succeeded, and the log file for that
> shows:
>
>
> make[1]: Entering directory `/opt/postgres/build-farm-18/HEAD/pgsql.build/contrib/amcheck'
> /usr/bin/mkdir -p '/opt/postgres/build-farm-18/HEAD/pgsql.build/tmp_install/opt/postgres/build-farm-18/HEAD/inst/lib'
> /usr/bin/mkdir -p '/opt/postgres/build-farm-18/HEAD/pgsql.build/tmp_install/opt/postgres/build-farm-18/HEAD/inst/share/extension'
> /usr/bin/mkdir -p '/opt/postgres/build-farm-18/HEAD/pgsql.build/tmp_install/opt/postgres/build-farm-18/HEAD/inst/share/extension'
> /usr/bin/install -c -m 755 amcheck.so '/opt/postgres/build-farm-18/HEAD/pgsql.build/tmp_install/opt/postgres/build-farm-18/HEAD/inst/lib/amcheck.so'
> /usr/bin/install -c -m 644 ./amcheck.control '/opt/postgres/build-farm-18/HEAD/pgsql.build/tmp_install/opt/postgres/build-farm-18/HEAD/inst/share/extension/'
> /usr/bin/install -c -m 644 ./amcheck--1.3--1.4.sql ./amcheck--1.2--1.3.sql ./amcheck--1.1--1.2.sql ./amcheck--1.0--1.1.sql ./amcheck--1.0.sql '/opt/postgres/build-farm-18/HEAD/pgsql.build/tmp_install/opt/postgres/build-farm-18/HEAD/inst/share/extension/'
> make[1]: Leaving directory `/opt/postgres/build-farm-18/HEAD/pgsql.build/contrib/amcheck'
>
>
> (wondering if this another of these cases where the "path includes postgres" thing bites us, and we're looking in the wrong place)
>
>
>

Nope, testing shows it's not that, so I am rather confused about what
was going on.

cheers

andrew

--
Andrew Dunstan
EDB:https://www.enterprisedb.com

On Thu, Mar 20, 2025 at 7:38 PM Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
>>>
>>> Buildfarm member snakefly doesn't like this too much. Since no other
>>> animals have failed, I guess it must be about local conditions on
>>> that machine, but the report is pretty opaque:
>>>
>>> # +++ tap check in src/test/modules/test_extensions +++
>>>
>>> # Failed test '$system extension is installed correctly on pg_available_extensions'
>>> # at t/001_extension_control_path.pl line 69.
>>> # got: 'f'
>>> # expected: 't'
>>>
>>> # Failed test '$system extension is installed correctly on pg_available_extensions with empty extension_control_path'
>>> # at t/001_extension_control_path.pl line 76.
>>> # got: 'f'
>>> # expected: 't'
>>> # Looks like you failed 2 tests of 5.
>>> [06:43:53] t/001_extension_control_path.pl ..
>>> Dubious, test returned 2 (wstat 512, 0x200)
>>> Failed 2/5 subtests
>>>
>>> Looking at the test, it presupposes that "amcheck" must be an
>>> available extension. I do not see anything that guarantees
>>> that that's so, though. It'd fail if contrib hasn't been
>>> installed. Is there a reason to use "amcheck" rather than
>>> something more certainly available, like "plpgsql"?
>>
>> I think something else must be going on. The failure in question came after the step "install-contrib" succeeded, and the log file for that shows:
>>
>>
>> make[1]: Entering directory `/opt/postgres/build-farm-18/HEAD/pgsql.build/contrib/amcheck'
>> /usr/bin/mkdir -p '/opt/postgres/build-farm-18/HEAD/pgsql.build/tmp_install/opt/postgres/build-farm-18/HEAD/inst/lib'
>> /usr/bin/mkdir -p '/opt/postgres/build-farm-18/HEAD/pgsql.build/tmp_install/opt/postgres/build-farm-18/HEAD/inst/share/extension'
>> /usr/bin/mkdir -p '/opt/postgres/build-farm-18/HEAD/pgsql.build/tmp_install/opt/postgres/build-farm-18/HEAD/inst/share/extension'
>> /usr/bin/install -c -m 755 amcheck.so '/opt/postgres/build-farm-18/HEAD/pgsql.build/tmp_install/opt/postgres/build-farm-18/HEAD/inst/lib/amcheck.so'
>> /usr/bin/install -c -m 644 ./amcheck.control '/opt/postgres/build-farm-18/HEAD/pgsql.build/tmp_install/opt/postgres/build-farm-18/HEAD/inst/share/extension/'
>> /usr/bin/install -c -m 644 ./amcheck--1.3--1.4.sql ./amcheck--1.2--1.3.sql ./amcheck--1.1--1.2.sql ./amcheck--1.0--1.1.sql ./amcheck--1.0.sql '/opt/postgres/build-farm-18/HEAD/pgsql.build/tmp_install/opt/postgres/build-farm-18/HEAD/inst/share/extension/'
>> make[1]: Leaving directory `/opt/postgres/build-farm-18/HEAD/pgsql.build/contrib/amcheck'
>>
>>
>> (wondering if this another of these cases where the "path includes postgres" thing bites us, and we're looking in the wrong place)
>
> Nope, testing shows it's not that, so I am rather confused about what was going on.
>

I'm not sure if I'm checking on the right place [1] but it seems that the
Contrib and ContribInstall is executed after Check step which causes this test
failure?

'steps_completed' => [
'SCM-checkout',
'Configure',
'Build',
'Check',
'Contrib',
'TestModules',
'Install',
'ContribInstall',
'TestModulesInstall',
'MiscCheck',
...
]

[1] https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=snakefly&dt=2025-03-20%2009%3A46%3A05
--
Matheus Alcantara

On 2025-03-21 Fr 11:52 AM, Matheus Alcantara wrote:
> On Thu, Mar 20, 2025 at 7:38 PM Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
>>>> Buildfarm member snakefly doesn't like this too much. Since no other
>>>> animals have failed, I guess it must be about local conditions on
>>>> that machine, but the report is pretty opaque:
>>>>
>>>> # +++ tap check in src/test/modules/test_extensions +++
>>>>
>>>> # Failed test '$system extension is installed correctly on pg_available_extensions'
>>>> # at t/001_extension_control_path.pl line 69.
>>>> # got: 'f'
>>>> # expected: 't'
>>>>
>>>> # Failed test '$system extension is installed correctly on pg_available_extensions with empty extension_control_path'
>>>> # at t/001_extension_control_path.pl line 76.
>>>> # got: 'f'
>>>> # expected: 't'
>>>> # Looks like you failed 2 tests of 5.
>>>> [06:43:53] t/001_extension_control_path.pl ..
>>>> Dubious, test returned 2 (wstat 512, 0x200)
>>>> Failed 2/5 subtests
>>>>
>>>> Looking at the test, it presupposes that "amcheck" must be an
>>>> available extension. I do not see anything that guarantees
>>>> that that's so, though. It'd fail if contrib hasn't been
>>>> installed. Is there a reason to use "amcheck" rather than
>>>> something more certainly available, like "plpgsql"?
>>> I think something else must be going on. The failure in question came after the step "install-contrib" succeeded, and the log file for that shows:
>>>
>>>
>>> make[1]: Entering directory `/opt/postgres/build-farm-18/HEAD/pgsql.build/contrib/amcheck'
>>> /usr/bin/mkdir -p '/opt/postgres/build-farm-18/HEAD/pgsql.build/tmp_install/opt/postgres/build-farm-18/HEAD/inst/lib'
>>> /usr/bin/mkdir -p '/opt/postgres/build-farm-18/HEAD/pgsql.build/tmp_install/opt/postgres/build-farm-18/HEAD/inst/share/extension'
>>> /usr/bin/mkdir -p '/opt/postgres/build-farm-18/HEAD/pgsql.build/tmp_install/opt/postgres/build-farm-18/HEAD/inst/share/extension'
>>> /usr/bin/install -c -m 755 amcheck.so '/opt/postgres/build-farm-18/HEAD/pgsql.build/tmp_install/opt/postgres/build-farm-18/HEAD/inst/lib/amcheck.so'
>>> /usr/bin/install -c -m 644 ./amcheck.control '/opt/postgres/build-farm-18/HEAD/pgsql.build/tmp_install/opt/postgres/build-farm-18/HEAD/inst/share/extension/'
>>> /usr/bin/install -c -m 644 ./amcheck--1.3--1.4.sql ./amcheck--1.2--1.3.sql ./amcheck--1.1--1.2.sql ./amcheck--1.0--1.1.sql ./amcheck--1.0.sql '/opt/postgres/build-farm-18/HEAD/pgsql.build/tmp_install/opt/postgres/build-farm-18/HEAD/inst/share/extension/'
>>> make[1]: Leaving directory `/opt/postgres/build-farm-18/HEAD/pgsql.build/contrib/amcheck'
>>>
>>>
>>> (wondering if this another of these cases where the "path includes postgres" thing bites us, and we're looking in the wrong place)
>> Nope, testing shows it's not that, so I am rather confused about what was going on.
>>
> I'm not sure if I'm checking on the right place [1] but it seems that the
> Contrib and ContribInstall is executed after Check step which causes this test
> failure?
>
> 'steps_completed' => [
> 'SCM-checkout',
> 'Configure',
> 'Build',
> 'Check',
> 'Contrib',
> 'TestModules',
> 'Install',
> 'ContribInstall',
> 'TestModulesInstall',
> 'MiscCheck',
> ...
> ]
>
> [1] https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=snakefly&dt=2025-03-20%2009%3A46%3A05

No. In the buildfarm, the Check step only runs the core regression
tests, not any TAP tests. The above shows fairly clearly that the
failure occurred after the ContribInstall step, which is what's puzzling me.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Matheus Alcantara <matheusssilv97(at)gmail(dot)com> writes:
> On Thu, Mar 20, 2025 at 7:38 PM Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
>>> (wondering if this another of these cases where the "path includes postgres" thing bites us, and we're looking in the wrong place)

>> Nope, testing shows it's not that, so I am rather confused about what was going on.

> I'm not sure if I'm checking on the right place [1] but it seems that the
> Contrib and ContribInstall is executed after Check step which causes this test
> failure?

No, this is not failing in Check.

I did just notice a clue though: on snakefly, the failing step's
log [1] includes

make[1]: Leaving directory `/opt/postgres/build-farm-18/HEAD/pgsql.build/src/backend'
rm -rf '/opt/postgres/build-farm-18/HEAD/pgsql.build'/tmp_install
/usr/bin/mkdir -p '/opt/postgres/build-farm-18/HEAD/pgsql.build'/tmp_install/log
make -C '../../../..' DESTDIR='/opt/postgres/build-farm-18/HEAD/pgsql.build'/tmp_install install >'/opt/postgres/build-farm-18/HEAD/pgsql.build'/tmp_install/log/install.log 2>&1
make -j1 checkprep >>'/opt/postgres/build-farm-18/HEAD/pgsql.build'/tmp_install/log/install.log 2>&1
PATH="/opt/postgres/build-farm-18/HEAD/pgsql.build/tmp_install/opt/postgres/build-farm-18/HEAD/inst/bin:/opt/postgres/build-farm-18/HEAD/pgsql.build/src/test/modules/test_extensions:$PATH" LD_LIBRARY_PATH="/opt/postgres/build-farm-18/HEAD/pgsql.build/tmp_install/opt/postgres/build-farm-18/HEAD/inst/lib:$LD_LIBRARY_PATH" INITDB_TEMPLATE='/opt/postgres/build-farm-18/HEAD/pgsql.build'/tmp_install/initdb-template initdb --auth trust --no-sync --no-instructions --lc-messages=C --no-clean '/opt/postgres/build-farm-18/HEAD/pgsql.build'/tmp_install/initdb-template >>'/opt/postgres/build-farm-18/HEAD/pgsql.build'/tmp_install/log/initdb-template.log 2>&1
echo "# +++ regress check in src/test/modules/test_extensions +++" && PATH="/opt/postgres/build-farm-18/HEAD/pgsql.build/tmp_install/opt/postgres/build-farm-18/HEAD/inst/bin:/opt/postgres/build-farm-18/HEAD/pgsql.build/src/test/modules/test_extensions:$PATH" LD_LIBRARY_PATH="/opt/postgres/build-farm-18/HEAD/pgsql.build/tmp_install/opt/postgres/build-farm-18/HEAD/inst/lib:$LD_LIBRARY_PATH" INITDB_TEMPLATE='/opt/postgres/build-farm-18/HEAD/pgsql.build'/tmp_install/initdb-template ../../../../src/test/regress/pg_regress --temp-instance=./tmp_check --inputdir=. --bindir= --temp-config=/opt/postgres/build-farm-18/tmp/buildfarm-C9Iy3s/bfextra.conf --no-locale --port=5678 --dbname=contrib_regression test_extensions test_extdepend
# +++ regress check in src/test/modules/test_extensions +++
# initializing database system by running initdb

showing that the step made its own tmp_install, and that only the core
"install" process was executed, so the lack of amcheck in that install
tree is not surprising. But concurrent runs on other animals, eg [2],
don't show a tmp_install rebuild happening. So those are using an
installation tree that *does* include contrib modules.

So what this comes down to is "why is snakefly doing a fresh install
here?". I don't know the buildfarm client well enough to identify
probable causes. I do note that Makefile.global.in conditionalizes
tmp_install rebuild on several variables:

temp-install: | submake-generated-headers
ifndef NO_TEMP_INSTALL
ifneq ($(abs_top_builddir),)
ifeq ($(MAKELEVEL),0)
rm -rf '$(abs_top_builddir)'/tmp_install
$(MKDIR_P) '$(abs_top_builddir)'/tmp_install/log
$(MAKE) -C '$(top_builddir)' DESTDIR='$(abs_top_builddir)'/tmp_install install >'$(abs_top_builddir)'/tmp_install/log/install.log 2>&1
$(MAKE) -j1 $(if $(CHECKPREP_TOP),-C $(CHECKPREP_TOP),) checkprep >>'$(abs_top_builddir)'/tmp_install/log/install.log 2>&1

I think we've had trouble before with that MAKELEVEL test...

regards, tom lane

[1] https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=snakefly&dt=2025-03-20%2009%3A46%3A05&stg=module-test_extensions-check
[2] https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=alligator&dt=2025-03-19%2006%3A10%3A38&stg=module-test_extensions-check

Hi David, thanks for testing!

On Wed, Mar 19, 2025 at 2:29 PM David E. Wheeler <david(at)justatheory(dot)com> wrote:
>
> First thing I notice is that prefix= uses the magic to insert
> “postgresql” into the path if it’s not already there:
>
> ``` console
> ❯ make PG_CONFIG=~/dev/c/postgres/pgsql-devel/bin/pg_config prefix=/Users/david/Downloads install
> /opt/homebrew/bin/gmkdir -p '/Users/david/Downloads/share/postgresql/extension'
> /opt/homebrew/bin/gmkdir -p '/Users/david/Downloads/share/postgresql/extension'
> /opt/homebrew/bin/gmkdir -p '/Users/david/Downloads/share/doc//postgresql/extension'
> /opt/homebrew/bin/ginstall -c -m 644 .//pair.control '/Users/david/Downloads/share/postgresql/extension/'
> /opt/homebrew/bin/ginstall -c -m 644 .//sql/pair--0.1.2.sql .//sql/pair--unpackaged--0.1.2.sql '/Users/david/Downloads/share/postgresql/extension/'
> /opt/homebrew/bin/ginstall -c -m 644 .//doc/pair.md '/Users/david/Downloads/share/doc//postgresql/extension/‘
> ```
>
> I think this should at least be documented, but generally feels
> unexpected to me. I’ve attached a patch that fleshes out the docs,
> along with an example of setting `extension_control_path` and
> `dynamic_library_path` to use the locations. It might not have the
> information right about the need for “postgresql” or “pgsql” in the
> path.

Did you miss to attach the patch?

> Everything else works very nicely except for extensions that use the
> Makefile `MODULEDIR` variable to install all of the share files except
> the control file into a particular directory, and the `directory` in
> the control file so that the files can be found. Here’s semver[2],
> which has both:
>
> ```console
> ❯ make PG_CONFIG=~/dev/c/postgres/pgsql-devel/bin/pg_config prefix=/Users/david/Downloads/postgresql install
> /opt/homebrew/bin/gmkdir -p '/Users/david/Downloads/postgresql/share/extension'
> /opt/homebrew/bin/gmkdir -p '/Users/david/Downloads/postgresql/share/semver'
> /opt/homebrew/bin/gmkdir -p '/Users/david/Downloads/postgresql/lib'
> /opt/homebrew/bin/gmkdir -p '/Users/david/Downloads/postgresql/share/doc//semver'
> /opt/homebrew/bin/ginstall -c -m 644 .//semver.control '/Users/david/Downloads/postgresql/share/extension/'
> /opt/homebrew/bin/ginstall -c -m 644 .//sql/semver--0.10.0--0.11.0.sql .//sql/semver--0.11.0--0.12.0.sql .//sql/semver--0.12.0--0.13.0.sql .//sql/semver--0.13.0--0.15.0.sql .//sql/semver--0.15.0--0.16.0.sql .//sql/semver--0.16.0--0.17.0.sql .//sql/semver--0.17.0--0.20.0.sql .//sql/semver--0.2.1--0.2.4.sql .//sql/semver--0.2.4--0.3.0.sql .//sql/semver--0.20.0--0.21.0.sql .//sql/semver--0.21.0--0.22.0.sql .//sql/semver--0.22.0--0.30.0.sql .//sql/semver--0.3.0--0.4.0.sql .//sql/semver--0.30.0--0.31.0.sql .//sql/semver--0.31.0--0.31.1.sql .//sql/semver--0.31.1--0.31.2.sql .//sql/semver--0.31.2--0.32.0.sql .//sql/semver--0.32.0--0.32.1.sql .//sql/semver--0.32.1--0.40.0.sql .//sql/semver--0.32.1.sql .//sql/semver--0.40.0.sql .//sql/semver--0.5.0--0.10.0.sql .//sql/semver--unpackaged--0.2.1.sql .//sql/semver.sql '/Users/david/Downloads/postgresql/share/semver/'
> /opt/homebrew/bin/ginstall -c -m 755 src/semver.dylib '/Users/david/Downloads/postgresql/lib/'
> /opt/homebrew/bin/gmkdir -p '/Users/david/Downloads/postgresql/lib/bitcode/src/semver'
> /opt/homebrew/bin/gmkdir -p '/Users/david/Downloads/postgresql/lib/bitcode'/src/semver/src/
> /opt/homebrew/bin/ginstall -c -m 644 src/semver.bc '/Users/david/Downloads/postgresql/lib/bitcode'/src/semver/src/
> cd '/Users/david/Downloads/postgresql/lib/bitcode' && /opt/homebrew/Cellar/llvm/19.1.7_1/bin/llvm-lto -thinlto -thinlto-action=thinlink -o src/semver.index.bc src/semver/src/semver.bc
> /opt/homebrew/bin/ginstall -c -m 644 .//doc/semver.mmd '/Users/david/Downloads/postgresql/share/doc//semver/‘
> ```
>
> Following `MODULEDIR=semver`, it puts the SQL files into
> `share/semver/` instead of `share/extension/`, as expected, but then,
> even though the control file has `directory=semver`, it can’t load
> them:
>
> ```pgsql
> david=# create extension semver;
> ERROR: could not open directory "/Users/david/dev/c/postgres/pgsql-devel/share/semver": No such file or directory
> ```

I've managed to reproduce the issue. The problem is on get_ext_ver_list
which calls get_extension_script_directory that try to search for .sql
files only on $sharedir.

> Looks like it’s only looking in the `semver` subdirectory under
> $libdir and not the whole path.
>
> But given that the `directory` variable in the control file can be a
> full path, I don’t see that there’s much of a way to generalize a
> solution. I guess there are three options:
>
> 1. If directory is a full path, try to load the files there. It
> probably already works that way, though I haven’t tired it.
>
Yes, if the directory is a full path it try to load the files from
there. It is implemented on get_extension_script_directory.

> 2. If the directory is not a full path, check for it under each
> directory in `extension_control_path`? But no, that points to
> `share/extension`, not `share`, so it can’t really searched unless it
> also lops off `extension` from the end of each path.

Maybe we could make the "extension" part of the extension control path
explicitly, like Peter has mentioned in his first patch version [1]?.
If "directory" is not set we could use "extension" otherwise use the
"directory" as a path suffix when searching on extension_control_path?

[1] https://www.postgresql.org/message-id/0d384836-7e6e-4932-af3b-8dad1f6fee43%40eisentraut.org

--
Matheus Alcantara

On Mar 21, 2025, at 17:52, Matheus Alcantara <matheusssilv97(at)gmail(dot)com> wrote:

> Did you miss to attach the patch?

No. You can see it in the archive[1]. Direct link[2].

> Maybe we could make the "extension" part of the extension control path
> explicitly, like Peter has mentioned in his first patch version [1]?.
> If "directory" is not set we could use "extension" otherwise use the
> "directory" as a path suffix when searching on extension_control_path?

So, omit “extension” from the path options, append it to search for control files, and then append the directory value (if not absolute) if it exists to look for files, and otherwise append “extensions” to find them, too. I think that makes sense.

Essentially it becomes a SHAREDIR search path.

Best,

David

[1]: https://postgr.es/m/[email protected]
[2]: https://www.postgresql.org/message-id/attachment/174397/v1-0001-Flesh-out-docs-for-the-prefix-make-variable.patch

On 2025-03-21 Fr 12:42 PM, Tom Lane wrote:
> Matheus Alcantara<matheusssilv97(at)gmail(dot)com> writes:
>> On Thu, Mar 20, 2025 at 7:38 PM Andrew Dunstan<andrew(at)dunslane(dot)net> wrote:
>>>> (wondering if this another of these cases where the "path includes postgres" thing bites us, and we're looking in the wrong place)
>>> Nope, testing shows it's not that, so I am rather confused about what was going on.
>> I'm not sure if I'm checking on the right place [1] but it seems that the
>> Contrib and ContribInstall is executed after Check step which causes this test
>> failure?
> No, this is not failing in Check.
>
> I did just notice a clue though: on snakefly, the failing step's
> log [1] includes
>
> make[1]: Leaving directory `/opt/postgres/build-farm-18/HEAD/pgsql.build/src/backend'
> rm -rf '/opt/postgres/build-farm-18/HEAD/pgsql.build'/tmp_install
> /usr/bin/mkdir -p '/opt/postgres/build-farm-18/HEAD/pgsql.build'/tmp_install/log
> make -C '../../../..' DESTDIR='/opt/postgres/build-farm-18/HEAD/pgsql.build'/tmp_install install >'/opt/postgres/build-farm-18/HEAD/pgsql.build'/tmp_install/log/install.log 2>&1
> make -j1 checkprep >>'/opt/postgres/build-farm-18/HEAD/pgsql.build'/tmp_install/log/install.log 2>&1
> PATH="/opt/postgres/build-farm-18/HEAD/pgsql.build/tmp_install/opt/postgres/build-farm-18/HEAD/inst/bin:/opt/postgres/build-farm-18/HEAD/pgsql.build/src/test/modules/test_extensions:$PATH" LD_LIBRARY_PATH="/opt/postgres/build-farm-18/HEAD/pgsql.build/tmp_install/opt/postgres/build-farm-18/HEAD/inst/lib:$LD_LIBRARY_PATH" INITDB_TEMPLATE='/opt/postgres/build-farm-18/HEAD/pgsql.build'/tmp_install/initdb-template initdb --auth trust --no-sync --no-instructions --lc-messages=C --no-clean '/opt/postgres/build-farm-18/HEAD/pgsql.build'/tmp_install/initdb-template >>'/opt/postgres/build-farm-18/HEAD/pgsql.build'/tmp_install/log/initdb-template.log 2>&1
> echo "# +++ regress check in src/test/modules/test_extensions +++" && PATH="/opt/postgres/build-farm-18/HEAD/pgsql.build/tmp_install/opt/postgres/build-farm-18/HEAD/inst/bin:/opt/postgres/build-farm-18/HEAD/pgsql.build/src/test/modules/test_extensions:$PATH" LD_LIBRARY_PATH="/opt/postgres/build-farm-18/HEAD/pgsql.build/tmp_install/opt/postgres/build-farm-18/HEAD/inst/lib:$LD_LIBRARY_PATH" INITDB_TEMPLATE='/opt/postgres/build-farm-18/HEAD/pgsql.build'/tmp_install/initdb-template ../../../../src/test/regress/pg_regress --temp-instance=./tmp_check --inputdir=. --bindir= --temp-config=/opt/postgres/build-farm-18/tmp/buildfarm-C9Iy3s/bfextra.conf --no-locale --port=5678 --dbname=contrib_regression test_extensions test_extdepend
> # +++ regress check in src/test/modules/test_extensions +++
> # initializing database system by running initdb
>
> showing that the step made its own tmp_install, and that only the core
> "install" process was executed, so the lack of amcheck in that install
> tree is not surprising. But concurrent runs on other animals, eg [2],
> don't show a tmp_install rebuild happening. So those are using an
> installation tree that *does* include contrib modules.
>
> So what this comes down to is "why is snakefly doing a fresh install
> here?". I don't know the buildfarm client well enough to identify
> probable causes. I do note that Makefile.global.in conditionalizes
> tmp_install rebuild on several variables:
>
> temp-install: | submake-generated-headers
> ifndef NO_TEMP_INSTALL
> ifneq ($(abs_top_builddir),)
> ifeq ($(MAKELEVEL),0)
> rm -rf '$(abs_top_builddir)'/tmp_install
> $(MKDIR_P) '$(abs_top_builddir)'/tmp_install/log
> $(MAKE) -C '$(top_builddir)' DESTDIR='$(abs_top_builddir)'/tmp_install install >'$(abs_top_builddir)'/tmp_install/log/install.log 2>&1
> $(MAKE) -j1 $(if $(CHECKPREP_TOP),-C $(CHECKPREP_TOP),) checkprep >>'$(abs_top_builddir)'/tmp_install/log/install.log 2>&1

Good catch. This is happening because the owner hasn't updated the
animal to REL_19_1. In 19 and 19.1 we fixed detection of exiting
installs to take account of the 'Is there postgres or pgsql in the
prefix' issue. So it was looking in the wrong place.

cheers

andrew

--
Andrew Dunstan
EDB:https://www.enterprisedb.com

On Mar 19, 2025, at 13:29, David E. Wheeler <david(at)justatheory(dot)com> wrote:

> I think this should at least be documented, but generally feels unexpected to me. I’ve attached a patch that fleshes out the docs, along with an example of setting `extension_control_path` and `dynamic_library_path` to use the locations. It might not have the information right about the need for “postgresql” or “pgsql” in the path. Back in 2003[1] it was just “postgres”, but I couldn’t find the logic for it just now.

Here’s a rebase.

Best,

David

Re: David E. Wheeler
> +<programlisting>
> +make install prefix=/etc/postgresql

I'd use /usr/local/postgresql there. "/etc" is just wrong.

> +</programlisting>
> + This will install the control SQL files into
> + <literal>/etc/postgresql/share</literal> and shared modules into
> + <literal>/etc/postgresql/lib</literal>. If the prefix does not
> + include the strings <literal>postgresql</literal> or

Just "postgres", see src/Makefile.global.in:86.

> + <literal>pgsql</literal>, such as:
> +<programlisting>
> +make install prefix=/etc/extras

/usr/local/extras

> +</programlisting>
> + Then the <literal>postgresql</literal> directory will be appended io the
> + prefix, installing the control SQL files into

"the extension control and SQL files"

> + <literal>/etc/extras/postgresql/share</literal> and shared modules into

.../postgresql/share/extension because ...

> + <literal>/etc/extras/postgresql/lib</literal>. Either way, you'll need to
> + set <xref linkend="guc-extension-control-path"/> and <xref
> + linkend="guc-dynamic-library-path"/> to allow
> + <productname>PostgreSQL</productname> to find the files:
> +</programlisting>
> +extension_control_path = '/etc/extras/postgresql/share/extension:$system'

... it's used here.

Christoph

On Apr 25, 2025, at 07:33, Christoph Berg <myon(at)debian(dot)org> wrote:
>
> Re: David E. Wheeler
>> +<programlisting>
>> +make install prefix=/etc/postgresql
>
> I'd use /usr/local/postgresql there. "/etc" is just wrong.

Thank you for the review. Here’s v3*.

Best,

David

* Also reviewable as a GitHub PR[1].

[1]: https://github.com/theory/postgres/pull/10

On Apr 25, 2025, at 15:23, David E. Wheeler <david(at)justatheory(dot)com> wrote:

> Thank you for the review. Here’s v3*.

V4 removes “/extension” from the end of the `extension_control_path` value.

Best,

David

On Mon, Apr 28, 2025 at 6:15 PM David E. Wheeler <david(at)justatheory(dot)com> wrote:
>
> On Apr 25, 2025, at 15:23, David E. Wheeler <david(at)justatheory(dot)com> wrote:
>
> > Thank you for the review. Here’s v3*.
>
> V4 removes “/extension” from the end of the `extension_control_path` value.
>

It looks good to me. Just some a minor point:

+ Then the <literal>postgresql</literal> directory will be appended io the
Typo on "io"? Maybe "into" or "in"?

--
Matheus Alcantara

On Apr 29, 2025, at 09:56, Matheus Alcantara <matheusssilv97(at)gmail(dot)com> wrote:

> Typo on "io"? Maybe "into" or "in”?

Bah, yes, it’s “to”. Updated in v5 (and also a PR[1] for those who prefer that UX).

Best,

David

[1]: https://github.com/theory/postgres/pull/10

On 28.04.25 23:14, David E. Wheeler wrote:
> On Apr 25, 2025, at 15:23, David E. Wheeler <david(at)justatheory(dot)com> wrote:
>
>> Thank you for the review. Here’s v3*.
>
> V4 removes “/extension” from the end of the `extension_control_path` value.

The documentation in config.sgml says:

Note that the path elements should typically end in
<literal>extension</literal> if the normal installation layouts are
followed.

So I think your change here between v3 and v4 is incorrect.

On May 1, 2025, at 07:50, Peter Eisentraut <peter(at)eisentraut(dot)org> wrote:

> The documentation in config.sgml says:
>
> Note that the path elements should typically end in
> <literal>extension</literal> if the normal installation layouts are
> followed.
>
> So I think your change here between v3 and v4 is incorrect.

Right, sorry, forgot about that, I updated it in anticipation of Matheus’s patch[1] being committed.

So v3 is fine for now, but if that patch is committed, we’ll need to reconcile those docs.

Best,

David

[1]: https://postgr.es/m/CAFY6G8dUXHRii5rNy7V8WmBrmBwp9W7y3g+HL6Tn-Lu8KkvK=A@mail.gmail.com

On 01.05.25 16:31, David E. Wheeler wrote:
> On May 1, 2025, at 07:50, Peter Eisentraut <peter(at)eisentraut(dot)org> wrote:
>
>> The documentation in config.sgml says:
>>
>> Note that the path elements should typically end in
>> <literal>extension</literal> if the normal installation layouts are
>> followed.
>>
>> So I think your change here between v3 and v4 is incorrect.
>
> Right, sorry, forgot about that, I updated it in anticipation of Matheus’s patch[1] being committed.
>
> So v3 is fine for now, but if that patch is committed, we’ll need to reconcile those docs.

I see. I have committed it now describing the current state.

Btw., the shown directory names that illustrate how "postgresql" is
appended were not correct. I have corrected that.

On May 1, 2025, at 16:24, Peter Eisentraut <peter(at)eisentraut(dot)org> wrote:

> I see. I have committed it now describing the current state.
>
> Btw., the shown directory names that illustrate how "postgresql" is appended were not correct. I have corrected that.

Thank you. I gotta say I find them confusing TBH.

Best,

David

On May 1, 2025, at 16:24, Peter Eisentraut <peter(at)eisentraut(dot)org> wrote:

> I see. I have committed it now describing the current state.

Quick follow-up to tweak a couple of commas.

--- a/doc/src/sgml/extend.sgml
+++ b/doc/src/sgml/extend.sgml
@@ -1813,8 +1813,8 @@ include $(PGXS)

<para>
You can select a separate directory prefix in which to install your
- extension's files, by setting the <command>make</command> variable
- <varname>prefix</varname> when executing <literal>make install</literal>
+ extension's files by setting the <command>make</command> variable
+ <varname>prefix</varname> when executing <literal>make install</literal>,
like so:
<programlisting>
make install prefix=/usr/local/postgresql

Best,

David