Skip to content

Fix extension load#19079

Merged
Mytherin merged 5 commits into
duckdb:mainfrom
dentiny:hjiang/fix-extension-load
Sep 22, 2025
Merged

Fix extension load#19079
Mytherin merged 5 commits into
duckdb:mainfrom
dentiny:hjiang/fix-extension-load

Conversation

@dentiny
Copy link
Copy Markdown
Member

@dentiny dentiny commented Sep 22, 2025 •
edited
Loading

When upgrading extensions to duckdb v1.4, I found extension cannot set description any more, the reason is
ExtensionLoader initialized for statically-linked community extension, ExtensionActiveLoad is not passed, thus no extension info.

Reading through the code, ExtensionActiveLoad already contains everything we need, including db instance and extension name:

duckdb/src/include/duckdb/main/extension_manager.hpp

Lines 27 to 39 in 46297ae

class ExtensionActiveLoad {
public:
ExtensionActiveLoad(DatabaseInstance &db, ExtensionInfo &info, string extension_name);
DatabaseInstance &db;
unique_lock<mutex> load_lock;
ExtensionInfo &info;
string extension_name;
public:
void FinishLoad(ExtensionInstallInfo &install_info);
void LoadFail(const ErrorData &error);
};

So in this PR, I removed the unnecessary ctor and fix its usage.

Before the fix, there's no way to set description, which is a regression from v1.3.
See #19073

After the fix

D SELECT * FROM duckdb_extensions() WHERE extension_name = 'observefs';
┌────────────────┬─────────┬───────────┬──────────────┬─────────────┬───────────┬───────────────────┬───────────────────┬────────────────┐
│ extension_name │ loaded  │ installed │ install_path │ description │  aliases  │ extension_version │   install_mode    │ installed_from │
│    varchar     │ boolean │  boolean  │   varchar    │   varchar   │ varchar[] │      varchar      │      varchar      │    varchar     │
├────────────────┼─────────┼───────────┼──────────────┼─────────────┼───────────┼───────────────────┼───────────────────┼────────────────┤
│ observefs      │ true    │ true      │ (BUILT-IN)   │ helloworld  │ []        │                   │ STATICALLY_LINKED │                │
└────────────────┴─────────┴───────────┴──────────────┴─────────────┴───────────┴───────────────────┴───────────────────┴────────────────┘

I use "helloworld" as my testing description, which correct displays.

dentiny
dentiny commented Sep 22, 2025
View reviewed changes
Comment thread src/include/duckdb/main/database.hpp
auto &manager = ExtensionManager::Get(*instance);
auto info = manager.BeginLoad(extension.Name());
if (!info) {
auto load_info = manager.BeginLoad(extension.Name());
Copy link
Copy Markdown
Member Author

@dentiny dentiny Sep 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we already have install_info here, rename for differentiation.

dentiny
dentiny commented Sep 22, 2025
View reviewed changes
Comment thread src/main/extension/extension_loader.cpp
: db(load_info.db), extension_name(load_info.extension_name), extension_info(load_info.info) {
}

ExtensionLoader::ExtensionLoader(DatabaseInstance &db, const string &name) : db(db), extension_name(name) {
Copy link
Copy Markdown
Member Author

@dentiny dentiny Sep 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

load_info already contains db instance and extension name inside, so it's a redundant ctor

@Mytherin Mytherin requested a review from Maxxen September 22, 2025 07:52
@duckdb-draftbot duckdb-draftbot marked this pull request as draft September 22, 2025 08:04
@dentiny dentiny marked this pull request as ready for review September 22, 2025 08:11
@duckdb-draftbot duckdb-draftbot marked this pull request as draft September 22, 2025 08:21
@dentiny dentiny marked this pull request as ready for review September 22, 2025 08:21
@duckdb-draftbot duckdb-draftbot marked this pull request as draft September 22, 2025 08:33
@dentiny dentiny marked this pull request as ready for review September 22, 2025 08:33
@Maxxen
Copy link
Copy Markdown
Member

Maxxen commented Sep 22, 2025

LGTM, thanks!

@dentiny
Copy link
Copy Markdown
Member Author

dentiny commented Sep 22, 2025

/duckdb_build_dir/build/release/_deps/ducklake_extension_fc-src/src/storage/ducklake_delete_filter.cpp:53:52: error: no matching function for call to ‘duckdb::ExtensionLoader::ExtensionLoader(duckdb::DatabaseInstance&, const char [9])’
53 | ExtensionLoader loader(instance, "ducklake");
| ^

It seems duckdb official extensions also rely on the constructor interface, so I would like to

  • Keep the ctor here for compatibility
  • Try to remove it from all official extensions, and see if we could completely remove the ctor

@duckdb-draftbot duckdb-draftbot marked this pull request as draft September 22, 2025 09:27
@dentiny dentiny marked this pull request as ready for review September 22, 2025 09:28
@Mytherin Mytherin merged commit 1c024c6 into duckdb:main Sep 22, 2025
68 of 69 checks passed
@Mytherin
Copy link
Copy Markdown
Collaborator

Mytherin commented Sep 22, 2025

Thanks!

krlmlr added a commit to krlmlr/duckdb-r that referenced this pull request Oct 21, 2025
@krlmlr
vendor: Update vendored sources to duckdb/duckdb@d26fef1
feaec58 
Merge v1.4 into main (duckdb/duckdb#19098)
Change core extensions bucket name (duckdb/duckdb#19082)
Fix extension load (duckdb/duckdb#19079)
Revert "set default value of MAIN_BRANCH_VERSIONING to false" (duckdb/duckdb#19057)
CI: Increase stale bot timeout to 1 year (duckdb/duckdb#19000)
Switching core extension upload to dedicated credentials (duckdb/duckdb#19060)
krlmlr added a commit to krlmlr/duckdb-r that referenced this pull request Nov 1, 2025
@krlmlr
vendor: Update vendored sources to duckdb/duckdb@d26fef1
f8ed27d 
Merge v1.4 into main (duckdb/duckdb#19098)
Change core extensions bucket name (duckdb/duckdb#19082)
Fix extension load (duckdb/duckdb#19079)
Revert "set default value of MAIN_BRANCH_VERSIONING to false" (duckdb/duckdb#19057)
CI: Increase stale bot timeout to 1 year (duckdb/duckdb#19000)
Switching core extension upload to dedicated credentials (duckdb/duckdb#19060)
krlmlr added a commit to krlmlr/duckdb-r that referenced this pull request Nov 2, 2025
@krlmlr
vendor: Update vendored sources to duckdb/duckdb@d26fef1
be75f23 
Merge v1.4 into main (duckdb/duckdb#19098)
Change core extensions bucket name (duckdb/duckdb#19082)
Fix extension load (duckdb/duckdb#19079)
Revert "set default value of MAIN_BRANCH_VERSIONING to false" (duckdb/duckdb#19057)
CI: Increase stale bot timeout to 1 year (duckdb/duckdb#19000)
Switching core extension upload to dedicated credentials (duckdb/duckdb#19060)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@Maxxen Maxxen Maxxen approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@dentiny @Maxxen @Mytherin