Subject: Re: extension_control_path and "directory"

From: Matheus Alcantara <[email-masked]>
Date: Thu, 24 Apr 2025 12:18:28 -0300

On Thu, Apr 24, 2025 at 7:21 AM Christoph Berg <[email-masked]> wrote:
>
> Re: Matheus Alcantara
> > I've tested with the semver extension and it seems to work fine with
> > this patch. Can you please check on your side to see if it's also
> > working?
>
> Hi Matheus,
>
> thanks for the patch, it does indeed work.
>
Thanks for testing and for reviewing.

> diff --git a/src/backend/commands/extension.c b/src/backend/commands/extension.c
> index 180f4af9be3..d68efd59118 100644
> --- a/src/backend/commands/extension.c
> +++ b/src/backend/commands/extension.c
> @@ -376,6 +376,14 @@ get_extension_control_directories(void)
>
>                         /* Substitute the path macro if needed */
>                         mangled = substitute_path_macro(piece, "$system", system_dir);
> +
> +                       /*
> +                        * Append "extension" suffix in case is a custom extension control
> +                        * path.
> +                        */
> +                       if (strcmp(piece, "$system") != 0)
> +                               mangled = psprintf("%s/extension", mangled);
>
> This would look prettier if it was something like
>
>                         mangled = substitute_path_macro(piece, "$system", system_dir "/extension");
>
> ... but I'm wondering if it wouldn't be saner if the control path
> should be stored without "extension" in that struct. Then opening the
> control file would be path + "extension/" + filename and the extra
> directory would be path + directory, without any on-the-fly stripping
> of trailing components.
>
> The extension_control_path GUC could also be adjusted to refer to the
> directory one level above the extension/foo.control location.
>
Storing the control path directly without any code to remove the
/extension at the end would be more trick I think, because we would need
to change the find_in_path() function to return the path without the
suffix.

In this new version I've introduced a new "basedir" field on
ExtensionControlFile so that we can save the base directory to search
for .control files and scripts. With this new field, on
get_extension_script_directory() we just need to join control->basedir
with control->directory. Note that we still need to handle the removal
of the /extension at the end of control path but I think that on this
new version the code looks a bit better (IMHO) since we just need to
handle on find_extension_control_filename(). WYT?

>
> +       /*
> +        * Assert that the control->control_dir end with /extension suffix so that
> +        * we can replace with the value from control->directory.
> +        */
> +       Assert(ctrldir_len >= suffix_len &&
> +                  strcmp(control->control_dir + ctrldir_len - suffix_len, "extension") == 0);
>
> If control_dir is coming from extension_control_path, it might have a
> different suffix. Replace the Assert by elog(ERROR). (Or see above.)
>
In v2 I've moved the logic to remove the /extension to
parse_extension_control_file(), do you think that this Assert on this
function would still be wrong? IIUC we should always have /extension at
the end of "control_dir" at this place, because the
extension_control_path GUC will omit the /extension at the end and we
will force it to have the suffix on the path at
find_extension_control_filename() and
get_extension_control_directories() functions. I'm missing something
here?

I've also included some more TAP tests on this new version.

-- 
Matheus Alcantara

---

From: "David E. Wheeler" <[email-masked]>
Date: Thu, 24 Apr 2025 18:27:37 -0400

> get_extension_control_directories() functions. I'm missing something

I took this patch for a spin and managed to make it core dump. How? Well I installed semver with this command:

```sh
make prefix=/Users/david/Downloads install
```

Then set the search paths and restarted:

```ini
extension_control_path = '/Users/david/Downloads/share/extension:$system'
dynamic_library_path = '/Users/david/Downloads/lib:$libdir'
```

Then I connected and ran `CREATE EXTENSION semver` and it segfaulted. I poked around for a few minutes and realized that my prefix is not what I expected. Because it doesn’t contain the string “postgres”, PGXS helpfully adds it. The actual paths are:

```ini
extension_control_path = '/Users/david/Downloads/share/postgresql/extension:$system'
dynamic_library_path = '/Users/david/Downloads/lib/postgresql:$libdir'
```

With that fix it no longer segafulted.

So I presume something crashes when a directory or file doesn’t exist.

But I am not at all sure we want this prefix behavior for installing extensions. I get that has been the behavior for setting the main sharedir and libdir for Postgres, but I don’t know that it makes sense for extension prefixes.

Best,

David

---

From: Matheus Alcantara <[email-masked]>
Date: Fri, 25 Apr 2025 10:25:14 -0300

> So I presume something crashes when a directory or file doesn’t exist.

Yes, you are right. The problem was where I was asserting
control->control_dir != NULL. I've moved the assert after the "if
(!filename)" check that returns an error if the extension was not found.

Attached v3 with this fix and also a TAP test for this scenario.

I'm just a bit confused how you get it working using /extension at the
end of extension_control_path since with this patch this suffix is not
necessary and since we hard coded append this it should return an error
when trying to search on something like
/Users/david/Downloads/share/postgresql/extension/extension

-- 
Matheus Alcantara

---

From: "David E. Wheeler" <[email-masked]>
Date: Fri, 25 Apr 2025 15:13:02 -0400

> Attached v3 with this fix and also a TAP test for this scenario.

That fixes the segfault, thank you.

> when trying to search on something like

It worked with

extension_control_path = '/Users/david/Downloads/share/postgresql/extension:$system’

But not with

extension_control_path = '/Users/david/Downloads/share/postgresql:$system’

And here is where the control file actually is:

❯ ll ~/Downloads/share/postgresql/extension  total 8
-rw-r--r--  1 david  staff   161B Apr 24 18:07 semver.control

