Re: Include extension path on pg_available_extensions

On Tue Oct 28, 2025 at 5:56 PM -03, Euler Taveira wrote:
> On Tue, Oct 28, 2025, at 9:29 AM, Matheus Alcantara wrote:
>> So here it is, see attached.
>>
>
> I took another look at this patch.
>
Thanks for reviewing!

> ! This adds a new "location" column on pg_available_extensions and
> ! pg_available_extension_versions views to show the path of locations that
> ! Postgres is seeing based on the extension_control_path GUC.
>
> Maybe the sentence above can be written in a different way such as
>
> Add a new "location" column to pg_available_extensions and
> pg_available_extension_versions views. It exposes the directory that the
> extension is located.
>
Sounds better, fixed.

> ! User configured paths is only visible for users that has the
> ! pg_read_extension_paths role, otherwise <insufficient privilege> is
> ! returned as a column value, the same behaviour that we already have on
> ! pg_stat_activity.
>
> s/User configured paths is/User-defined locations are/
>
Fixed.

> +typedef struct
> +{
> + char *macro;
> + char *loc;
> +} Location;
>
> Location is a generic name. I would use something like ExtensionLocation or
> ExtLocation or ExtControlPath. Do you really need a struct here? You are
> storing the same element (location) in both members. Couldn't you use a single
> string to represent the location (with and without the macro)?
>
ExtensionLocation sounds good, fixed for this.

I think that the struct is necessary because we use the "loc" field for
other things other than just printing on "location" column results. For
instance, on pg_available_extension_versions() we get the list of
ExtensionLocation's and use the "loc" field to call AllocateDir() and
ReadDir() and then we pass the location pointer to
get_available_versions_for_extension() that it will decide if it should
use the "loc" or the "macro" field by calling the location_to_display().
So I don't think that we can use a single string to represent the
location because we may use the raw location or the macro value
depending on the case.

> +/*
> + * Return the location to display for the given Location based on the user
> + * privileges. If the user connected to the database don't have the
> + * permissions <insufficient privilege> is returned.
> + */
> +static char *
> +location_to_display(Location *loc)
> +{
>
> Could you improve this comment?
>
Fixed.

> Return the extension location. If the current user doesn't have sufficient
> privileges, don't show the location. You could also rename this function
> (something like get_extension_location). Since has_privs_of_role returns a
> bool, you can simplify the code and call it directly into the if block. You can
> also use GetUserId() directly instead of declaring another variable.
>
Fixed.

> +{ oid => '6434', oid_symbol => 'ROLE_PG_READ_EXTENSION_PATHS',
> + rolname => 'pg_read_extension_paths', rolsuper => 'f', rolinherit => 't',
> + rolcreaterole => 'f', rolcreatedb => 'f', rolcanlogin => 'f',
> + rolreplication => 'f', rolbypassrls => 'f', rolconnlimit => '-1',
> + rolpassword => '_null_', rolvaliduntil => '_null_' },
>
> I'm not convinced that we need a new predefined role just to control if it can
> expose the extension location. Should it return the location only for
> superusers? Can't one of the current predefined roles be used? If it doesn't
> fit, you should probably use a generic name so this new predefined role can be
> used by other extension-related stuff in the future.
>
Yeah, I think that we can limit this only for superusers. Fixed.

> @@ -43,31 +51,63 @@ is($ecp, "\$system$sep$ext_dir$sep$ext_dir2",
> $node->safe_psql('postgres', "CREATE EXTENSION $ext_name");
> $node->safe_psql('postgres', "CREATE EXTENSION $ext_name2");
>
> +
> my $ret = $node->safe_psql('postgres',
> "select * from pg_available_extensions where name = '$ext_name'");
>
> Remove this new line.
>
Fixed.

> Adding more tests is always a good thing. However, in this case, we can
> simplify the tests. The current queries already cover the
> get-the-extension-location case. If you add an additional test showing the
> insufficient privilege case, that's fine. The other tests are basically
> exercising the permission system.
>
Fixed.

> Documentation is missing. These views are documented in system-views.sgml.
>
Fixed

--
Matheus Alcantara