custom_extension_repository over https:// #9818
Conversation
…ing.org/' To be allowed to set CustomExtensionRepository to a remote extension repository requiring https or s3 protocol httpfs to be available, so for this to succeed httpfs needs to be: 1. already loaded (potentially built-in) 2. available to be installed at the local_extension_path The problem is that allowing to change the custom_extension_repository to something needing httpfs allows users to corner themselves out. Potentially this can be relaxed, but given until serving extensions over https:// was not really working, we can as well be strict and relax this later.
There are, as of now, no tests for this, but for the fact that the rest of the system keeps working
|
I think this can be merged, can be polished a bit by allowing autoloading of httpfs, but that's also OK in a separate step since in any case this expand what was previously possible, fixing some corner cases in the process. While writing the docs I also realised that we are not erroring out nicely on: |
Another round of extension docs, depends on duckdb/duckdb#9818
|
Autoloading of httpfs (or any other extension required by file system), has been handled, even though the used API for doing this might need to be reviewed. |
|
There is a funny corner case to be handled, fixing reentrancy. |
Idea is that it should not be possible to ever go back again in the InstallExtension codepath.
5a44bce to
709d238
Compare
| void ExtensionHelper::InstallExtension(DBConfig &config, FileSystem &fs, const string &extension, bool force_install, | ||
| const string &repository) { | ||
| ReentrancyLock reentrant_lock; | ||
| if (!reentrant_lock.LockIsValid()) { |
There was a problem hiding this comment.
Perhaps we can disable auto-loading here entirely (like it is currently) so that we don't have to deal with re-entrancy either?
| // Either extension OR repository might require some extensions to be loaded | ||
| if (!StringUtil::StartsWith(extension, "http://")) { | ||
| // Passing false as last parameter to avoid reentrancy issues | ||
| TryAutoLoadExtension(context, FileSystem::LookupExtensionForPattern(extension), false); |
There was a problem hiding this comment.
Let's remove the auto-loading here as this is introducing a lot of complexity - let's instead fail with a nicer error message if httpfs is not loaded
| } | ||
| } | ||
| auto read_handle = fs.OpenFile(file, FileFlags::FILE_FLAGS_READ); | ||
| auto test_data = std::unique_ptr<unsigned char[]> {new unsigned char[read_handle->GetFileSize()]}; | ||
| read_handle->Read(test_data.get(), read_handle->GetFileSize()); | ||
| WriteExtensionFileToDisk(fs, temp_path, (void *)test_data.get(), read_handle->GetFileSize()); | ||
|
|
||
| if (StringUtil::EndsWith(file, ".gz")) { |
There was a problem hiding this comment.
Can this be unified with the code that does this below?
| // Potentially this can be relaxed, but given until serving extensions over https:// was not really working, we can | ||
| // as | ||
| // well be strict and relax this later. | ||
| if (!context.db->ExtensionIsLoaded("httpfs")) { |
There was a problem hiding this comment.
I think it's better for this to fail during the installation, and not when setting the option
|
This pull request is stale because it has been open 90 days with no activity. Remove stale label or comment or this will be closed in 30 days. |
|
Solved/superseeded by #11677 |
This PR allows to make extension over https protocol work.
Fixes #9818
Demo is like:
git checkout 0fd6fb9198 && GEN=ninja make && wget https://carlopi.github.io/extension-repository/0fd6fb9198/osx_arm64/httpfs.duckdb_extensionTo build the duckdb binary + download httpfs extension locally (otherwise also building it it's fine).
Then:
All but the
httpfsextension (that is loaded from the locally build folder) are loaded over https from the GitHub Page hosted at https://carlopi.github.io/extension-repository, that can only accessed over https, backed by the repository with this content: https://github.com/carlopi/extension-repository/tree/main/0fd6fb9198/osx_arm64.Extension are first looked up in the gzip-compressed version, and if not present in the non-compressed one.
This holds both for when custom_extension_repository is a remote OR local directory.
See for example:
Where extensions works if at least one of the files
name.duckdb_extension.gzorname.duckdb_extensionare present at the expected path.To be done, but not part of this PR:
context, might need to rework interface a bit, to be reviewed)This is still allowing to host repositories on https://, with the only caveat that
httpfsextension has to be either already present on the system (being installed before hand from the official extension repository).This also allows to simplify building and using of local extensions, given that once passed
SET custom_extension_repository='build/release', (and with the infrastructure discussed in #9800), it would be possible to make autoloading and autoinstall work in a similar way to what users will experience. This allows simplifications of infrastructure.