Merge pull request ClickHouse#79113 from ClickHouse/settings-lost-replica

Move settings from engine to query level during `recoverLostReplica`

Author: nikitamikhaylov

src/Databases/DatabaseReplicated.cpp

@@ -23,6 +23,7 @@
 #include <Interpreters/DDLTask.h>
 #include <Interpreters/DatabaseCatalog.h>
 #include <Interpreters/InterpreterCreateQuery.h>
+#include <Interpreters/InterpreterSetQuery.h>
 #include <Interpreters/ReplicatedDatabaseQueryStatusSource.h>
 #include <Interpreters/evaluateConstantExpression.h>
 #include <Interpreters/executeDDLQueryOnCluster.h>
@@ -1462,8 +1463,13 @@ void DatabaseReplicated::recoverLostReplica(const ZooKeeperPtr & current_zookeep
                 }
 
                 auto query_ast = parseQueryFromMetadataInZooKeeper(table_name, create_query_string);
-                LOG_INFO(log, "Executing {}", query_ast->formatForLogging());
+
                 auto create_query_context = make_query_context();
+                /// Check larger comment in DatabaseOnDisk::createTableFromAST
+                /// TL;DR applySettingsFromQuery will move the settings from engine to query level
+                /// making it possible to overcome a backward incompatible change.
+                InterpreterSetQuery::applySettingsFromQuery(query_ast, create_query_context);
+                LOG_INFO(log, "Executing {}", query_ast->formatForLogging());
                 InterpreterCreateQuery(query_ast, create_query_context).execute();
             };
 

tests/integration/test_replicated_database/data/clickhouse/part1.csv

@@ -0,0 +1,2 @@
+a,b
+1,2

tests/integration/test_replicated_database/test_settings_recover_lost_replica.py

@@ -0,0 +1,81 @@
+import logging
+import os
+
+import pytest
+
+from helpers.cluster import ClickHouseCluster
+from helpers.config_cluster import minio_secret_key
+from helpers.mock_servers import start_mock_servers
+from helpers.test_tools import TSV
+
+logging.getLogger().setLevel(logging.INFO)
+logging.getLogger().addHandler(logging.StreamHandler())
+
+SCRIPT_DIR = os.path.dirname(os.path.realpath(__file__))
+S3_DATA = [
+    "data/clickhouse/part1.csv",
+]
+
+
+def create_buckets_s3(cluster):
+    minio = cluster.minio_client
+
+    for file in S3_DATA:
+        minio.fput_object(
+            bucket_name=cluster.minio_bucket,
+            object_name=file,
+            file_path=os.path.join(SCRIPT_DIR, file),
+        )
+    for obj in minio.list_objects(cluster.minio_bucket, recursive=True):
+        print(obj.object_name)
+
+
+@pytest.fixture(scope="module")
+def started_cluster():
+    try:
+        cluster = ClickHouseCluster(__file__)
+        # Until 24.10, query level settings were specified in the .sql file
+        cluster.add_instance(
+            "old_node",
+            image="clickhouse/clickhouse-server",
+            tag="24.9.2.42",
+            with_zookeeper=True,
+            with_installed_binary=True,
+            with_minio=True,
+            stay_alive=True,
+        )
+
+        cluster.add_instance(
+            "new_node",
+            with_zookeeper=True,
+        )
+
+        logging.info("Starting cluster...")
+        cluster.start()
+        logging.info("Cluster started")
+
+        create_buckets_s3(cluster)
+
+        yield cluster
+    finally:
+        cluster.shutdown()
+
+
+def test_query_settings_in_create_recover_lost_replica(started_cluster):
+    old_node = started_cluster.instances["old_node"]
+    old_node.query("DROP DATABASE IF EXISTS replicated_lost_replica SYNC")
+    old_node.query(
+        "CREATE DATABASE replicated_lost_replica ENGINE = Replicated('/test/replicated_lost_replica', 'shard1', 'replica' || '1');"
+    )
+    old_node.query("DROP TABLE IF EXISTS replicated_lost_replica.b")
+    old_node.query(
+        f"""CREATE TABLE replicated_lost_replica.b Engine = S3('http://minio1:9001/root/data/clickhouse/part1.csv', 'minio', '{minio_secret_key}') SETTINGS s3_create_new_file_on_insert = 1;"""
+    )
+
+    new_node = started_cluster.instances["new_node"]
+    new_node.query("DROP DATABASE IF EXISTS replicated_lost_replica SYNC")
+    # Adding new replica will trigger the `recoverLostReplica` method.
+    new_node.query(
+        "CREATE DATABASE replicated_lost_replica ENGINE = Replicated('/test/replicated_lost_replica', 'shard1', 'replica' || '2');"
+    )
+    new_node.query("SYSTEM SYNC DATABASE REPLICA replicated_lost_replica")