Implement DatabaseManager::SetDefaultDatabase #7878
Conversation
The `DatabaseManager::default_database` is returned by `DatabaseManager::GetDefaultDatabase` when the default catalog in the search path is invalid. This commit adds a method to change `default_database`. This makes it possible that new `Connection`s can use a database other than 'memory' as default.
src/main/database_manager.cpp
Outdated
| auto has_active_transaction = context.transaction.HasActiveTransaction(); | ||
| if (!has_active_transaction) { | ||
| context.transaction.BeginTransaction(); | ||
| } | ||
|
|
||
| auto db_entry = GetDatabase(context, new_value); | ||
|
|
||
| if (!has_active_transaction) { | ||
| context.transaction.ClearTransaction(); | ||
| } |
There was a problem hiding this comment.
I want to validate here that the database referred to by new_value actually exists. However, GetDatabase requires a transaction to be active. What is the best way to create a transaction when non is active? Or is there another way to validate that the database exists which does not require to start a transaction?
There was a problem hiding this comment.
RunFunctionInTransaction
it's part of ClientContext, client_context.cpp:969
There was a problem hiding this comment.
Thank you for the hint. I changed the code to use this function.
src/main/database_manager.cpp
Outdated
| void DatabaseManager::SetDefaultDatabase(ClientContext &context, const string &new_value) { | ||
| optional_ptr<AttachedDatabase> db_entry; | ||
|
|
||
| context.RunFunctionInTransaction([&]() { db_entry = GetDatabase(context, new_value); }); |
There was a problem hiding this comment.
RunFunctionInTransaction is very heavy-handed. It launches a new transaction if none is active (and commits it afterwards). Generally almost all code in DuckDB (be it parser, planner, optimizer, executor) is run with an active transaction already. Is this necessary only for the unittest? I imagine that when this function is used internally it will be used in locations where a transaction already exists.
There was a problem hiding this comment.
Indeed I added this to make a unit test case succeed in which I did not start a transaction before calling SetDefaultDatabase. Otherwise, an InternalException with message "TransactionContext::ActiveTransaction called without active transaction" would be thrown. I thought that it would be worth avoiding this exception by starting a new transaction, considering that RunFunctionInTransaction does not have a significant overhead when there is already an active transaction, does it?
But if I understand you correctly, you think that it is better to remove RunFunctionInTransaction and just call GetDatabase directly?
There was a problem hiding this comment.
Yes, most functions in the catalog rely on a transaction existing. I think it is better to remove it.
There was a problem hiding this comment.
All right, I removed the call to RunFunctionInTransaction. Thank you for the feedback.
…faultDatabase' Most functions that call 'SetDefaultDatabase' will already have an active transaction, hence it is not necessary to create a new one.
The
DatabaseManager::default_databaseis returned byDatabaseManager::GetDefaultDatabasewhen the default catalog in the search path is invalid. In this PR, a methodSetDefaultDatabaseis added which can be used to changedefault_database.With this change, new
Connections can use a database as default other than the one that was added first to theDatabaseManager.The value for
default_databasecan only be changed internally by callingSetDefaultDatabase. There is noSET GLOBALoption for this implemented in this PR.I have written a unit test for this, but did not know where to put it. If you could tell me a good location for it, I can add it to the PR, too.