Re: extension_control_path and "directory"

On Thu, Apr 24, 2025 at 7:21 AM Christoph Berg <myon(at)debian(dot)org> 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