On Tue, Mar 11, 2025 at 12:59 PM Peter Eisentraut <peter@eisentraut.org> wrote:
> Yes, that structure looks ok. But you can remove one level of block in
> get_extension_control_directories().
>
Sorry, missed during debugging. Fixed
> 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?)
>
Fixed, and also added a new test case for this.
> 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.
>
Good catch. I fixed this by creating a new function to construct the
ExtensionControlFile and changed the pg_available_extensions to set the
control_dir. The read_extension_control_file was also changed to just
call this new function constructor. I implemented the logic to check if
the control_dir is already set on parse_extension_control_file because
it seems to me that make more sense to not call
find_extension_control_filename instead of putting this logic there
since we already set the control_dir when we find the control file, and
having the logic to set the control_dir or skip the find_in_path seems
more confusing on this function instead of on
parse_extension_control_file. Please let me know what you think.
--
Matheus Alcantara
