I think proposed behaviour does make sense, but requires changing also the loop here:

                for (auto &option : unrecognized_options) {
                        auto &name = option.first;
                        auto &value = option.second;
        
                        auto extension_name = ExtensionHelper::FindExtensionInEntries(name, EXTENSION_SETTINGS);
                        if (extension_name.empty()) {
                                continue;
                        }
                        if (!ExtensionHelper::TryAutoLoadExtension(*this, extension_name)) {
                                throw InvalidInputException(
                                    "To set the %s setting, the %s extension needs to be loaded. But it could not be autoloaded.", name,
                                    extension_name);
                        }
                        //...
                }

Since TryAutoLoadExtension will (on load) actually remove from the unrecognized_options container the current element, and I am not sure what follows.

I think to bring this home either:

• making a copy of unrecognized_options at the top of LoadExtensionSettings function, iterating on the copy and refactoring it a bit to reconcile with changes made by AddExtensionOption
• have AddExtensionOptions register the added extension + default value in a different map, and then reconcile them in LoadExtensionSettings (iterating on this secondary map)

The first seems possibly to be simpler, but I might get messy / eventually trip someone that the iteration is not safe.

Also currently IF a core extension (that has an autoloadable setting as registered) happened to ADD a different setting (and publish for a past DuckDB version that can't known of the setting existing), even though autoloading kicks in the config check will still fail.

To properly test this I think the way would be adding a test storage extension, do not register it for autoloading, and check that a C++ based test happen to pass (where config options are set and attached database is test_storage_extension:file.db

Thanks Carlo. I think this still messy, but probably it's better to copy the list being iterated on in LoadExtensionSettings.

Regarding testing - remember that the extension need to be registered to the file db type to autoload before all of this kicks in. Any existing examples?

I would go for some indirect testing on autoloading (say checking sqlite_scanner and postgres_scanner extension options values after autoloading for the various cases of default provided or not times initial value overridden or not), given some code is common.

Testing the options for storage extensions is doable, but brittle, maybe it's fine as is.

@carlopi I pushed the changes to the unrecognized options logic we discuss.

I would go for some indirect testing on autoloading (say checking sqlite_scanner and postgres_scanner extension options values after autoloading for the various cases of default provided or not times initial value overridden or not), given some code is common.

I'm probably still missing something stupid - are you saying that the CI for those extensions will check this and that's good enough?

Note that you need something like python to even use unrecognized extensions and/or explicit CPP code that uses a map and the dedicated constructor for it rather than just construct a config and set what it wants on it)

I was thinking of a c++ test that has the pg_use_binary_copy provided by postgres_scanner, and checking that once posgres_scanner is autoloaded via LoadExtensionSettings the right value is stored (that should be the one provided by the user, if any, or the default one if none).

See for example the line here: https://github.com/duckdb/duckdb-postgres/blob/dc0957a3755ea058b047bf6381f5b2894b848e1f/src/postgres_extension.cpp#L162 where postgres_scanner option comes with a default, or the option sqlite_all_varchar in sqlite_sanner that comes without default.

This would test the new behaviour of AddExtensionOption via LoadExtensionSettings. This do not check the storage setting path directly, but it's probably the best way to get some coverage / checking behaviour stays the same over time.

To run the test then it would be of:

• building sqlite_scanner and postgres_scanner but don't link them (possibly a workaround is `CORE_EXTENSIONS=sqlite_scanner;postgres_scanner" and saving the content of build/release/repository to path/to/repository)
• rebuild without extension linked
• run the C++ tests path/to/repository as install directory (via a config).

I would need to check how to have such a test run in CI.

I think this changeset is simple enough not to warrant adding a complex CI setup to test it.

Thanks!