On 10.03.25 21:25, Matheus Alcantara wrote:
> On Thu, Mar 6, 2025 at 10:46 AM Peter Eisentraut <peter@eisentraut.org> wrote:
>> This looks very good to me. I have one issue to point out: The logic
>> in get_extension_control_directories() needs to be a little bit more
>> careful to align with the rules in find_in_path(). For example, it
>> should use first_path_var_separator() to get the platform-specific path
>> separator, and probably also substitute_path_macro() and
>> canonicalize_path() etc., to keep everything consistent.
>>
> I fixed this hardcoded path separator issue on the TAP test and forgot
> to fix it also on code, sorry, fixed on this new version.
>> (Maybe it would be ok to move the function to dfmgr.c to avoid having
>> to export too many things from there.)
>>
> I've exported substitute_path_macro because adding a new function on
> dfmgr would require #include nodes/pg_list.h and I'm not sure what
> approach would be better, please let me know what you think.
Yes, that structure looks ok. But you can remove one level of block in
get_extension_control_directories().
I found a bug that was already present in my earlier patch versions:
@@ -423,7 +424,7 @@ find_extension_control_filename(ExtensionControlFile
*control)
ecp = Extension_control_path;
if (strlen(ecp) == 0)
ecp = "$system";
- result = find_in_path(basename, Extension_control_path,
"extension_control_path", "$system", system_dir);
+ result = find_in_path(basename, ecp, "extension_control_path",
"$system", system_dir);
Without this, it won't work if you set extension_control_path empty.
(Maybe add a test for that?)
I think this all works now, but I think the way
pg_available_extensions() works is a bit strange and inefficient. After
it finds a candidate control file, it calls
read_extension_control_file() with the extension name, that calls
parse_extension_control_file(), that calls
find_extension_control_filename(), and that calls find_in_path(), which
searches that path again!
There should be a simpler way into this. Maybe
pg_available_extensions() should fill out the ExtensionControlFile
structure itself, set ->control_dir with where it found it, then call
directly to parse_extension_control_file(), and that should skip the
finding if the directory is already set. Or something similar.
