Feedback-ID: ie0a040ee:Fastmail
Message-ID: <7c178ff3-20a5-493e-a704-e6b3263e782f@eisentraut.org>
Date: Tue, 11 Mar 2025 16:58:55 +0100
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: RFC: Additional Directory for Extensions
To: Matheus Alcantara <matheusssilv97@gmail.com>
Cc: Andrew Dunstan <andrew@dunslane.net>,
 Gabriele Bartolini <gabriele.bartolini@enterprisedb.com>,
 Craig Ringer <craig.ringer@enterprisedb.com>,
 "David E. Wheeler" <david@justatheory.com>,
 Robert Haas <robertmhaas@gmail.com>, Alvaro Herrera
 <alvherre@alvh.no-ip.org>,
 PostgreSQL-development <pgsql-hackers@postgresql.org>
References: 
 <CA+TgmoaMk==vOUmWwJ5ZA-EueNqa9RQaWMEiHCF1ZKW_YCcj8g@mail.gmail.com>
 <B86AF59F-5B65-4CC9-91A4-B79A089C2876@justatheory.com>
 <CAGRY4ny=Oy-F0qCRFQa=XtEqyUbMygvbqqKahAvbae7v4LTOrQ@mail.gmail.com>
 <AF4A50CD-C5CF-406D-9700-1B889F40E8FB@justatheory.com>
 <CAGRY4nx8Fmea4udk--PKdvfgpNdqC+rcueAmGwuw2PD9dv+zqQ@mail.gmail.com>
 <D30A91FA-A6D4-4737-941F-0BBB2984B730@justatheory.com>
 <0d384836-7e6e-4932-af3b-8dad1f6fee43@eisentraut.org>
 <4f7209ba-1ca2-475e-9a70-0f3fe0be6378@eisentraut.org>
 <CAFY6G8dzuLXFjONX5E-TvyD5E9r21b5vp1My_nMcqdGD0VYfYw@mail.gmail.com>
 <12312928-5516-49b8-81c1-a7b56fd01bc8@dunslane.net>
 <CAFY6G8dGcv1D8tCkjrz+9DvAYG9h4U=hBm0VXjJEXExy44hauQ@mail.gmail.com>
 <CAFY6G8eOMkGXLqzUz=qXrwt4X_HvmVZ13570DAbgAiw+3q03DA@mail.gmail.com>
 <CAFY6G8fV5FTqn7p8pxbvPYUD5+ffik-J50GChjKH2Gg7d=fRPg@mail.gmail.com>
 <38c53446-6b65-4222-b76b-351e56f4841c@eisentraut.org>
 <CAFY6G8cGeUV0f5K8v-Du0ts3iZyRE6Q5dNtYjQq8cjS4epLX5A@mail.gmail.com>
Content-Language: en-US
From: Peter Eisentraut <peter@eisentraut.org>
In-Reply-To: 
 <CAFY6G8cGeUV0f5K8v-Du0ts3iZyRE6Q5dNtYjQq8cjS4epLX5A@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Archived-At: 
 <https://www.postgresql.org/message-id/7c178ff3-20a5-493e-a704-e6b3263e782f%40eisentraut.org>
Precedence: bulk

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.