On 10.03.25 21:25, Matheus Alcantara wrote:
On Thu, Mar 6, 2025 at 10:46 AM Peter Eisentraut <pe...@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.
