Synchronise extensions before reconnect #316
Conversation
Y--
force-pushed
the
yl/reload-extensions
branch
from
077b7ef
to
b61fe8d
Compare
Y--
force-pushed
the
yl/reload-extensions
branch
from
b61fe8d
to
1eebeba
Compare
Y--
force-pushed
the
yl/reload-extensions
branch
from
1eebeba
to
65da870
Compare
| @@ -202,6 +198,8 @@ DuckDBManager::LoadExtensions(duckdb::ClientContext &context) { | |||
| if (extension.name == "httpfs") { | |||
| continue; | |||
| } | |||
|
|
|||
| DuckDBQueryOrThrow(context, "INSTALL " + extension.name); | |||
There was a problem hiding this comment.
Interesting, was this not needed before?
There was a problem hiding this comment.
In the test we insert rows directly in extensions table, so we don't run the install_extension function. I hesitated using the function or making sure that no matter what we install the extension if needed as it makes the UX nicer, though I don't have strong opinion on it.
There was a problem hiding this comment.
Is INSTALL an actual no-op if the extension is already installed? Or does it do something semi-expensive like network requests, which would be good to avoid on every DB initialization.
There was a problem hiding this comment.
Yep it is a no-op, cf. here
| auto drop_secret_cmd = duckdb::StringUtil::Format("DROP SECRET pgduckb_secret_%d;", secret_id); | ||
| pgduckdb::DuckDBQueryOrThrow(drop_secret_cmd); | ||
| pgduckdb::DuckDBQueryOrThrow(context, drop_secret_cmd); |
There was a problem hiding this comment.
This change make sure that we don't have a re-entrant call to DuckDBManager::Get
Fixes #217 This changes the defaults for `duckdb.max_memory` and `duckdb.disabled_filesystems` to the agreed upon settings. While doing that I found two issues that this also fixes: 1. `duckdb.disabled_filesystems = 'LocalFilesystem'` was basically a way to make DuckDB execution unusable, due to us doing file system access when initializing the database/connection. So now we enable the setting later. Also we now don't call `INSTALL {extension}` during connection initialization anymore. 2. #316 didn't fix the loading of extensions when `duckdb.install_extension()` was called. Only for direct inserts to the `duckdb.extensions` table. That's fixed by not using the low-level `CatalogTupleInsert`, and instead switching to SPI. 3. Postgres superusers should not be restricted by `duckdb.disabled_filesystems`, they should be able to do whatever they want. So now this setting is not actually configured for superusers anymore.
If duckdb.extensions table is inserted, updated or deleted in same connection we would not sync changes for next query. Introduce sequence that keeps track of each change so we can compare with and sync if required.
Inspired from #253
Fixes #278