Thread: [MASSMAIL]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
whenloading modules that back functions. The location is made configurable to allow build-time testing of extensions
thatdo 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
thispatch into the core. And not only because it’s overhead for package maintainers like Christoph, but because a
numberof 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
createa 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
Dockerimages can keep extensions they install separate from core extensions, in a directory mounted to a persistent
volumewith none of the core extensions. Along with tweaking dynamic_library_path to support additional directories for
sharedobject libraries, which can also be mounted to a separate path, we can have a persistent and clean separation of
immutablecore 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
stillallow 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
makingpkglibdir configurable  me optimistic that this might be the right time to support additional configuration for
theextension 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.8010600@ocharles.org.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@alvh.no-ip.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@technowledgy.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
thispath 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
theperson who compiled it thought would make sense. 

Best,

David

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

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

Or SIGHUP?

D

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

> Yes, I like the suggestion to make it require a restart, which lets the sysadmin control it and not limited to
whateverthe 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
justthose 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
theextension directory, where it would be loaded in preference to the core libpgtypes library, if they couldn’t exploit
theoriginal. 

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
wouldenable much tidier extension installation: The control file, SQL scripts, and DSOs could all be in a single
directoryfor 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
whateverthe 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
justthose 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
intothe extension directory, where it would be loaded in preference to the core libpgtypes library, if they couldn’t
exploitthe 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
wouldenable much tidier extension installation: The control file, SQL scripts, and DSOs could all be in a single
directoryfor 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@debian.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
haveto 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
wouldenable much tidier extension installation: The control file, SQL scripts, and DSOs could all be in a single
directoryfor 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
washoping to find a path where it knows it’s opening a DSO for the purpose of an extension, so we could limit the
lookupthere. 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@debian.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
SHAREDIRto 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
becausewe 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@justatheory.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
pointersto address concerns about providing a second directory for extensions and DSOs. Quite a few people have talked
aboutthe need for this in the Extension Mini Summits[1], so I’m sure I could get some collaborators to make
improvementsor 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@alvh.no-ip.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@gmail.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
postmasterwhat directories to read files from. It feels a little outside the scope of a regular user to even be aware
ofthe 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
thispatch 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
andany other SHAREDIR files an extension might include. But also, I think it should support all the pg_config
installationtargets 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@justatheory.com> wrote:
> I guess the question then is what security controls are appropriate for this feature, which after all tells the
postmasterwhat directories to read files from. It feels a little outside the scope of a regular user to even be aware
ofthe 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
andany other SHAREDIR files an extension might include. But also, I think it should support all the pg_config
installationtargets 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@gmail.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
putsitems 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@justatheory.com> wrote:
> I realize this probably isn’t going to happen for 17, given the freeze, but I would very much welcome feedback and
pointersto address concerns about providing a second directory for extensions and DSOs. Quite a few people have talked
aboutthe need for this in the Extension Mini Summits[1], so I’m sure I could get some collaborators to make
improvementsor 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@gmail.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@jeltef.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@justatheory.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
(PGXSputs items listed in DOCS into DOCDIR). Some might also produce man pages (for their binaries), HTML docs, and
otherstuff. 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, Jun 25, 2024 at 6:12 AM Alvaro Herrera <alvherre@alvh.no-ip.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@jeltef.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/16and 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
andwhere the postmaster looks for specific files. And PGXS already supports installing files in these locations, thanks
toits 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
installingand managing extensions. And maybe that’s fine for now, as it can still be used to address the immutability
problemsdescried 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,
mightbe a separate thread. 

Best,

David

On Jun 25, 2024, at 10:43 AM, Robert Haas <robertmhaas@gmail.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
thingto 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@justatheory.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
thingto 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@justatheory.com> wrote:
>
> On Jun 24, 2024, at 5:32 PM, Jelte Fennema-Nio <postgres@jeltef.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@justatheory.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

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.bartolini@enterprisedb.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—thisincludes the installation of Postgres extensions and their libraries. This restriction prevents us from
addingextensions on the fly, requiring them to be included in the main PostgreSQL operand image. As a result, users who
needspecific 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
ephemeralvolume that contains the necessary extensions (namely $sharedir and $pkglibdir from pg_config). These volumes
wouldbe created and populated with the required extensions when the container starts and destroyed when it shuts down.
Tomake this work, each extension must be independently packaged as a container image containing the appropriate files
fora 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.ringer@enterprisedb.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.

On Thu, 22 Aug 2024 at 21:00, Gabriele Bartolini
<gabriele.bartolini@enterprisedb.com> wrote:
> On Thu, 22 Aug 2024 at 09:32, Jelte Fennema-Nio <postgres@jeltef.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.bartolini@enterprisedb.com> wrote:
>
> Hi Jelte,
>
> On Thu, 22 Aug 2024 at 09:32, Jelte Fennema-Nio <postgres@jeltef.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.ringer@enterprisedb.com> wrote:
> On Thu, 22 Aug 2024 at 21:00, Gabriele Bartolini
> <gabriele.bartolini@enterprisedb.com> wrote:
> > On Thu, 22 Aug 2024 at 09:32, Jelte Fennema-Nio <postgres@jeltef.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.ringer@enterprisedb.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-basedstructure for extensions, where every file for an extension is in a directory named for the extension,
andsubdirectories 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
potentiallyallow for multiple versions of an extension to be installed at once, if we required the version to be part
ofthe directory name. 

