Обсуждение: Correct comment wording in extension.c

On Wed, Jan 7, 2026 at 1:13 PM albert tan <alterttan1223@gmail.com> wrote:
> I believe:
>   'will be searched' is better than 'will be search'.
>   'it assumes' is better than 'it assume'.

Those are indeed grammatical errors. From a quick eyeball scan, this
file has other wording issues:

/*
 * Ignore already-found names.  They are not reachable by the
 * path search, so don't shown them.
 */

"show"

/*
 * The directory parameter can be omitted, absolute, or relative to the
 * installation's base directory, which can be the sharedir or a custom
 * path that it was set extension_control_path. It depends where the
 * .control file was found.
 */

I imagine this means "or a custom path that was set via extension_control_path".

/*
 * Return a list of directories declared on extension_control_path GUC.
 */

I would guess this normally phrased something like "Return the list of
directories in extension_control_path".

--
John Naylor
Amazon Web Services

On Wed, Jan 7, 2026 at 3:33 PM albert tan <alterttan1223@gmail.com> wrote:
> Hi Join,
> Thanks for pointing out extra issues. I have addressed them in v2.

Thanks, but I can't use this -- it looks like you just picked the
first difference you saw in my proposed edits and ignored the rest,
leaving the result just as awkward or ungrammatical as before (see
below). That doesn't save me any time.

> >/*
> > * The directory parameter can be omitted, absolute, or relative to the
> > * installation's base directory, which can be the sharedir or a custom
> > * path that it was set extension_control_path. It depends where the
> > * .control file was found.
> > */
> >
> >I imagine this means "or a custom path that was set via extension_control_path".

- * path that it was set extension_control_path. It depends where the
+ * path that was set extension_control_path. It depends where the

And, while looking again, there's another error here: "depends on"

> >/*
> > * Return a list of directories declared on extension_control_path GUC.
> > */
>
> >I would guess this normally phrased something like "Return the list of
> >directories in extension_control_path".

- * Return a list of directories declared on extension_control_path GUC.
+ * Return the list of directories declared on extension_control_path GUC.

My main gripe here was "declared on <foo> GUC", not the article, which
on second thought may as well remain "a list" since we do use that
elsewhere to mean "an instance of the List type". I went with "Return
a list of directories declared in the extension_control_path GUC.".

Also, I decided that the paragraph that started this report needs to
be rewritten:

> ' The control file will be search on Extension_control_path paths if
>   control->control_dir is NULL, otherwise it will use the value of control_dir
>   to read and parse the .control file, so it assume that the control_dir is a
>   valid path for the control file being parsed.'

- I'm not sure what the "assume" phrase is explaining, since we always
check for errors when opening a file. The contract elsewhere for
filling in control_dir doesn't seem as relevant as the NULL behavior.
- Passive voice is often used to maintain discourse cohesion, but the
following "it" doesn't refer to anything in particular.

On Fri, Jan 9, 2026 at 1:17 PM li carol <carol.li2025@outlook.com> wrote:
> This should be corrected to:
>
> "Works in a very similar way to find_in_path, but it receives an already parsed List of paths to search the basename,
andit does not support macro replacement or custom error messages (for simplicity). By 'already parsed List of paths'
thisfunction expects that the paths already have all macros replaced." 

Thanks, but I see more that should be done here (also, for something
this long it's better to just mention what the corrections are, or
write a patch, so I don't have to figure it out). The original comment
is awkwardly written in that it has a separate paragraph to explain
what a term in the first paragraph means, so I simplified it to say
what it means.

This has now gone beyond a trivial fix, so I've attached what I
propose to commit barring other opinions.

--
John Naylor
Amazon Web Services

On Sun, Jan 11, 2026 at 11:58 AM Chao Li <li.evan.chao@gmail.com> wrote:
> v4-0001 looks good to me.

When I say I'm going to commit "unless/barring", no response is
necessary unless there is a problem.

> I just went through extension.c and got a few more comment improvements. I put them in v5-0002, please take a look.

- * for them therefore uses about O(N^2) time when there are N versions of
+ * for them therefore uses O(N^2) time when there are N versions of

- /* store the OID of the namespace to-be-changed */
+ /* store the OID of the namespace to be changed */

Please read these responses to you from last week:

https://www.postgresql.org/message-id/1909358.1767854695%40sss.pgh.pa.us
https://www.postgresql.org/message-id/7k27ut6kugt2mizo5cy5wplbbrz4ccbkxvsc2iqu5yde344iya%40dxktdjw6uvi2

I've pushed v4.

--
John Naylor
Amazon Web Services

> On Jan 13, 2026, at 13:40, John Naylor <johncnaylorls@gmail.com> wrote:
>
> On Sun, Jan 11, 2026 at 11:58 AM Chao Li <li.evan.chao@gmail.com> wrote:
>> v4-0001 looks good to me.
>
> When I say I'm going to commit "unless/barring", no response is
> necessary unless there is a problem.
>
>> I just went through extension.c and got a few more comment improvements. I put them in v5-0002, please take a look.
>
> - * for them therefore uses about O(N^2) time when there are N versions of
> + * for them therefore uses O(N^2) time when there are N versions of
>
> - /* store the OID of the namespace to-be-changed */
> + /* store the OID of the namespace to be changed */
>
> Please read these responses to you from last week:
>
> https://www.postgresql.org/message-id/1909358.1767854695%40sss.pgh.pa.us
> https://www.postgresql.org/message-id/7k27ut6kugt2mizo5cy5wplbbrz4ccbkxvsc2iqu5yde344iya%40dxktdjw6uvi2
>
> I've pushed v4.
>

Sure, I have read them carefully.

But you really missed:
```
diff --git a/src/backend/commands/extension.c b/src/backend/commands/extension.c
index 596105ee078..a9529692ae3 100644
--- a/src/backend/commands/extension.c
+++ b/src/backend/commands/extension.c
@@ -2398,7 +2398,7 @@ pg_available_extension_versions(PG_FUNCTION_ARGS)

                                /*
                                 * Ignore already-found names.  They are not reachable by the
-                                * path search, so don't shown them.
+                                * path search, so don't show them.
                                 */
                                extname_str = makeString(extname);
                                if (list_member(found_ext, extname_str))
```

This is not the one in v4.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/