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