So I don’t know the answer to your question, but it’d be handy to have functions that return a list of resolved paths from extension_control_path and dynamic_library_path, since they get mangled.

Best,

David

---

From: Matheus Alcantara <[email-masked]>
Date: Fri, 25 Apr 2025 18:18:58 -0300

> That fixes the segfault, thank you.
Great, thanks for testing!

> So I don’t know the answer to your question, but it’d be handy to have functions that return a list of resolved paths from extension_control_path and dynamic_library_path, since they get mangled.
Ok, I was testing using extension_control_path = '$system:/my/custom/path'
(starting with the macro) and it was working as expected, testing with
the macro at the end does not work.

The problem was on find_extension_control_filename() that was appending
the /extension at the end of the entire extension_control_path GUC value
instead of just the custom paths.

To append the /extension at each path on extension_control_path would
require some changes on find_in_path() that
find_extension_control_filename() calls, which I think that it would
make the function more complicated. I've them created a similar
find_in_paths() function that works in the same way but it receives a
List of paths instead of the string of paths separated by ":". We can
get this List of paths using get_extension_control_directories() that
also handle the macro substitution like find_in_path().

Attached v4 with these fixes. I hope that now you should be able to omit
the /extension from the GUC value.

-- 
Matheus Alcantara

---

From: "David E. Wheeler" <[email-masked]>
Date: Mon, 28 Apr 2025 16:49:23 -0400

> the macro at the end does not work.

Great example of why it’s useful to do as much testing as possible! That’s an entirely reasonable place to start testing :-)

> instead of just the custom paths.

Oh yeah, lol, that wouldn’t work.

> the /extension from the GUC value.

Yes! It now works with this configuration:

```ini
extension_control_path = '/Users/david/Downloads/share/postgresql:$system'
dynamic_library_path = '/Users/david/Downloads/lib/postgresql:$libdir’
```

Which is nicely more consistent. Kind of want that first one to be called “share_path” now, though, since it’s not just extensions. Although I guess it’s only extension control file searching that uses it (for now).

If I understand this bit correctly:

```c

/*
* Append "extension" suffix in case is a custom extension control
* path.
*/
if (strcmp(piece, "$system") != 0)
mangled = psprintf("%s/extension", mangled);
```

The value of `piece` is a single path from the search path, right? If so, I think it’s either `$system` or something else; I don’t it would ever be that `$system` is a substring of a single path. Is that right?

Other than that, I think this patch is good to go. Thanks!

Best,

David

`

---

From: Matheus Alcantara <[email-masked]>
Date: Tue, 29 Apr 2025 10:49:09 -0300

> Which is nicely more consistent. Kind of want that first one to be called “share_path” now, though, since it’s not just extensions. Although I guess it’s only extension control file searching that…
Thanks for testing!

> The value of `piece` is a single path from the search path, right? If so, I think it’s either `$system` or something else; I don’t it would ever be that `$system` is a substring of a single path. I…
Yes, it is a single path from the search path, in your case it will be
"/Users/david/Downloads/share/postgresql" and "$system". We split these
paths based on the system path separator and get the next "piece" here:

            char       *piece = first_path_var_separator(ecp);

The first_path_var_separator() changes the "ecp" parameter on every call,
it returns the next path on "ecp" and changes it to have the remaining
paths to iterate over it.

> Other than that, I think this patch is good to go. Thanks!
Thanks for reviewing!

-- 
Matheus Alcantara

---

From: "David E. Wheeler" <[email-masked]>
Date: Tue, 29 Apr 2025 10:08:26 -0400

> paths to iterate over it.

Right. My point is a minor one, but I thin you can use an if/ else there:

```c
if (strcmp(piece, "$system") == 0) {
	/* Substitute the path macro if needed */
	mangled = substitute_path_macro(piece, "$system", system_dir);
} else {
	/*
	* Append "extension" suffix in case is a custom extension
	* control path.
	*/
	mangled = psprintf("%s/extension", mangled);
}
```

Best,

David

---

From: Matheus Alcantara <[email-masked]>
Date: Tue, 29 Apr 2025 12:06:51 -0300

>         mangled = psprintf("%s/extension", mangled);

The substitute_path_macro() already handles the if/else on "piece" but I
think that this if/else version looks nicer. Fixed.

I've also included some documentation changes for this v5 version to
remove the "extension" from the examples and also mention the scenario
when using the "directory" on the .control file.

-- 
Matheus Alcantara

---

From: "David E. Wheeler" <[email-masked]>
Date: Tue, 29 Apr 2025 11:12:02 -0400

> when using the "directory" on the .control file.

Nice, thanks. I’ve made a PR in my GitHub clone for anyone who likes to look it over that way.

https://github.com/theory/postgres/pull/11/files

Best,

David

---

From: Peter Eisentraut <[email-masked]>
Date: Fri, 2 May 2025 16:51:55 +0200

> when using the "directory" on the .control file.

Thanks, I have committed this.  I did a bit of code reformatting and 
adjusted the documentation a bit.  It's good to get this in before beta1 
so that we don't have to change the valid values of 
extension_control_path past beta1.

---

From: Matheus Alcantara <[email-masked]>
Date: Fri, 2 May 2025 12:04:06 -0300

> extension_control_path past beta1.

Thanks Peter!

-- 
Matheus Alcantara

---

From: Christoph Berg <[email-masked]>
Date: Fri, 2 May 2025 17:53:04 +0200

Re: Matheus Alcantara
> > extension_control_path past beta1.

And thanks everyone!

Christoph