I think this would be a much nicer layout for packaging, installing, and managing extensions versus the current method
ofstrewing files around to a slew of different directories. But it would come at some cost, in terms of backward with
theexisting 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,
solvingmost of the integration problems (immutable servers, packaging testing, etc.) at the cost of a slightly
unexpecteddirectory layout. What I mean by that is that the current patch is pretty much just using extension_destdir
asa prefix to all of those directories from pg_config, so they never have to change, but it does mean that you end up
installingextensions 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@justatheory.com> wrote:
> On Aug 21, 2024, at 19:07, Craig Ringer <craig.ringer@enterprisedb.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-basedstructure for extensions, where every file for an extension is in a directory named for the extension,
andsubdirectories 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
methodof 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
modificationof 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,
solvingmost of the integration problems (immutable servers, packaging testing, etc.) at the cost of a slightly
unexpecteddirectory layout. What I mean by that is that the current patch is pretty much just using extension_destdir
asa prefix to all of those directories from pg_config, so they never have to change, but it does mean that you end up
installingextensions 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

On Aug 27, 2024, at 04:56, Gabriele Bartolini <gabriele.bartolini@enterprisedb.com> wrote:

> The extension image could follow a naming convention like this (order can be adjusted): `<extension name>-<pg
major>-<extensionversion>-<distro>(-<seq>)`. For example, `pgvector-16-0.7.4-bookworm-1` would represent the first
imagebuilt 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
versionincluded in the folder name—for example, `pgvector-0.7.4` instead of just `pgvector`. However, I'm a bit rusty
withthe 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
directoryname. 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.ringer@enterprisedb.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
patchis just a prefix for various PGXS/pg_config directories; the longer-term proposal I’ve made here is not a prefix
forsharedir, mandir, etc., but a directory that contains directories named for extensions. So even if we were to take
thisapproach, 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
proposaldoesn’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@justatheory.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
containsdirectories 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
myproposal 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.ringer@enterprisedb.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
justPGXS, 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
theright thing. Maybe for the new version of the server with the proposed new pattern it would dispatch to the new
thingsomehow 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
anda 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
teamand committers are on board with the general idea of search paths and per-extension directory organization, it
wouldbe 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
myproposal 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@gmail.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-basedextension packaging, as promised up-thread. 

Best,

David

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

On Nov 11, 2024, at 02:16, Peter Eisentraut <peter@eisentraut.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
exactlythe 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
searchingfor any file in any path.  Once a control file is found, the other extension support files (script files and
auxiliarycontrol 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
ofthe 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
worktoo. 

In the broader extension organization RFC I’ve been working up[1], I propose a new Makefile prefix for the destination
directoryfor an extension. It hinges on the idea that an extension has all of its files organized in a directory with
theextension 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"
partimplicit. 

+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
thatway.  The obvious solution is that extensions change this to just 'foo'.  But this will require a lot updating work
formany 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
intowhich it’s being installed right now?”. Doesn’t seem necessary to search a path if the specific location is set at
installtime. 

> Maybe we could devise some sort of rule that if the extension control file is found via the path outside of $system,
thenthe 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
foran extension, then once you’ve found the control files all the other files are there, too. But `$libdir` is
presumablystill meaningful, since the first time you call an extension function in a new connection, it just tries to
loadthat 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
filelocation along the lines of the other thread[2] and the RFC I’m working up based on them both[1], especially since
thereare 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
simplymounting 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
searchingfor any file in any path.  Once a control file is found, the other extension support files (script files and
auxiliarycontrol 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
ofthe 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
thatway.  The obvious solution is that extensions change this to just 'foo'.  But this will require a lot updating work
formany 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
intowhich it’s being installed right now?”. Doesn’t seem necessary to search a path if the specific location is set at
installtime.
 

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
filelocation along the lines of the other thread[2] and the RFC I’m working up based on them both[1], especially since
thereare 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@eisentraut.org> wrote:

> No, you can also install them into a common directory and mount that one.  For example, you install the extension at
buildtime into /tmp/foo/{lib,share/extension}, you package that up as a disk image, mount it at /opt/extensions/myext,
andthen 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
intowhich it’s being installed right now?”. Doesn’t seem necessary to search a path if the specific location is set at
installtime. 
>
> No, this is not set at build or install time.  This is for typical extensions hardcoded, and $libdir is resolved by
thePostgreSQL 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
thatway.  The obvious solution is that extensions change this to just 'foo'.  But this will require a lot updating work
formany 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_PATHNAMEreplacement 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
filelocation along the lines of the other thread[2] and the RFC I’m working up based on them both[1], especially since
thereare 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
doubtit’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@justatheory.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
thatway.  The obvious solution is that extensions change this to just 'foo'.  But this will require a lot updating work
formany 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
theMODULE_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
thatway.  The obvious solution is that extensions change this to just 'foo'.  But this will require a lot updating work
formany 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
theMODULE_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@eisentraut.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
latterstyle, 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