From 10b6a3574b7dcf763e5daaa21e027937430fee70 Mon Sep 17 00:00:00 2001 From: Tuan Pham Anh Date: Mon, 3 Nov 2025 11:34:32 +0000 Subject: [PATCH 1/2] Merge pull request #88490 from tuanpach/not-support-transaction-for-non-append-supporting-disks Not support transaction for tables with disks which do not support writing with append. Signed-off-by: Anton Ivashkin --- src/Disks/ObjectStorages/IMetadataStorage.h | 3 ++ .../MetadataStorageFromDisk.cpp | 6 +++ .../ObjectStorages/MetadataStorageFromDisk.h | 2 + src/Interpreters/IInterpreter.cpp | 12 +++-- src/Storages/StorageMergeTree.cpp | 50 ++++++++++++------- src/Storages/StorageMergeTree.h | 4 +- tests/config/config.d/storage_conf.xml | 14 ++++++ ...ge_tree_disk_support_transaction.reference | 0 ...57_merge_tree_disk_support_transaction.sql | 41 +++++++++++++++ 9 files changed, 109 insertions(+), 23 deletions(-) create mode 100644 tests/queries/0_stateless/03657_merge_tree_disk_support_transaction.reference create mode 100644 tests/queries/0_stateless/03657_merge_tree_disk_support_transaction.sql diff --git a/src/Disks/ObjectStorages/IMetadataStorage.h b/src/Disks/ObjectStorages/IMetadataStorage.h index f193c78dfc78..c7c8de877c4a 100644 --- a/src/Disks/ObjectStorages/IMetadataStorage.h +++ b/src/Disks/ObjectStorages/IMetadataStorage.h @@ -325,6 +325,9 @@ class IMetadataStorage : private boost::noncopyable const std::string & /* config_prefix */, ContextPtr /* context */) {} + /// Only support writing with Append if MetadataTransactionPtr created by `createTransaction` has `supportAddingBlobToMetadata` + virtual bool supportWritingWithAppend() const { return false; } + protected: [[noreturn]] static void throwNotImplemented() { diff --git a/src/Disks/ObjectStorages/MetadataStorageFromDisk.cpp b/src/Disks/ObjectStorages/MetadataStorageFromDisk.cpp index a30d3081c639..72ebe2abf3d8 100644 --- a/src/Disks/ObjectStorages/MetadataStorageFromDisk.cpp +++ b/src/Disks/ObjectStorages/MetadataStorageFromDisk.cpp @@ -134,6 +134,12 @@ MetadataTransactionPtr MetadataStorageFromDisk::createTransaction() return std::make_shared(*this); } +/// It supports writing with Append because `createTransaction` creates `MetadataStorageFromDiskTransaction` which has `supportAddingBlobToMetadata` +bool MetadataStorageFromDisk::supportWritingWithAppend() const +{ + return true; +} + StoredObjects MetadataStorageFromDisk::getStorageObjects(const std::string & path) const { auto metadata = readMetadata(path); diff --git a/src/Disks/ObjectStorages/MetadataStorageFromDisk.h b/src/Disks/ObjectStorages/MetadataStorageFromDisk.h index 75a26e83604c..ce3c7dcc3129 100644 --- a/src/Disks/ObjectStorages/MetadataStorageFromDisk.h +++ b/src/Disks/ObjectStorages/MetadataStorageFromDisk.h @@ -33,6 +33,8 @@ class MetadataStorageFromDisk final : public IMetadataStorage MetadataTransactionPtr createTransaction() override; + bool supportWritingWithAppend() const override; + const std::string & getPath() const override; MetadataStorageType getType() const override { return MetadataStorageType::Local; } diff --git a/src/Interpreters/IInterpreter.cpp b/src/Interpreters/IInterpreter.cpp index dc375c0a908d..be42c0ca91af 100644 --- a/src/Interpreters/IInterpreter.cpp +++ b/src/Interpreters/IInterpreter.cpp @@ -1,9 +1,10 @@ #include -#include +#include #include #include -#include #include +#include +#include namespace DB { @@ -48,10 +49,13 @@ void IInterpreter::checkStorageSupportsTransactionsIfNeeded(const StoragePtr & s throw Exception(ErrorCodes::NOT_IMPLEMENTED, "Storage {} (table {}) does not support transactions", storage->getName(), storage->getStorageID().getNameForLogs()); - /// Do not allow transactions with replicated tables anyway (unless it's a readonly SELECT query) + if (is_readonly_query) + return; + + /// Do not allow transactions with replicated tables or MergeTree tables anyway (unless it's a readonly SELECT query) /// because it may try to process transaction on MergeTreeData-level, /// but then fail with a logical error or something on Storage{Replicated,Shared}MergeTree-level. - if (!is_readonly_query && storage->supportsReplication()) + if (storage->supportsReplication() || dynamic_cast(storage.get()) != nullptr) throw Exception(ErrorCodes::NOT_IMPLEMENTED, "{} (table {}) does not support transactions", storage->getName(), storage->getStorageID().getNameForLogs()); } diff --git a/src/Storages/StorageMergeTree.cpp b/src/Storages/StorageMergeTree.cpp index 31e76d188a64..a60443015d77 100644 --- a/src/Storages/StorageMergeTree.cpp +++ b/src/Storages/StorageMergeTree.cpp @@ -4,9 +4,12 @@ #include #include +#include +#include #include #include #include +#include #include #include #include @@ -24,6 +27,7 @@ #include #include #include +#include #include #include #include @@ -35,12 +39,11 @@ #include #include #include +#include +#include #include #include #include -#include -#include -#include #include #include #include @@ -48,8 +51,6 @@ #include #include #include -#include -#include namespace ProfileEvents @@ -146,6 +147,22 @@ static MergeTreeTransactionPtr tryGetTransactionForMutation(const MergeTreeMutat return {}; } +static bool supportTransaction(const Disks & disks, LoggerPtr log) +{ + for (const auto & disk : disks) + { + if (auto * object_storage = dynamic_cast(disk.get())) + { + if (!object_storage->getMetadataStorage()->supportWritingWithAppend()) + { + LOG_DEBUG(log, "Disk {} does not support writing with append", object_storage->getName()); + return false; + } + } + } + return true; +} + StorageMergeTree::StorageMergeTree( const StorageID & table_id_, const String & relative_data_path_, @@ -156,17 +173,18 @@ StorageMergeTree::StorageMergeTree( const MergingParams & merging_params_, std::unique_ptr storage_settings_) : MergeTreeData( - table_id_, - metadata_, - context_, - date_column_name, - merging_params_, - std::move(storage_settings_), - false, /// require_part_metadata - mode) + table_id_, + metadata_, + context_, + date_column_name, + merging_params_, + std::move(storage_settings_), + false, /// require_part_metadata + mode) , reader(*this) , writer(*this) , merger_mutator(*this) + , support_transaction(supportTransaction(getDisks(), log.load())) { initializeDirectoriesAndFormatVersion(relative_data_path_, LoadingStrictnessLevel::ATTACH <= mode, date_column_name); @@ -184,10 +202,7 @@ StorageMergeTree::StorageMergeTree( loadMutations(); loadDeduplicationLog(); - prewarmCaches( - getActivePartsLoadingThreadPool().get(), - getMarkCacheToPrewarm(0), - getPrimaryIndexCacheToPrewarm(0)); + prewarmCaches(getActivePartsLoadingThreadPool().get(), getMarkCacheToPrewarm(0), getPrimaryIndexCacheToPrewarm(0)); } @@ -2931,5 +2946,4 @@ CommittingBlocksSet StorageMergeTree::getCommittingBlocks() const std::lock_guard lock(committing_blocks_mutex); return committing_blocks; } - } diff --git a/src/Storages/StorageMergeTree.h b/src/Storages/StorageMergeTree.h index 0bffa6ead7d3..8acc15b96487 100644 --- a/src/Storages/StorageMergeTree.h +++ b/src/Storages/StorageMergeTree.h @@ -57,7 +57,7 @@ class StorageMergeTree final : public MergeTreeData bool supportsParallelInsert() const override { return true; } - bool supportsTransactions() const override { return true; } + bool supportsTransactions() const override { return support_transaction; } void read( QueryPlan & query_plan, @@ -178,6 +178,8 @@ class StorageMergeTree final : public MergeTreeData std::map mutation_prepared_sets_cache; PlainLightweightUpdatesSync lightweight_updates_sync; + const bool support_transaction; + void loadMutations(); /// Load and initialize deduplication logs. Even if deduplication setting diff --git a/tests/config/config.d/storage_conf.xml b/tests/config/config.d/storage_conf.xml index 41fb46434319..203eb6eaebed 100644 --- a/tests/config/config.d/storage_conf.xml +++ b/tests/config/config.d/storage_conf.xml @@ -42,6 +42,14 @@ 0 0 + + object_storage + s3 + plain_rewritable + http://localhost:11111/test/s3_plain_rewritable/ + clickhouse + clickhouse + object_storage @@ -79,6 +87,12 @@ 22548578304 1 + + object_storage + local + disks/test/local_plain_rewritable/ + plain_rewritable + cache diff --git a/tests/queries/0_stateless/03657_merge_tree_disk_support_transaction.reference b/tests/queries/0_stateless/03657_merge_tree_disk_support_transaction.reference new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/tests/queries/0_stateless/03657_merge_tree_disk_support_transaction.sql b/tests/queries/0_stateless/03657_merge_tree_disk_support_transaction.sql new file mode 100644 index 000000000000..dfc668920825 --- /dev/null +++ b/tests/queries/0_stateless/03657_merge_tree_disk_support_transaction.sql @@ -0,0 +1,41 @@ +-- Tags: no-ordinary-database, no-fasttest, no-encrypted-storage, no-async-insert + +CREATE OR REPLACE TABLE t (x INT) ENGINE=MergeTree ORDER BY x; +SET implicit_transaction=True; +INSERT INTO TABLE t VALUES (1); +SET implicit_transaction=False; + +CREATE OR REPLACE TABLE t (x INT) ENGINE=MergeTree ORDER BY x SETTINGS disk='local_disk'; +SET implicit_transaction=True; +INSERT INTO TABLE t VALUES (2); +SET implicit_transaction=False; + +CREATE OR REPLACE TABLE t (x INT) ENGINE=MergeTree ORDER BY x SETTINGS disk='local_disk_2'; +SET implicit_transaction=True; +INSERT INTO TABLE t VALUES (3); +SET implicit_transaction=False; + +CREATE OR REPLACE TABLE t (x INT) ENGINE=MergeTree ORDER BY x SETTINGS disk='local_disk_3'; +SET implicit_transaction=True; +INSERT INTO TABLE t VALUES (4); +SET implicit_transaction=False; + +CREATE OR REPLACE TABLE t (x INT) ENGINE=MergeTree ORDER BY x SETTINGS disk='s3_disk'; +SET implicit_transaction=True; +INSERT INTO TABLE t VALUES (5); +SET implicit_transaction=False; + +CREATE OR REPLACE TABLE t (x INT) ENGINE=MergeTree ORDER BY x SETTINGS disk='s3_plain_rewritable'; +SET implicit_transaction=True; +INSERT INTO TABLE t VALUES (6); -- { serverError NOT_IMPLEMENTED } +SET implicit_transaction=False; + +CREATE OR REPLACE TABLE t (x INT) ENGINE=MergeTree ORDER BY x SETTINGS disk='s3_cache'; +SET implicit_transaction=True; +INSERT INTO TABLE t VALUES (7); +SET implicit_transaction=False; + +CREATE OR REPLACE TABLE t (x INT) ENGINE=MergeTree ORDER BY x SETTINGS disk='local_plain_rewritable'; +SET implicit_transaction=True; +INSERT INTO TABLE t VALUES (8); -- { serverError NOT_IMPLEMENTED } +SET implicit_transaction=False; From 7877582d7adf7391904d4256754c9d8b301b65de Mon Sep 17 00:00:00 2001 From: Tuan Pham Anh Date: Mon, 17 Nov 2025 08:36:51 +0000 Subject: [PATCH 2/2] Merge pull request #89999 from tuanpach/fix-84669-2nd-attempt Check if encrypted disks support writing with append Signed-off-by: Anton Ivashkin --- src/Disks/supportWritingWithAppend.cpp | 21 ++++++++++ src/Disks/supportWritingWithAppend.h | 7 ++++ src/Storages/StorageMergeTree.cpp | 11 ++---- tests/config/config.d/storage_conf.xml | 27 +++++++++++++ ...57_merge_tree_disk_support_transaction.sql | 38 +++++++++++++++---- 5 files changed, 90 insertions(+), 14 deletions(-) create mode 100644 src/Disks/supportWritingWithAppend.cpp create mode 100644 src/Disks/supportWritingWithAppend.h diff --git a/src/Disks/supportWritingWithAppend.cpp b/src/Disks/supportWritingWithAppend.cpp new file mode 100644 index 000000000000..80d342c4fde6 --- /dev/null +++ b/src/Disks/supportWritingWithAppend.cpp @@ -0,0 +1,21 @@ +#include + +#include + +namespace DB +{ +bool supportWritingWithAppend(const DiskPtr & disk) +{ + auto target_disk = disk; + if (auto delegate_disk = target_disk->getDelegateDiskIfExists()) + target_disk = delegate_disk; + + if (auto * object_storage = dynamic_cast(target_disk.get())) + { + if (!object_storage->getMetadataStorage()->supportWritingWithAppend()) + return false; + } + return true; +} +} + diff --git a/src/Disks/supportWritingWithAppend.h b/src/Disks/supportWritingWithAppend.h new file mode 100644 index 000000000000..ce133c9e335c --- /dev/null +++ b/src/Disks/supportWritingWithAppend.h @@ -0,0 +1,7 @@ +#pragma once + +#include +namespace DB +{ +bool supportWritingWithAppend(const DiskPtr & disk); +} diff --git a/src/Storages/StorageMergeTree.cpp b/src/Storages/StorageMergeTree.cpp index a60443015d77..a3bf8b4b8118 100644 --- a/src/Storages/StorageMergeTree.cpp +++ b/src/Storages/StorageMergeTree.cpp @@ -9,7 +9,7 @@ #include #include #include -#include +#include #include #include #include @@ -151,13 +151,10 @@ static bool supportTransaction(const Disks & disks, LoggerPtr log) { for (const auto & disk : disks) { - if (auto * object_storage = dynamic_cast(disk.get())) + if (!supportWritingWithAppend(disk)) { - if (!object_storage->getMetadataStorage()->supportWritingWithAppend()) - { - LOG_DEBUG(log, "Disk {} does not support writing with append", object_storage->getName()); - return false; - } + LOG_DEBUG(log, "Disk {} does not support writing with append", disk->getName()); + return false; } } return true; diff --git a/tests/config/config.d/storage_conf.xml b/tests/config/config.d/storage_conf.xml index 203eb6eaebed..ab0ddf34f418 100644 --- a/tests/config/config.d/storage_conf.xml +++ b/tests/config/config.d/storage_conf.xml @@ -50,6 +50,16 @@ clickhouse clickhouse + + cache + s3_plain_rewritable + s3_plain_rewritable_cache/ + 128Mi + 1 + 0 + 0 + 0 + object_storage @@ -113,6 +123,23 @@ clickhouse 0 + + cache + s3_plain_rewritable_cache + s3_plain_rewritable_cache_multi/ + 22548578304 + + + cache + local_cache + local_cache_multi/ + 22548578304 + + + encrypted + s3_plain_rewritable_cache + 1234567812345678 + diff --git a/tests/queries/0_stateless/03657_merge_tree_disk_support_transaction.sql b/tests/queries/0_stateless/03657_merge_tree_disk_support_transaction.sql index dfc668920825..65d2330b1bd3 100644 --- a/tests/queries/0_stateless/03657_merge_tree_disk_support_transaction.sql +++ b/tests/queries/0_stateless/03657_merge_tree_disk_support_transaction.sql @@ -7,35 +7,59 @@ SET implicit_transaction=False; CREATE OR REPLACE TABLE t (x INT) ENGINE=MergeTree ORDER BY x SETTINGS disk='local_disk'; SET implicit_transaction=True; -INSERT INTO TABLE t VALUES (2); +INSERT INTO TABLE t VALUES (1); SET implicit_transaction=False; CREATE OR REPLACE TABLE t (x INT) ENGINE=MergeTree ORDER BY x SETTINGS disk='local_disk_2'; SET implicit_transaction=True; -INSERT INTO TABLE t VALUES (3); +INSERT INTO TABLE t VALUES (1); SET implicit_transaction=False; CREATE OR REPLACE TABLE t (x INT) ENGINE=MergeTree ORDER BY x SETTINGS disk='local_disk_3'; SET implicit_transaction=True; -INSERT INTO TABLE t VALUES (4); +INSERT INTO TABLE t VALUES (1); SET implicit_transaction=False; CREATE OR REPLACE TABLE t (x INT) ENGINE=MergeTree ORDER BY x SETTINGS disk='s3_disk'; SET implicit_transaction=True; -INSERT INTO TABLE t VALUES (5); +INSERT INTO TABLE t VALUES (1); SET implicit_transaction=False; CREATE OR REPLACE TABLE t (x INT) ENGINE=MergeTree ORDER BY x SETTINGS disk='s3_plain_rewritable'; SET implicit_transaction=True; -INSERT INTO TABLE t VALUES (6); -- { serverError NOT_IMPLEMENTED } +INSERT INTO TABLE t VALUES (1); -- { serverError NOT_IMPLEMENTED } SET implicit_transaction=False; CREATE OR REPLACE TABLE t (x INT) ENGINE=MergeTree ORDER BY x SETTINGS disk='s3_cache'; SET implicit_transaction=True; -INSERT INTO TABLE t VALUES (7); +INSERT INTO TABLE t VALUES (1); SET implicit_transaction=False; CREATE OR REPLACE TABLE t (x INT) ENGINE=MergeTree ORDER BY x SETTINGS disk='local_plain_rewritable'; SET implicit_transaction=True; -INSERT INTO TABLE t VALUES (8); -- { serverError NOT_IMPLEMENTED } +INSERT INTO TABLE t VALUES (1); -- { serverError NOT_IMPLEMENTED } +SET implicit_transaction=False; + +CREATE OR REPLACE TABLE t (x INT) ENGINE=MergeTree ORDER BY x SETTINGS disk='s3_plain_rewritable_cache'; +SET implicit_transaction=True; +INSERT INTO TABLE t VALUES (1); -- { serverError NOT_IMPLEMENTED } +SET implicit_transaction=False; + +CREATE OR REPLACE TABLE t (x INT) ENGINE=MergeTree ORDER BY x SETTINGS disk='s3_plain_rewritable_cache_multi'; +SET implicit_transaction=True; +INSERT INTO TABLE t VALUES (1); -- { serverError NOT_IMPLEMENTED } SET implicit_transaction=False; + +CREATE OR REPLACE TABLE t (x INT) ENGINE=MergeTree ORDER BY x SETTINGS disk='local_cache'; +SET implicit_transaction=True; +INSERT INTO TABLE t VALUES (1); +SET implicit_transaction=False; + +CREATE OR REPLACE TABLE t (x INT) ENGINE=MergeTree ORDER BY x SETTINGS disk='local_cache_multi'; +SET implicit_transaction=True; +INSERT INTO TABLE t VALUES (1); +SET implicit_transaction=False; + +CREATE OR REPLACE TABLE t (x INT) ENGINE=MergeTree ORDER BY x SETTINGS disk='encrypted_s3_plain_rewritable_cache'; +SET implicit_transaction=True; +INSERT INTO TABLE t VALUES (1); -- { serverError NOT_IMPLEMENTED } \ No newline at end of file