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
