Re: extension_control_path: ignore empty elements and deduplicate directories

> On Nov 10, 2026, at 04:03, Matheus Alcantara <matheusssilv97(at)gmail(dot)com> wrote:
>
> Hi,
>
> Thanks for sharing the bug and the patch!

Thank you very much for the review.

>
> I tested v2 and the 0001 and 0002 tests fail on master and pass with the
> patches. With '$system:', ':$system', ':' and '$system:rel' both the
> views and CREATE EXTENSION now report the same error, and '' still means
> $system.
>
> I have some comments:
>
> 1: The error is only raised when the path is used
>
> Since the check is done in get_extension_control_directories(), an
> invalid value is accepted without any complaint and the error only
> shows up later, on whatever happens to read the path:
>
> ALTER SYSTEM SET extension_control_path = '$system:';
> SELECT pg_reload_conf();
>
> -- later, on any session and with any role
> SELECT count(*) FROM pg_available_extensions;
> ERROR: empty element in parameter "extension_control_path"
>
> This matches dynamic_library_path, which also accepts an empty element
> on SET/ALTER SYSTEM and only complains when a library is loaded, so I'm
> fine with it. A check hook could reject the value earlier, but an
> invalid value already on postgresql.conf would then prevent the server
> from starting, so I don't think it's worth it.
>
> However, there is one case that is specific to extension_control_path.
> It breaks CREATE FUNCTION and DO with a language that doesn't exist,
> because extension_file_exists() is called to build the hint of the
> "language does not exist" error, so the error from the path replaces the
> original one:
>
> SET extension_control_path = '$system:';
> CREATE FUNCTION f() RETURNS int LANGUAGE nosuchlang AS 'x';
> ERROR: empty element in parameter "extension_control_path"
>
> while on master the same command reports:
>
> ERROR: language "nosuchlang" does not exist
>
> I think that extension_file_exists() should not replace the original
> error here, since it's only used to decide whether to add a hint.

Thanks for pointing out this use case. In v3-0001, I added a bool parameter, skip_invalid_path, to get_extension_control_directories(), so that extension_file_exists() can skip empty and relative path elements and preserve the original unknown-language error and any applicable hint. I also added a new test to 0001 based on your example.

>
> 2: Error message on 0001
>
> dfmgr.c already has "zero-length component in parameter \"%s\"" for
> dynamic_library_path. Before 81eaaa2c41d, when CREATE EXTENSION still
> used find_in_path(), that was the error for a leading or repeated
> separator (a trailing one got "is not an absolute path" instead). Maybe
> we can reuse it for all the empty element cases? It would keep both
> parameters consistent and avoid a new error message.

Updated the error message in v3-0001.

>
> 3: Behaviour change on 0001 and 0002
>
> A setting with a trailing separator, like '$system:', works on master,
> since /extension usually doesn't exist and is just skipped. With 0001 it
> becomes an error on every extension lookup. The same happens with 0002
> for a relative element after a directory that already contains the
> extension, which is the case covered by the new test. I think that's the
> right behaviour, but since it can break a setting that works today on
> 18, I'm wondering how the backpatching would work on this case. This
> behavior that you shared on this thread doesn't seems right to me, but
> fixing only on master doesn't seens enough to me since we will have
> difference behaviours across the versions.

I chose to silently discard empty path elements in v1 precisely with back-patching in mind. But I also thought Surya’s suggestion to raise an error for empty path elements was reasonable, so I changed the behavior in v2. As I mentioned in my earlier email, my feeling is that this behavior change doesn’t need to be back-patched.

If committers decide to back-patch, perhaps we could silently discard empty path elements on released branches? In that case, I can help prepare the back-patches.

>
> 4: find_in_paths() cleanup on 0002
>
> With 0002 every location is canonicalized and checked to be absolute
> when the list is built, so the pstrdup(), canonicalize_path() and
> pfree() calls in find_in_paths() are redundant, and the loop could use
> location->loc directly.

In v2, find_in_paths() still calls canonicalize_path() because system_dir has not been canonicalized in get_extension_control_directories(). Although get_share_path() canonicalizes sharepath, if sharepath is the root directory (/), psprintf("%s/extension", sharepath) introduces a duplicate slash, so system_dir still needs to be canonicalized.

I’ve now made find_in_paths() static and added canonicalization of system_dir to get_extension_control_directories(), allowing us to remove the redundant string operations from find_in_paths(). Done in v3-0001.

> Also, unrelated to this patch, find_in_paths()
> is not static and has a local prototype on extension.c, although it's
> only used there. Since we are touching this code, maybe it's worth
> making it static too.
>

Made find_in_paths() static in v3-0002.

> About 0003, I don't think it's needed. I think that listing the same
> directory more than once is a user mistake, and the result is still
> correct, since the views deduplicate by extension name and CREATE
> EXTENSION uses the first match. I don't think we should add code to work
> around this kind of mistake.

As two reviewers considered 0003 is not necessary, let me drop it for now.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/