On Tue, Mar 11, 2025 at 12:59 PM Peter Eisentraut <pe...@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
v7-0001-extension_control_path.patch
Description: Binary data
