Обсуждение: Include extension path on pg_available_extensions
Hi all,
On [1] it was mentioned that it could be a good idea to include the
extension location when listening the available extensions on
pg_available_extensions to make it clear to the user the location of an
extension that Postgres is seeing based on the extension_control_path
GUC.
The attached patch implements this idea. Extensions installed on $system
path will not show the actual value of the $system macro and it will
show the macro itself, for example:
postgres=# show extension_control_path;
extension_control_path
---------------------------------------------------
/usr/local/my/extensions/share/postgresql:$system
(1 row)
postgres=# select * from pg_available_extensions;
name | default_version | installed_version | comment |
location
---------+-----------------+-------------------+--------------------------------------------------+---------------------------------------------------
envvar | 1.0.0 | | Get the value of a server environment variable |
/usr/local/my/extensions/share/postgresql/extension
amcheck | 1.5 | | functions for verifying relation integrity | $system
bloom | 1.0 | | bloom access method - signature file based index | $system
I'm not sure if this should be included on 18 release since this is not
a bug fix but an improvement on the extension system by itself.
Any opinions on this?
[1] https://www.postgresql.org/message-id/CAKFQuwbR1Fzr8yRuMW%3DN1UMA1cTpFcqZe9bW_-ZF8%3DBa2Ud2%3Dw%40mail.gmail.com
--
Matheus Alcantara
Вложения
On 9/16/25 8:18 AM, Matheus Alcantara wrote: > Any opinions on this? > > [1] https://www.postgresql.org/message-id/CAKFQuwbR1Fzr8yRuMW%3DN1UMA1cTpFcqZe9bW_-ZF8%3DBa2Ud2%3Dw%40mail.gmail.com > Just as the discussion here. Adding extension location is a good idea. Suppose there is an amcheck 1.5 located in the $system directory. There is also an amcheck 1.4.5 located in another path. Strange results will then occur: postgres=# SHOW extension_control_path; extension_control_path ------------------------ $system (1 row) postgres=# CREATE EXTENSION amcheck; CREATE EXTENSION postgres=# select * from pg_available_extensions; name | default_version | installed_version | comment | location ------------+-----------------+-------------------+--------------------------------------------+---------- amcheck | 1.5 | 1.5 | functions for verifying relation integrity | $system This seems to be fine. However, if another path is added, strange results will occur. postgres=# SET extension_control_path TO '/Users/quanzl/build/pg-availext:$system'; SET postgres=# select * from pg_available_extensions; name | default_version | installed_version | comment | location ------------+-----------------+-------------------+--------------------------------------------+------------------------------------------- amcheck | 1.4.5 | 1.5 | functions for verifying relation integrity | /Users/quanzl/build/pg-availext/extension The results shown here will cause confusion. It is better to show the path used at creation. So, it would be a better option to add a new column to the pg_extension table. -- Quan Zongliang
> On 9/16/25 8:18 AM, Matheus Alcantara wrote: > >> Any opinions on this? >> [1] https://www.postgresql.org/message-id/CAKFQuwbR1Fzr8yRuMW%3DN1UMA1cTpFcqZe9bW_-ZF8%3DBa2Ud2%3Dw%40mail.gmail.com > Just as the discussion here. Adding extension location is a good idea. +1. I like the ideal. > -- > Matheus Alcantara > <v1-0001-Add-path-of-extension-on-pg_available_extensions.patch> Got a few comments: 1 - extension.c ``` +/* + * A location configured on extension_control_path GUC. + * + * The macro is the macro plaeholder that the extension_control_path support + * and which is replaced by a system value that is stored on loc. For custom + * paths that don't have a macro the macro field is NULL. + */ ``` Some problems in the comment: * typo: plaebholder -> placeholder * "the extension_control_path support” where “support” should be “supports” * “stored on loc” should be “stored in loc” Also, “The macro is the macro placeholder …” sounds redundant, suggested revision: “The macro field stores the name of amacro (for example “$system”) that extension_control_path supports, which is replaced by …" 2 - extension.c ``` + Location *location = palloc0_object(Location); + + location->macro = NULL; + location->loc = system_dir; + paths = lappend(paths, location); ``` As you immediately assign values to all fields, palloc0_object() is not needed, palloc_object() is good enough. 3 - extension.c ``` @@ -366,6 +384,7 @@ get_extension_control_directories(void) int len; char *mangled; char *piece = first_path_var_separator(ecp); + Location *location = palloc0_object(Location); ``` In all execution paths, location will be initiated, thus palloc_object() is good enough. 4 - extension.c ``` + /* location */ + if (location->macro == NULL) + values[3] = CStringGetTextDatum(location->loc); + else + values[3] = CStringGetTextDatum(location->macro); ``` There are multiple places of this “if-else”. So, “macro” basically is for display, and loc is the real location. I am thinking,maybe we can change the definition of Location to: ``` structure Location { Char *display; Char *real; ``` When it is not a macro, just assign real to display, so that we can avoid all these “if-else”. Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/
On Wed, Oct 22, 2025, at 10:28 PM, Chao Li wrote: >> On 9/16/25 8:18 AM, Matheus Alcantara wrote: >> >>> Any opinions on this? >>> [1] https://www.postgresql.org/message-id/CAKFQuwbR1Fzr8yRuMW%3DN1UMA1cTpFcqZe9bW_-ZF8%3DBa2Ud2%3Dw%40mail.gmail.com >> Just as the discussion here. Adding extension location is a good idea. > > > +1. I like the ideal. > Exposing useful information might be a good idea except if it doesn't compromise security. IIRC there is no function or view that exposes absolute path to regular users. The view pg_available_extensions has PUBLIC access. Check similar functions using a query like: SELECT proname, x.unnest AS argname FROM (SELECT proname, unnest(proargnames) FROM pg_proc) AS x WHERE x.unnest ~ 'file' OR x.unnest ~ 'path'; Some of the functions that return absolute path revoked PUBLIC access for security reason. See pg_show_all_file_settings, pg_hba_file_rules, and pg_ident_file_mappings. (All of these functions have a view that returns its content similar to pg_available_extensions.) See system_views.sql. Do we want to use a similar pattern (revoke PUBLIC access from the function)? It breaks the compatibility but perhaps using an existent pre-defined role (pg_read_all_settings?) may be less harmful. There are at least 2 alternatives: * separate function: add a new function that returns the absolute path. Don't grant PUBLIC access. It doesn't break compatibility but you need to modify your query. * insufficient privilege: if the role doesn't have the sufficient privileges, return NULL or '<insufficient privilege>' (similar to pg_stat_activity). I don't have a strong preference but the latter can impose more effort to use if you don't know the role has sufficient privilege. However, it is clear why the absolute path is not returned. -- Euler Taveira EDB https://www.enterprisedb.com/
Thanks for testing this! On Wed Oct 22, 2025 at 9:19 PM -03, Quan Zongliang wrote: > On 9/16/25 8:18 AM, Matheus Alcantara wrote: > >> Any opinions on this? >> >> [1] https://www.postgresql.org/message-id/CAKFQuwbR1Fzr8yRuMW%3DN1UMA1cTpFcqZe9bW_-ZF8%3DBa2Ud2%3Dw%40mail.gmail.com >> > Just as the discussion here. Adding extension location is a good idea. > Suppose there is an amcheck 1.5 located in the $system directory. There > is also an amcheck 1.4.5 located in another path. > > Strange results will then occur: > postgres=# SHOW extension_control_path; > extension_control_path > ------------------------ > $system > (1 row) > > postgres=# CREATE EXTENSION amcheck; > CREATE EXTENSION > postgres=# select * from pg_available_extensions; > > name | default_version | > installed_version | comment | location > ------------+-----------------+-------------------+--------------------------------------------+---------- > amcheck | 1.5 | 1.5 | functions for > verifying relation integrity | $system > > This seems to be fine. > > However, if another path is added, strange results will occur. > > postgres=# SET extension_control_path TO > '/Users/quanzl/build/pg-availext:$system'; > SET > postgres=# select * from pg_available_extensions; > name | default_version | installed_version | > comment | location > ------------+-----------------+-------------------+--------------------------------------------+------------------------------------------- > amcheck | 1.4.5 | 1.5 | functions for > verifying relation integrity | /Users/quanzl/build/pg-availext/extension > > The results shown here will cause confusion. It is better to show the > path used at creation. > I agree that this sounds strange but the documentation [1] mention the following: If extensions with equal names are present in multiple directories in the configured path, only the instance found first in the path will be used. So I think that users should not use different paths to install the same extension with different versions in practice. > So, it would be a better option to add a new column to the pg_extension > table. > You mean add the location column on pg_extension instead of pg_available_extensions? I'm not sure if I get the point here. [1] https://www.postgresql.org/docs/18/runtime-config-client.html#GUC-EXTENSION-CONTROL-PATH -- Matheus Alcantara
Thanks for reviewing this!
On Wed Oct 22, 2025 at 10:28 PM -03, Chao Li wrote:
>> <v1-0001-Add-path-of-extension-on-pg_available_extensions.patch>
>
> Got a few comments:
>
> 1 - extension.c
> ```
> +/*
> + * A location configured on extension_control_path GUC.
> + *
> + * The macro is the macro plaeholder that the extension_control_path support
> + * and which is replaced by a system value that is stored on loc. For custom
> + * paths that don't have a macro the macro field is NULL.
> + */
> ```
>
> Some problems in the comment:
>
> * typo: plaebholder -> placeholder
> * "the extension_control_path support” where “support” should be “supports”
> * “stored on loc” should be “stored in loc”
>
Fixed
> Also, “The macro is the macro placeholder …” sounds redundant, suggested revision: “The macro field stores the name
ofa macro (for example “$system”) that extension_control_path supports, which is replaced by …"
>
> 2 - extension.c
> ```
> + Location *location = palloc0_object(Location);
> +
> + location->macro = NULL;
> + location->loc = system_dir;
> + paths = lappend(paths, location);
> ```
>
Fixed
> As you immediately assign values to all fields, palloc0_object() is not needed, palloc_object() is good enough.
>
> 3 - extension.c
> ```
> @@ -366,6 +384,7 @@ get_extension_control_directories(void)
> int len;
> char *mangled;
> char *piece = first_path_var_separator(ecp);
> + Location *location = palloc0_object(Location);
> ```
>
> In all execution paths, location will be initiated, thus palloc_object() is good enough.
>
Fixed
> 4 - extension.c
> ```
> + /* location */
> + if (location->macro == NULL)
> + values[3] = CStringGetTextDatum(location->loc);
> + else
> + values[3] = CStringGetTextDatum(location->macro);
> ```
>
> There are multiple places of this “if-else”. So, “macro” basically is for display, and loc is the real location. I am
thinking,maybe we can change the definition of Location to:
>
> ```
> structure Location {
> Char *display;
> Char *real;
> ```
>
> When it is not a macro, just assign real to display, so that we can avoid all these “if-else”.
>
These struct fields sounds a bit unclear by just looking it without
reading the usages to me TBH. What do you think by creating a static
function that do the if-else and just use it? Perhaps make into a macro?
Attached v2 with all the fixes.
--
Matheus Alcantara
Вложения
On Thu Oct 23, 2025 at 10:56 AM -03, Euler Taveira wrote: > On Wed, Oct 22, 2025, at 10:28 PM, Chao Li wrote: >>> On 9/16/25 8:18 AM, Matheus Alcantara wrote: >>> >>>> Any opinions on this? >>>> [1] https://www.postgresql.org/message-id/CAKFQuwbR1Fzr8yRuMW%3DN1UMA1cTpFcqZe9bW_-ZF8%3DBa2Ud2%3Dw%40mail.gmail.com >>> Just as the discussion here. Adding extension location is a good idea. >> >> >> +1. I like the ideal. >> > > Exposing useful information might be a good idea except if it doesn't > compromise security. IIRC there is no function or view that exposes absolute > path to regular users. > > The view pg_available_extensions has PUBLIC access. Check similar functions > using a query like: > > SELECT proname, > x.unnest AS argname > FROM > (SELECT proname, > unnest(proargnames) > FROM pg_proc) AS x > WHERE x.unnest ~ 'file' > OR x.unnest ~ 'path'; > > Some of the functions that return absolute path revoked PUBLIC access for > security reason. See pg_show_all_file_settings, pg_hba_file_rules, and > pg_ident_file_mappings. (All of these functions have a view that returns its > content similar to pg_available_extensions.) See system_views.sql. > > Do we want to use a similar pattern (revoke PUBLIC access from the function)? > It breaks the compatibility but perhaps using an existent pre-defined role > (pg_read_all_settings?) may be less harmful. > > There are at least 2 alternatives: > > * separate function: add a new function that returns the absolute path. Don't > grant PUBLIC access. It doesn't break compatibility but you need to modify > your query. > > * insufficient privilege: if the role doesn't have the sufficient privileges, > return NULL or '<insufficient privilege>' (similar to pg_stat_activity). I > don't have a strong preference but the latter can impose more effort to use > if you don't know the role has sufficient privilege. However, it is clear why > the absolute path is not returned. > Yeah, I agree. I've implemented the first version in a way it doesn't show the real value of the $system macro because of security reasons but I agree that we don't want that any user can see the configured path of custom extensions too. I would prefer to go with the '<insufficient privilege>' since it's something that we already have today in other views and users may already know how to deal with it. -- Matheus Alcantara
On 10/23/25 9:56 PM, Euler Taveira wrote: > > * insufficient privilege: if the role doesn't have the sufficient privileges, > return NULL or '<insufficient privilege>' (similar to pg_stat_activity). I > don't have a strong preference but the latter can impose more effort to use > if you don't know the role has sufficient privilege. However, it is clear why > the absolute path is not returned. > +1 I think this way is better.
On Thu Oct 23, 2025 at 10:57 PM -03, Quan Zongliang wrote: > On 10/23/25 9:56 PM, Euler Taveira wrote: > >> >> * insufficient privilege: if the role doesn't have the sufficient privileges, >> return NULL or '<insufficient privilege>' (similar to pg_stat_activity). I >> don't have a strong preference but the latter can impose more effort to use >> if you don't know the role has sufficient privilege. However, it is clear why >> the absolute path is not returned. >> > > +1 > I think this way is better. > So here it is, see attached. I've created a new role pg_read_extension_paths for this, I'm not sure if it's the best way to do this. I'm open for other ideas, perhaps we can reuse some other role? -- Matheus Alcantara
Вложения
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.
! 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.
! 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/
+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)?
+/*
+ * 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?
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.
+{ 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.
@@ -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.
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.
Documentation is missing. These views are documented in system-views.sgml.
--
Euler Taveira
EDB https://www.enterprisedb.com/
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
Вложения
Hi, On Mon, Sep 15, 2025 at 09:18:25PM -0300, Matheus Alcantara wrote: > postgres=# select * from pg_available_extensions; > name | default_version | installed_version | comment | location > ---------+-----------------+-------------------+--------------------------------------------------+--------------------------------------------------- > envvar | 1.0.0 | | Get the value of a server environment variable | /usr/local/my/extensions/share/postgresql/extension > amcheck | 1.5 | | functions for verifying relation integrity | $system > bloom | 1.0 | | bloom access method - signature file based index | $system I am not sure just adding the column at the end is best, I would have put it before comment so that stays last, maybe somebody else has some bikeshedding input here? Michae
Thanks for reviewing this! On Sun Nov 2, 2025 at 12:11 PM -03, Michael Banck wrote: > On Mon, Sep 15, 2025 at 09:18:25PM -0300, Matheus Alcantara wrote: >> postgres=# select * from pg_available_extensions; >> name | default_version | installed_version | comment | location >> ---------+-----------------+-------------------+--------------------------------------------------+--------------------------------------------------- >> envvar | 1.0.0 | | Get the value of a server environment variable | /usr/local/my/extensions/share/postgresql/extension >> amcheck | 1.5 | | functions for verifying relation integrity | $system >> bloom | 1.0 | | bloom access method - signature file based index | $system > > I am not sure just adding the column at the end is best, I would have > put it before comment so that stays last, maybe somebody else has some > bikeshedding input here? > Yeah, I think that it looks better to keep the comment at the end. If no objections I'll swap the order of "comment" and "location" columns on the next version. -- Matheus Alcantara EDB: http://www.enterprisedb.com
On Thu, Nov 6, 2025 at 9:29 AM Matheus Alcantara <matheusssilv97@gmail.com> wrote:
Thanks for reviewing this!
On Sun Nov 2, 2025 at 12:11 PM -03, Michael Banck wrote:
> On Mon, Sep 15, 2025 at 09:18:25PM -0300, Matheus Alcantara wrote:
>> postgres=# select * from pg_available_extensions;
>> name | default_version | installed_version | comment | location
>> ---------+-----------------+-------------------+--------------------------------------------------+---------------------------------------------------
>> envvar | 1.0.0 | | Get the value of a server environment variable | /usr/local/my/extensions/share/postgresql/extension
>> amcheck | 1.5 | | functions for verifying relation integrity | $system
>> bloom | 1.0 | | bloom access method - signature file based index | $system
>
> I am not sure just adding the column at the end is best, I would have
> put it before comment so that stays last, maybe somebody else has some
> bikeshedding input here?
>
Yeah, I think that it looks better to keep the comment at the end. If no
objections I'll swap the order of "comment" and "location" columns on
the next version.
--
Matheus Alcantara
EDB: http://www.enterprisedb.com
Hello!
I have a small bikeshedding comment around making "location" the 4th column returned for "select * from pg_available_extensions", as opposed to leaving "comment" the 4th column returned for "select * from pg_available_extensions".
If a bit of software runs "select * from pg_available_extensions" and fetches the contents of the 4th column, that column will return "comment" for current versions of postgres but "location" for patched versions of postgres.
In many ways, this could be considered a feature and not a bug, because we should be encouraged to write our SQL like so:
select name, default_version, installed_version, comment from pg_available_extensions
and not like so:
select * from pg_available_extensions
I'm curious to know if this is a legitimate consideration or not.
Also, there were no surprises when I compiled and tested this: the location shows correctly for a superuser, and "<insufficient privilege>" shows correctly for a non-superuser.
-- -- Manni Wood EDB: https://www.enterprisedb.com
Thank you for reviewing this! On Mon Nov 10, 2025 at 3:25 PM -03, Manni Wood wrote: > Hello! > > I have a small bikeshedding comment around making "location" the 4th column > returned for "select * from pg_available_extensions", as opposed to leaving > "comment" the 4th column returned for "select * from > pg_available_extensions". > > If a bit of software runs "select * from pg_available_extensions" and > fetches the contents of the 4th column, that column will return "comment" > for current versions of postgres but "location" for patched versions of > postgres. > > In many ways, this could be considered a feature and not a bug, because we > should be encouraged to write our SQL like so: > > select name, default_version, installed_version, comment from > pg_available_extensions > > and not like so: > > select * from pg_available_extensions > > I'm curious to know if this is a legitimate consideration or not. > > Also, there were no surprises when I compiled and tested this: the location > shows correctly for a superuser, and "<insufficient privilege>" shows > correctly for a non-superuser. > Good point, I think that it's a legitimate consideration. That being said I would get back to prefer to keep the location as the latest column to avoid such issues even if SELECT * is not something that users should do in practice, but I think that it's worth to avoid break any application with such change. -- Matheus Alcantara EDB: http://www.enterprisedb.com
Hi, On Mon, Nov 10, 2025 at 07:48:03PM -0300, Matheus Alcantara wrote: > On Mon Nov 10, 2025 at 3:25 PM -03, Manni Wood wrote: > > I have a small bikeshedding comment around making "location" the 4th column > > returned for "select * from pg_available_extensions", as opposed to leaving > > "comment" the 4th column returned for "select * from > > pg_available_extensions". > > > > If a bit of software runs "select * from pg_available_extensions" and > > fetches the contents of the 4th column, that column will return "comment" > > for current versions of postgres but "location" for patched versions of > > postgres. > > > > In many ways, this could be considered a feature and not a bug, because we > > should be encouraged to write our SQL like so: > > > > select name, default_version, installed_version, comment from > > pg_available_extensions > > > > and not like so: > > > > select * from pg_available_extensions > > > > I'm curious to know if this is a legitimate consideration or not. > > > > Also, there were no surprises when I compiled and tested this: the location > > shows correctly for a superuser, and "<insufficient privilege>" shows > > correctly for a non-superuser. > > > Good point, I think that it's a legitimate consideration. That being > said I would get back to prefer to keep the location as the latest > column to avoid such issues even if SELECT * is not something that users > should do in practice, but I think that it's worth to avoid break any > application with such change. When the trusted column got added to the pg_availe_extensions view in 50fc694, it wasn't added to the end, but next to superuser, where it logically makes sense IMO: |@@ -317,7 +317,8 @@ CREATE VIEW pg_available_extensions AS | | CREATE VIEW pg_available_extension_versions AS | SELECT E.name, E.version, (X.extname IS NOT NULL) AS installed, |- E.superuser, E.relocatable, E.schema, E.requires, E.comment |+ E.superuser, E.trusted, E.relocatable, |+ E.schema, E.requires, E.comment | FROM pg_available_extension_versions() AS E | LEFT JOIN pg_extension AS X | ON E.name = X.extname AND E.version = X.extversion; As far as I know, Postgres does not guarantee stable system catalogs between major versions, so I don't think users should or could rely on stable column ordering, really. Michael
Hi Michael,
I am just getting started in the Postgres community (this is my first code review). So please excuse me if I have
missedsomething (in terms of process etc).
I reviewed your proposed code changes in the attached patch file and they look good to me. I have some minor comments:
1. In src/test/modules/test_extensions/t/001_extension_control_path.pl, it would be nice if you could add a test that
validatesthat the correct Extension location is displayed, if for example, the extension is being picked up from a
customizedlocation.
2. Nit-pick: In src/backend/commands/extension.c:get_available_versions_for_extension(), you could probably combine the
following2 lines into one to say "'comment' & 'location' stay the same.
/* comment stays the same */
/* location stays the same */
Hope this is helpful.
Thanks,
-Rohit
On Mon Nov 10, 2025 at 8:10 PM -03, Michael Banck wrote: >> > I have a small bikeshedding comment around making "location" the 4th column >> > returned for "select * from pg_available_extensions", as opposed to leaving >> > "comment" the 4th column returned for "select * from >> > pg_available_extensions". >> > >> > If a bit of software runs "select * from pg_available_extensions" and >> > fetches the contents of the 4th column, that column will return "comment" >> > for current versions of postgres but "location" for patched versions of >> > postgres. >> > >> > In many ways, this could be considered a feature and not a bug, because we >> > should be encouraged to write our SQL like so: >> > >> > select name, default_version, installed_version, comment from >> > pg_available_extensions >> > >> > and not like so: >> > >> > select * from pg_available_extensions >> > >> > I'm curious to know if this is a legitimate consideration or not. >> > >> Good point, I think that it's a legitimate consideration. That being >> said I would get back to prefer to keep the location as the latest >> column to avoid such issues even if SELECT * is not something that users >> should do in practice, but I think that it's worth to avoid break any >> application with such change. > > When the trusted column got added to the pg_availe_extensions view in > 50fc694, it wasn't added to the end, but next to superuser, where it > logically makes sense IMO: > > |@@ -317,7 +317,8 @@ CREATE VIEW pg_available_extensions AS > | > | CREATE VIEW pg_available_extension_versions AS > | SELECT E.name, E.version, (X.extname IS NOT NULL) AS installed, > |- E.superuser, E.relocatable, E.schema, E.requires, E.comment > |+ E.superuser, E.trusted, E.relocatable, > |+ E.schema, E.requires, E.comment > | FROM pg_available_extension_versions() AS E > | LEFT JOIN pg_extension AS X > | ON E.name = X.extname AND E.version = X.extversion; > > As far as I know, Postgres does not guarantee stable system catalogs > between major versions, so I don't think users should or could rely on > stable column ordering, really. > Thanks for pointing this, I didn't know about this detail. I'm not against swapping the orders of "comment" and "location" columns, I also think that it would look better, I'm just afraid of breaking any compatibility with anything, but it seems that it's not the case. -- Matheus Alcantara EDB: http://www.enterprisedb.com
On Mon Nov 10, 2025 at 11:06 PM -03, Rohit Prasad wrote: > Hi Michael, > I think you wanted to say Matheus :) > I am just getting started in the Postgres community (this is my first > code review). So please excuse me if I have missed something (in terms > of process etc). > Thank you for reviewing this! > I reviewed your proposed code changes in the attached patch file and > they look good to me. > Thanks. > I have some minor comments: > 1. In src/test/modules/test_extensions/t/001_extension_control_path.pl, > it would be nice if you could add a test that validates that the > correct Extension location is displayed, if for example, the extension > is being picked up from a customized location. > I don't know if I get your point here. On the v4 patch we have: - "test_custom_ext_paths|1.0|1.0|Test extension_control_path", + "test_custom_ext_paths|1.0|1.0|Test extension_control_path|$ext_dir_canonicalized/extension", The $ext_dir_canonicalized in this case is a custom path configured on extension_control_path GUC that the "test_custom_ext_paths" extension was installed. > 2. Nit-pick: In > src/backend/commands/extension.c:get_available_versions_for_extension(), > you could probably combine the following 2 lines into one to say > "'comment' & 'location' stay the same. > /* comment stays the same */ > /* location stays the same */ > Fixed on attached v5 On this new v5 version I also swap the order of "comment" and "location" columns as it was suggested by Michael. -- Matheus Alcantara EDB: http://www.enterprisedb.com
Вложения
Hi Matheus, Apologies for the mix up on the names :-) I agree with your responses (please ignore my prior comment about test_extension code; I had missed some of your changesthere). I also reviewed v5 of your patch and the changes look good to me. Thanks, -Rohit
On Tue Nov 11, 2025 at 8:17 PM -03, Rohit Prasad wrote: > Hi Matheus, > > Apologies for the mix up on the names :-) > > I agree with your responses (please ignore my prior comment about test_extension code; I had missed some of your changesthere). > > I also reviewed v5 of your patch and the changes look good to me. > No problem and thank you for reviewing! -- Matheus Alcantara EDB: http://www.enterprisedb.com
Hi Matheus, I just reviewed and tested the patch again, got a few more comments: > On Nov 11, 2025, at 20:47, Matheus Alcantara <matheusssilv97@gmail.com> wrote: > > > On this new v5 version I also swap the order of "comment" and "location" > columns as it was suggested by Michael. > > -- > Matheus Alcantara > EDB: http://www.enterprisedb.com > > <v5-0001-Add-path-of-extension-on-pg_available_extensions.patch> 1 - commit comment ``` User-defined locations are 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. ``` First, there is a typo: "for users that has” should be “have”. Then, Is this still true? The code change shows: ``` + /* We only want to show extension paths for superusers. */ + if (superuser_arg(GetUserId())) + { ``` And the code change says: ``` + GUC. Only superusers can see the contents of this column. ``` But the TAP test contains a case for normal user: ``` +# Create an user to test permissions to read extension locations. +my $user = "user01"; +$node->safe_psql('postgres', "CREATE USER $user"); + my $ecp = $node->safe_psql('postgres', 'show extension_control_path;'); is($ecp, "\$system$sep$ext_dir$sep$ext_dir2", @@ -46,28 +54,46 @@ $node->safe_psql('postgres', "CREATE EXTENSION $ext_name2"); my $ret = $node->safe_psql('postgres', "select * from pg_available_extensions where name = '$ext_name'"); is( $ret, - "test_custom_ext_paths|1.0|1.0|Test extension_control_path", + "test_custom_ext_paths|1.0|1.0|$ext_dir_canonicalized/extension|Test extension_control_path", "extension is installed correctly on pg_available_extensions”); ``` And I tried to run the TAP test, it passed. So I’m really confused here. 2 ``` + else + return "<insufficient privilege>"; +} ``` Why do we just show nothing (empty string)? Every row showing a long string of <insufficient privilege> just looks not good,that’s a lot of duplications. ``` evantest=> select * from pg_available_extension_versions; name | version | installed | superuser | trusted | relocatable | schema | requires | location | comment --------------------+---------+-----------+-----------+---------+-------------+------------+---------------------+--------------------------+------------------------------------------------------------------------ refint | 1.0 | f | t | f | t | | | <insufficientprivilege> | functions for implementing referential integrity (obsolete) unaccent | 1.1 | f | t | t | t | | | <insufficientprivilege> | text search dictionary that removes accents ``` Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/
On Thu Nov 13, 2025 at 6:11 AM -03, Chao Li wrote:
>> <v5-0001-Add-path-of-extension-on-pg_available_extensions.patch>
>
> 1 - commit comment
> ```
> User-defined locations are 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.
> ```
>
> First, there is a typo: "for users that has” should be “have”.
>
> Then, Is this still true? The code change shows:
> ```
> + /* We only want to show extension paths for superusers. */
> + if (superuser_arg(GetUserId()))
> + {
> ```
>
> And the code change says:
> ```
> + GUC. Only superusers can see the contents of this column.
> ```
>
> But the TAP test contains a case for normal user:
> ```
> +# Create an user to test permissions to read extension locations.
> +my $user = "user01";
> +$node->safe_psql('postgres', "CREATE USER $user");
> +
> my $ecp = $node->safe_psql('postgres', 'show extension_control_path;');
>
> is($ecp, "\$system$sep$ext_dir$sep$ext_dir2",
> @@ -46,28 +54,46 @@ $node->safe_psql('postgres', "CREATE EXTENSION $ext_name2");
> my $ret = $node->safe_psql('postgres',
> "select * from pg_available_extensions where name = '$ext_name'");
> is( $ret,
> - "test_custom_ext_paths|1.0|1.0|Test extension_control_path",
> + "test_custom_ext_paths|1.0|1.0|$ext_dir_canonicalized/extension|Test extension_control_path",
> "extension is installed correctly on pg_available_extensions”);
> ```
>
> And I tried to run the TAP test, it passed.
>
> So I’m really confused here.
>
Oops, I forgot to update the commit message. The extension location is
only visible for super users. Fixed on v6.
The created user on TAP test is used to check that non superusers can not
see the location column content. The other test case changes are using
the 'postgres' user, so it should see the extension location value.
> 2
> ```
> + else
> + return "<insufficient privilege>";
> +}
> ```
>
> Why do we just show nothing (empty string)? Every row showing a long string of <insufficient privilege> just looks
notgood, that’s a lot of duplications.
>
This is the same behavior of pg_stat_activity. Returning an empty string
could make the user think that something is not right with the
implementation. Returning <insufficient privilege> for every row is a
lot of duplication I agree but I think that it make clear for the user
that it don't have the necessary role to view the column value.
Thanks for reviewing!
--
Matheus Alcantara
EDB: http://www.enterprisedb.com