> On Nov 8, 2026, at 12:37, surya poondla <[email protected]> wrote: > > Hi Chao, > > I tested v1 on 0607aa61090, and did some extra code reading for this. A few > comments:
Thank you very much for the review.
>
> 1. The new DEBUG3 message exposes the expanded paths to non-superusers. Since
> f3c9e341cdf commit, pg_available_extensions shows <insufficient privilege> as
> the location to them, and since your db5ed03217b commit it shows $system
> rather than the share path even to superusers. The GUC is also
> GUC_SUPERUSER_ONLY. But any role can SET client_min_messages = debug3 and see
> every directory, including the real share path. As an ordinary role (without
> pg_read_all_settings):
>
> SHOW extension_control_path;
> ERROR: permission denied to examine "extension_control_path"
>
> SET client_min_messages = debug3;
> SELECT count(*) FROM pg_available_extensions;
> DEBUG: get_extension_control_directories: path "/opt/ext/extension"
> DEBUG: get_extension_control_directories: path
> "/usr/local/pgsql/share/extension"
>
> find_in_path() has a similar DEBUG3 for dynamic_library_path, but
> non-superusers rarely reach it, whereas here any query on the views does. I
> would drop the elog.
Ah, good catch. I actually reviewed f3c9e341cdf, but I had completely forgotten
about that patch.
>
> 2. In the original extension_control_path commit (4f7f7b03758), CREATE
> EXTENSION searched with find_in_path(), which errors out when the search
> reaches an empty element ("zero-length component in parameter", or "is not an
> absolute path" for a trailing one). Commit 81eaaa2c41d replaced that with
> find_in_paths(), which kept only the absolute-path check, and the same
> commit's "%s/extension" suffix is what turns an empty element into
> /extension. Rather than skipping silently, I would rather report the mistake,
> at least on master. The check would have to go in
> get_extension_control_directories() (or a check hook), since find_in_paths()
> only sees the resulting "/extension" and can't tell it from "/". That would
> also make the views and CREATE EXTENSION agree, which they haven't since
> 4f7f7b03758; otherwise ':' silently lists nothing while '' means $system. It
> is a behaviour change, though, the views, and extension_file_exists() (used
> for the CREATE FUNCTION unknown-language hint), would then error out on such
> a setting, so a silent skip may be the better fit for back branches.
>
I’m okay with raising an error for empty path elements. That would let us
remove the DEBUG3 logging and resolve point 1 as well. I added the logging
mainly for testing purposes.
My feeling is that this patch might not need to be back-patched.
> 3. As superuser, a relative element is shown by pg_available_extensions
> (resolved against the data directory), but CREATE EXTENSION rejects it in
> find_in_paths() once the search reaches it:
>
> SET extension_control_path = '$system:rel';
> SELECT name, location FROM pg_available_extensions WHERE name = 'relext';
> name | location
> --------+---------------
> relext | rel/extension
>
> CREATE EXTENSION relext;
> ERROR: component in parameter "extension_control_path" is not an
> absolute path
>
> The same place as in (2) could reject both.
>
Thanks for reporting that. I hadn’t noticed the problem. As this is a separate
issue, I put the fix in 0002.
> 4. I'm not convinced 0002 is needed. The saving is one readdir() per repeated
> element in the views, for a setting I don't expect to be common. The location
> checks in the new tests already pass on master, because the views deduplicate
> by extension name.
>
As I mentioned in the original patch email, this change is of relatively low
value, but I think it’s also harmless. For example, if an extension directory
is on network storage, avoiding a duplicate directory scan would be preferable.
So I’d leave it as 0003 for now and wait for more opinions.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
v2-0001-Reject-empty-elements-in-extension_control_path.patch
Description: Binary data
v2-0002-Reject-relative-paths-consistently-in-extension_c.patch
Description: Binary data
v2-0003-Deduplicate-extension-control-search-directories.patch
Description: Binary data
