On Wed, 26 Jun 2024 at 00:32, David E. Wheeler <da...@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 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
