| From: | "Matheus Alcantara" <matheusssilv97(at)gmail(dot)com> |
|---|---|
| To: | "Chao Li" <li(dot)evan(dot)chao(at)gmail(dot)com>, "Matheus Alcantara" <matheusssilv97(at)gmail(dot)com> |
| Cc: | "Rohit Prasad" <rohit(dot)prasad(at)arm(dot)com>, <pgsql-hackers(at)lists(dot)postgresql(dot)org>, <mbanck(at)gmx(dot)net> |
| Subject: | Re: Include extension path on pg_available_extensions |
| Date: | 2025-11-13 14:36:09 |
| Message-ID: | DE7N521UVATK.OOQ2SLKA0Z9R@gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
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 not good, 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
| Attachment | Content-Type | Size |
|---|---|---|
| v6-0001-Add-path-of-extension-on-pg_available_extensions.patch | text/plain | 18.6 KB |
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Marcos Pegoraro | 2025-11-13 14:39:42 | Re: Document NULL |
| Previous Message | Bertrand Drouvot | 2025-11-13 14:11:08 | Re: Consistently use the XLogRecPtrIsInvalid() macro |