Thanks for the new version!
I tested v3 and all the previous comments are addressed. CREATE FUNCTION
and DO with an unknown language now report the original error with
'$system:', ':' and '$system:rel', and the hint is still shown when a
remaining directory contains the extension.
Regarding the backpatch, I don't think that having a different behaviour
on master and on the back branches is a good idea. If '$system:' raises
an error on 20 but is silently ignored on 18 and 19, the same setting
behaves differently depending on the version, which seems confusing to
me, especially for a parameter that was only introduced on 18. I think
that we should either raise the error on all branches or silently ignore
the empty elements on all branches, and then backpatch whichever we
choose. (CCing Peter so we can get a committer thought on this).
This is also related to the "directory" documentation thread [1] that
you previously mention, where the '$system/foo' case came up.
Mahendrakar noticed there that with '$system/foo:$system'
pg_available_extensions lists the extensions from $system, while CREATE
EXTENSION fails on any of them, and asked whether a check hook to reject
such elements at SET time would be worth pursuing. With v3-0002 both now
fail consistently:
SET extension_control_path = '$system/foo:$system';
SELECT count(*) FROM pg_available_extensions;
ERROR: component in parameter "extension_control_path" is not an absolute
path
So the inconsistency seems to be fixed here. But his concern about the
check hook also applies to 0002: a value like '$system:$system/foo'
works today for any extension found in $system, and with 0002 it fails
on every extension lookup. The only difference with a check hook would
be that the error shows up when the value is set (or when the server
starts) instead of when the path is used. So I think that whatever we
decide for the backpatch should also cover 0002.
Some small comments on v3:
1: On 0002, the location is allocated before the absolute path check,
so the skip branch needs to free it again. Since location->macro is
only set for $system, which always expands to an absolute path, the
pfree(location->macro) can't be reached. Maybe we could build and check
the mangled path first and only allocate the location when the element
is kept?
2: The USE_ASSERT_CHECKING block on find_in_paths() seems a bit too
much to me. Now that it's static and only used with the output of
get_extension_control_directories(), I think that
Assert(is_absolute_path(path)) and the header comment are enough.
3: The 0002 commit message still only mentions replacing the check in
find_in_paths() with an assertion. It would be good to also mention
that find_in_paths() is now static, that system_dir is canonicalized,
and that skip_invalid_path now also skips relative elements.
[1]
https://postgr.es/m/cabkiuwpgilzdg4d8zrbj6dhnwyghqkfxz2gdstxgeodzh6b...@mail.gmail.com
--
Matheus Alcantara
EDB: https://www.enterprisedb.com
