Update vendored DuckDB sources to c46a01b579

src/duckdb/src/common/error_data.cpp

@@ -80,9 +80,8 @@ void ErrorData::Throw(const string &prepended_message) const {
 	if (!prepended_message.empty()) {
 		string new_message = prepended_message + raw_message;
 		throw Exception(extra_info, type, new_message);
-	} else {
-		throw Exception(extra_info, type, raw_message);
 	}
+	throw Exception(extra_info, type, raw_message);
 }
 
 const ExceptionType &ErrorData::Type() const {

src/duckdb/src/function/table/read_duckdb.cpp

@@ -189,8 +189,9 @@ AttachedDatabaseWrapper::AttachedDatabaseWrapper(ClientContext &context,
 AttachedDatabaseWrapper::~AttachedDatabaseWrapper() {
 	if (attached_database) {
 		auto &db_manager = DatabaseManager::Get(context);
-		db_manager.DetachDatabase(context, attached_database->GetName(), OnEntryNotFound::RETURN_NULL);
+		auto name = attached_database->GetName();
 		attached_database.reset();
+		db_manager.DetachDatabase(context, name, OnEntryNotFound::RETURN_NULL);
 	}
 }
 DuckDBReader::DuckDBReader(ClientContext &context_p, OpenFileInfo file_p, const DuckDBFileReaderOptions &options)

src/duckdb/src/function/table/version/pragma_version.cpp

@@ -1,5 +1,5 @@
 #ifndef DUCKDB_PATCH_VERSION
-#define DUCKDB_PATCH_VERSION "0-dev4877"
+#define DUCKDB_PATCH_VERSION "0-dev4892"
 #endif
 #ifndef DUCKDB_MINOR_VERSION
 #define DUCKDB_MINOR_VERSION 5
@@ -8,10 +8,10 @@
 #define DUCKDB_MAJOR_VERSION 1
 #endif
 #ifndef DUCKDB_VERSION
-#define DUCKDB_VERSION "v1.5.0-dev4877"
+#define DUCKDB_VERSION "v1.5.0-dev4892"
 #endif
 #ifndef DUCKDB_SOURCE_ID
-#define DUCKDB_SOURCE_ID "be5250ba48"
+#define DUCKDB_SOURCE_ID "c46a01b579"
 #endif
 #include "duckdb/function/table/system_functions.hpp"
 #include "duckdb/main/database.hpp"

src/duckdb/src/include/duckdb/main/attached_database.hpp

@@ -8,9 +8,6 @@
 
 #pragma once
 
-#include "duckdb/common/common.hpp"
-#include "duckdb/common/case_insensitive_map.hpp"
-#include "duckdb/common/mutex.hpp"
 #include "duckdb/main/config.hpp"
 #include "duckdb/catalog/catalog_entry.hpp"
 
@@ -39,6 +36,10 @@ enum class AttachVisibility { SHOWN, HIDDEN };
 //! Use this mode with caution, as it disables recovery from crashes for the file.
 enum class RecoveryMode : uint8_t { DEFAULT = 0, NO_WAL_WRITES = 1 };
 
+//! CHECKPOINT: Throws, if the checkpoint fails. Always cleans up.
+//! TRY_CHECKPOINT: Does not throw when failing a checkpoint. Always cleans up.
+enum class DatabaseCloseAction { CHECKPOINT, TRY_CHECKPOINT };
+
 class DatabaseFilePathManager;
 
 struct StoredDatabasePath {
@@ -49,7 +50,6 @@ struct StoredDatabasePath {
 	DatabaseFilePathManager &manager;
 	string path;
 
-public:
 	void OnDetach();
 };
 
@@ -71,7 +71,7 @@ struct AttachOptions {
 	unordered_map<string, Value> options;
 	//! (optionally) a catalog can be provided with a default table
 	QualifiedName default_table;
-	//! Whether or not this is the main database
+	//! Whether this is the main database.
 	bool is_main_database = false;
 	//! The visibility of the attached database
 	AttachVisibility visibility = AttachVisibility::SHOWN;
@@ -94,7 +94,8 @@ class AttachedDatabase : public CatalogEntry, public enable_shared_from_this<Att
 	//! Initializes the catalog and storage of the attached database.
 	void Initialize(optional_ptr<ClientContext> context = nullptr);
 	void FinalizeLoad(optional_ptr<ClientContext> context);
-	void Close();
+	//! Close the database before shutting it down.
+	void Close(const DatabaseCloseAction action);
 
 	Catalog &ParentCatalog() override;
 	const Catalog &ParentCatalog() const override;
@@ -136,6 +137,9 @@ class AttachedDatabase : public CatalogEntry, public enable_shared_from_this<Att
 
 	static bool NameIsReserved(const string &name);
 	static string ExtractDatabaseName(const string &dbpath, FileSystem &fs);
+	// Invoke Close() on an attached database, if its use count is 1.
+	// Only call this in places where you know that the (last) shared pointer is about to go out of scope.
+	static void InvokeCloseIfLastReference(shared_ptr<AttachedDatabase> &attached_database);
 
 private:
 	DatabaseInstance &db;
@@ -150,7 +154,12 @@ class AttachedDatabase : public CatalogEntry, public enable_shared_from_this<Att
 	AttachVisibility visibility = AttachVisibility::SHOWN;
 	bool is_initial_database = false;
 	bool is_closed = false;
+	mutex close_lock;
 	unordered_map<string, Value> attach_options;
+
+private:
+	//! Clean any (shared) resources held by the database.
+	void Cleanup();
 };
 
 } // namespace duckdb

src/duckdb/src/include/duckdb/main/database_manager.hpp

@@ -8,12 +8,8 @@
 
 #pragma once
 
-#include "duckdb/common/atomic.hpp"
 #include "duckdb/common/case_insensitive_map.hpp"
 #include "duckdb/common/common.hpp"
-#include "duckdb/common/enums/access_mode.hpp"
-#include "duckdb/common/enums/on_entry_not_found.hpp"
-#include "duckdb/common/mutex.hpp"
 #include "duckdb/common/optional_ptr.hpp"
 #include "duckdb/main/config.hpp"
 #include "duckdb/parser/parsed_data/attach_info.hpp"
@@ -82,7 +78,7 @@ class DatabaseManager {
 	idx_t ApproxDatabaseCount();
 	//! Removes all databases from the catalog set. This is necessary for the database instance's destructor,
 	//! as the database manager has to be alive when destroying the catalog set objects.
-	void ResetDatabases(unique_ptr<TaskScheduler> &scheduler);
+	void ResetDatabases();
 
 	transaction_t GetNewQueryNumber() {
 		return current_query_number++;

src/duckdb/src/include/duckdb/transaction/meta_transaction.hpp

@@ -9,14 +9,13 @@
 #pragma once
 
 #include "duckdb/common/common.hpp"
-#include "duckdb/common/atomic.hpp"
 #include "duckdb/main/valid_checker.hpp"
 #include "duckdb/common/types/timestamp.hpp"
-#include "duckdb/common/unordered_map.hpp"
 #include "duckdb/common/optional_ptr.hpp"
 #include "duckdb/common/reference_map.hpp"
 #include "duckdb/common/error_data.hpp"
 #include "duckdb/common/case_insensitive_map.hpp"
+#include "duckdb/main/attached_database.hpp"
 
 namespace duckdb {
 class AttachedDatabase;
@@ -63,6 +62,8 @@ class MetaTransaction {
 
 	ErrorData Commit();
 	void Rollback();
+	// Finalize the transaction after a COMMIT of ROLLBACK.
+	void Finalize();
 
 	idx_t GetActiveQuery();
 	void SetActiveQuery(transaction_t query_number);
@@ -82,21 +83,21 @@ class MetaTransaction {
 	void DetachDatabase(AttachedDatabase &database);
 
 private:
-	//! Lock to prevent all_transactions and transactions from getting out of sync
+	//! Lock to prevent all_transactions and transactions from getting out of sync.
 	mutex lock;
-	//! The set of active transactions for each database
+	//! The set of active transactions for each database.
 	reference_map_t<AttachedDatabase, TransactionReference> transactions;
-	//! The set of transactions in order of when they were started
+	//! The set of referenced databases in invocation order.
 	vector<reference<AttachedDatabase>> all_transactions;
-	//! The database we are modifying - we can only modify one database per transaction
+	//! The database we are modifying. We can only modify one database per meta transaction.
 	optional_ptr<AttachedDatabase> modified_database;
-	//! Whether or not the meta transaction is marked as read only
+	//! Whether the meta transaction is marked as read only.
 	bool is_read_only;
-	//! Lock for referenced_databases
+	//! Lock for referenced_databases.
 	mutex referenced_database_lock;
-	//! The set of used / referenced databases
+	//! The set of used (referenced) databases.
 	reference_map_t<AttachedDatabase, shared_ptr<AttachedDatabase>> referenced_databases;
-	//! Map of name -> used database for databases that are in-use by this transaction
+	//! Map of name -> database for databases that are in-use by this transaction.
 	case_insensitive_map_t<reference<AttachedDatabase>> used_databases;
 };
 

src/duckdb/src/main/attached_database.cpp

@@ -152,7 +152,10 @@ AttachedDatabase::AttachedDatabase(DatabaseInstance &db, Catalog &catalog_p, Sto
 }
 
 AttachedDatabase::~AttachedDatabase() {
-	Close();
+	// FIXME: Theoretically, we should catch all places higher up the call stack where the
+	// FIXME: shared pointer goes out of scope and we need to invoke a checkpoint.
+	// FIXME: However, for now, we keep this to avoid unintentional behavior / oversights.
+	Close(DatabaseCloseAction::TRY_CHECKPOINT);
 }
 
 bool AttachedDatabase::IsSystem() const {
@@ -195,6 +198,16 @@ string AttachedDatabase::ExtractDatabaseName(const string &dbpath, FileSystem &f
 	return name;
 }
 
+void AttachedDatabase::InvokeCloseIfLastReference(shared_ptr<AttachedDatabase> &attached_db) {
+	{
+		lock_guard<mutex> guard(attached_db->close_lock);
+		if (attached_db.use_count() == 1) {
+			attached_db->Close(DatabaseCloseAction::CHECKPOINT);
+		}
+	}
+	attached_db.reset();
+}
+
 void AttachedDatabase::Initialize(optional_ptr<ClientContext> context) {
 	if (IsSystem()) {
 		catalog->Initialize(context, true);
@@ -258,40 +271,47 @@ void AttachedDatabase::OnDetach(ClientContext &context) {
 	}
 }
 
-void AttachedDatabase::Close() {
+void AttachedDatabase::Close(const DatabaseCloseAction action) {
 	if (is_closed) {
 		return;
 	}
 	D_ASSERT(catalog);
 	is_closed = true;
 
-	// shutting down: attempt to checkpoint the database
-	// but only if we are not cleaning up as part of an exception unwind
-	if (!Exception::UncaughtException() && storage && !ValidChecker::IsInvalidated(db)) {
-		if (!storage->InMemory()) {
+	try {
+		auto create_checkpoint = true;
+		if (action == DatabaseCloseAction::TRY_CHECKPOINT && Exception::UncaughtException()) {
+			create_checkpoint = false;
+		} else if (!storage || storage->InMemory() || ValidChecker::IsInvalidated(db)) {
+			create_checkpoint = false;
+		}
+
+		if (create_checkpoint) {
+			auto &config = DBConfig::GetConfig(db);
+			if (config.options.checkpoint_on_shutdown) {
+				CheckpointOptions options;
+				options.wal_action = CheckpointWALAction::DELETE_WAL;
+				storage->CreateCheckpoint(QueryContext(), options);
+			}
+		}
+	} catch (std::exception &ex) {
+		ErrorData data(ex);
+		if (action == DatabaseCloseAction::TRY_CHECKPOINT) {
 			try {
-				auto &config = DBConfig::GetConfig(db);
-				if (config.options.checkpoint_on_shutdown) {
-					CheckpointOptions options;
-					options.wal_action = CheckpointWALAction::DELETE_WAL;
-					storage->CreateCheckpoint(QueryContext(), options);
-				}
-			} catch (std::exception &ex) {
-				ErrorData data(ex);
-				try {
-					DUCKDB_LOG_ERROR(db, "AttachedDatabase::Close()\t\t" + data.Message());
-				} catch (...) { // NOLINT
-				}
+				DUCKDB_LOG_ERROR(db, "Silent exception in AttachedDatabase::Close():\t" + data.Message());
 			} catch (...) { // NOLINT
 			}
+		} else {
+			Cleanup();
+			data.Throw("Detached database '" + name + "', but CHECKPOINT during DETACH failed. \n");
 		}
-		try {
-			// destroy the storage
-			storage->Destroy();
-		} catch (...) { // NOLINT
-		}
+	} catch (...) { // NOLINT
 	}
 
+	Cleanup();
+}
+
+void AttachedDatabase::Cleanup() {
 	transaction_manager.reset();
 	catalog.reset();
 	storage.reset();

src/duckdb/src/main/database.cpp

@@ -77,7 +77,7 @@ DatabaseInstance::DatabaseInstance() : db_validity(*this) {
 DatabaseInstance::~DatabaseInstance() {
 	// destroy all attached databases
 	if (db_manager) {
-		db_manager->ResetDatabases(scheduler);
+		db_manager->ResetDatabases();
 	}
 	// destroy child elements
 	connection_manager.reset();

src/duckdb/src/main/database_manager.cpp

@@ -208,6 +208,9 @@ void DatabaseManager::DetachDatabase(ClientContext &context, const string &name,
 	}
 
 	attached_db->OnDetach(context);
+
+	// DetachInternal removes the AttachedDatabase from the list of databases that can be referenced.
+	AttachedDatabase::InvokeCloseIfLastReference(attached_db);
 }
 
 void DatabaseManager::Alter(ClientContext &context, AlterInfo &info) {
@@ -394,10 +397,10 @@ vector<shared_ptr<AttachedDatabase>> DatabaseManager::GetDatabases() {
 	return result;
 }
 
-void DatabaseManager::ResetDatabases(unique_ptr<TaskScheduler> &scheduler) {
-	auto databases = GetDatabases();
-	for (auto &entry : databases) {
-		entry->Close();
+void DatabaseManager::ResetDatabases() {
+	auto shared_db_pointers = GetDatabases();
+	for (auto &entry : shared_db_pointers) {
+		entry->Close(DatabaseCloseAction::TRY_CHECKPOINT);
 		entry.reset();
 	}
 }

src/duckdb/src/transaction/meta_transaction.cpp

@@ -174,6 +174,15 @@ void MetaTransaction::Rollback() {
 	}
 }
 
+void MetaTransaction::Finalize() {
+	// Try to checkpoint any attached databases potentially still held by this transaction.
+	for (auto &database : referenced_databases) {
+		// If the use count is down to one, then we already detached the database.
+		// That means new transactions can no longer obtain a shared pointer to it.
+		AttachedDatabase::InvokeCloseIfLastReference(database.second);
+	}
+}
+
 idx_t MetaTransaction::GetActiveQuery() {
 	return active_query;
 }

src/duckdb/src/transaction/transaction_context.cpp

@@ -6,6 +6,7 @@
 #include "duckdb/main/client_data.hpp"
 #include "duckdb/main/database.hpp"
 #include "duckdb/transaction/meta_transaction.hpp"
+#include "duckdb/main/attached_database.hpp"
 
 namespace duckdb {
 
@@ -55,11 +56,11 @@ void TransactionContext::Commit() {
 			s->TransactionRollback(*transaction, context, error);
 		}
 		throw TransactionException("Failed to commit: %s", error.RawMessage());
-	} else {
-		for (auto &state : context.registered_state->States()) {
-			state->TransactionCommit(*transaction, context);
-		}
 	}
+	for (auto &state : context.registered_state->States()) {
+		state->TransactionCommit(*transaction, context);
+	}
+	transaction->Finalize();
 }
 
 void TransactionContext::SetAutoCommit(bool value) {
@@ -94,6 +95,7 @@ void TransactionContext::Rollback(optional_ptr<ErrorData> error) {
 	if (rollback_error.HasError()) {
 		rollback_error.Throw();
 	}
+	transaction->Finalize();
 }
 
 void TransactionContext::ClearTransaction() {