Thread

Re: Include extension path on pg_available_extensions

Matheus Alcantara <matheusssilv97@gmail.com> — 2025-10-23T18:13:46Z

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 of a 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