ATTACH OR REPLACE database to allow swapping of new data. #15355
Conversation
|
Thanks for the PR! Looks good to me - good idea. |
|
Cool idea! I looked at this trying to find problems with the fact that dependencies are left hanging, but DuckDB seems to handle that fine (errors do make sense), and given this is opt-in it's likely OK. One bit that is not working as it should is: ATTACH OR REPLACE 'name.db' as db;
--- works
ATTACH 'sqlite:name.db' as db2;
--- fails on load of sqlite_scanner, see belowATTACH OR REPLACE 'name.db' as db;
--- works
ATTACH OR REPLACE 'sqlite:name.db' as db;
--- worksI think There might be other corner cases in this area, or code might be added in the future, so maybe it's best to share more code between ATTACH OR REPLACE and simple ATTACH. EDIT: actually the problem is only given the name is the same, and so it get skipped due to the early check on the name. Unsure if including the type should make this work there. |
| return SourceResultType::FINISHED; | ||
| if (info->on_conflict == OnCreateConflict::REPLACE_ON_CONFLICT) { | ||
| // same path, same name, DB does not need replacing | ||
| if (existing_db->GetCatalog().GetDBPath() == path) { |
There was a problem hiding this comment.
As found by @carlopi this needs to check GetCatalogType as well
db33648 to
6b1d755
Compare
|
Thanks for the review! Great catch on the types, I'll add a check to ensure we only skip reattach if it also matches the type. I feel like same path/name but different type should be an explicit error, since if a DuckDB/SQLite file is attached at a given path and in active use, it's hard to think of a valid use case where it would be externally overwritten as a different type. For now I've left it as is, but please let me know if it makes sense to throw a BinderError instead, since it could highlight user error of trying to reattach with a different type by mistake. I may have found a separate bug. It seems if you ATTACH a duckdb file but claim that it's an sqlite file, the ATTACH will succeed, but this will lead to issues later. I might file this one separately once confirmed. |
|
Sqlite was an example that do not have a lot of practical sense given a file can't be both formats at the same time, but extensions can potentially override/change behaviour in various ways, so given we support that I though it was worth supporting it also here. Idea is that The fact that |
|
@carlopi Ah ok, that makes sense to me. I'll leave it as-is then, thanks for the explanation. Should I update the ATTACH documentation to reflect the new syntax, or is it better to wait for review/tests to complete first? |
|
@xevix: sending a PR to duckdb/duckdb-web making clear in the PR message that it's connected to a 1.2 feature would be great. If PR is ready, can you undraft the PR? (we draft them automatically on changes) |
|
@carlopi I think things should be good to go, but please let me know if there’s anything else I should do. Good luck with the 1.2 release 🎉 |
|
Hey all, I retested the PR merging in latest main so it should still be good to go. Might have to roll this documentation back until then duckdb/duckdb-web#4467. |
|
Thanks for having a look. I tested building Using the |
|
Can we not test this without the SQLite scanner? Adding a test depending on the SQLite scanner to the main repo means the test will not be triggered in most of the CI here. |
|
I think so, if there's another type of file-based DB that can be I can also remove this line from the test, it was mainly to address @carlopi 's concern above. Please let me know and I'll remove it. |
|
httpfs is a bit of a special case - there are CI runs that specifically run httpfs test cases that are part of the DuckDB test suite. This doesn't apply to other out-of-tree extensions. I'm fine with removing the line from the test. |
|
Thanks! |
ATTACH OR REPLACE database to allow swapping of new data. (duckdb/duckdb#15355)
ATTACH OR REPLACE database to allow swapping of new data. (duckdb/duckdb#15355)
ATTACH OR REPLACE database to allow swapping of new data. (duckdb/duckdb#15355)
ATTACH OR REPLACE database to allow swapping of new data. (duckdb/duckdb#15355)
ATTACH OR REPLACE database to allow swapping of new data. (duckdb/duckdb#15355)
ATTACH OR REPLACE database to allow swapping of new data. (duckdb/duckdb#15355)
Addresses #15208
Adds support for replacing existing attached database.
This is just a proposal for more concrete discussion, but no worries at all if this doesn't fit with the vision for ATTACH functionality in DuckDB. This is my first PR to DuckDB so apologies ahead of time if I missed something critical.
Example usage:
Application query: