| From: | Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> |
|---|---|
| To: | Matheus Alcantara <matheusssilv97(at)gmail(dot)com> |
| Cc: | Pg Hackers <pgsql-hackers(at)postgresql(dot)org> |
| Subject: | Re: Include extension path on pg_available_extensions |
| Date: | 2025-10-23 01:28:13 |
| Message-ID: | 0DDF33A5-4F8C-4725-A124-B301147FA1FA@gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
> 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 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);
```
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/
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Tom Lane | 2025-10-23 01:49:19 | Memory leak due to thinko in PL/Python error handling |
| Previous Message | Michael Paquier | 2025-10-23 00:35:04 | Re: Extended Statistics set/restore/clear functions. |