Re: extension_control_path: ignore empty elements and deduplicate directories

Hi,

Thanks for sharing the bug and the patch!

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.

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.

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.

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. 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.

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.

--
Matheus Alcantara
EDB: https://www.enterprisedb.com