Backport #11739 to 20.3: [RFC] Parse metadata stored in zookeeper before checking for equality

robot-clickhouse · robot-clickhouse · commit 5cf1c2556ea2 · 2020-07-27T18:21:26.000+03:00
diff --git a/src/Storages/MergeTree/ReplicatedMergeTreeTableMetadata.cpp b/src/Storages/MergeTree/ReplicatedMergeTreeTableMetadata.cpp
@@ -210,7 +210,7 @@ void ReplicatedMergeTreeTableMetadata::checkImmutableFieldsEquals(const Replicat
 
 }
 
-void ReplicatedMergeTreeTableMetadata::checkEquals(const ReplicatedMergeTreeTableMetadata & from_zk) const
+void ReplicatedMergeTreeTableMetadata::checkEquals(const ReplicatedMergeTreeTableMetadata & from_zk, const ColumnsDescription & columns, const Context & context) const
 {
 
     checkImmutableFieldsEquals(from_zk);
@@ -232,20 +232,24 @@ void ReplicatedMergeTreeTableMetadata::checkEquals(const ReplicatedMergeTreeTabl
                 ErrorCodes::METADATA_MISMATCH);
     }
 
-    if (skip_indices != from_zk.skip_indices)
+    String parsed_zk_skip_indices = IndicesDescription::parse(from_zk.skip_indices, columns, context).toString();
+    if (skip_indices != parsed_zk_skip_indices)
     {
         throw Exception(
                 "Existing table metadata in ZooKeeper differs in skip indexes."
                 " Stored in ZooKeeper: " + from_zk.skip_indices +
+                ", parsed from ZooKeeper: " + parsed_zk_skip_indices +
                 ", local: " + skip_indices,
                 ErrorCodes::METADATA_MISMATCH);
     }
 
-    if (constraints != from_zk.constraints)
+    String parsed_zk_constraints = ConstraintsDescription::parse(from_zk.constraints).toString();
+    if (constraints != parsed_zk_constraints)
     {
         throw Exception(
                 "Existing table metadata in ZooKeeper differs in constraints."
                 " Stored in ZooKeeper: " + from_zk.constraints +
+                ", parsed from ZooKeeper: " + parsed_zk_constraints +
                 ", local: " + constraints,
                        ErrorCodes::METADATA_MISMATCH);
     }
diff --git a/src/Storages/MergeTree/ReplicatedMergeTreeTableMetadata.h b/src/Storages/MergeTree/ReplicatedMergeTreeTableMetadata.h
@@ -60,7 +60,7 @@ struct ReplicatedMergeTreeTableMetadata
         }
     };
 
-    void checkEquals(const ReplicatedMergeTreeTableMetadata & from_zk) const;
+    void checkEquals(const ReplicatedMergeTreeTableMetadata & from_zk, const ColumnsDescription & columns, const Context & context) const;
 
     Diff checkAndFindDiff(const ReplicatedMergeTreeTableMetadata & from_zk) const;
 
diff --git a/src/Storages/StorageReplicatedMergeTree.cpp b/src/Storages/StorageReplicatedMergeTree.cpp
@@ -458,7 +458,7 @@ void StorageReplicatedMergeTree::checkTableStructure(const String & zookeeper_pr
     Coordination::Stat metadata_stat;
     String metadata_str = zookeeper->get(zookeeper_prefix + "/metadata", &metadata_stat);
     auto metadata_from_zk = ReplicatedMergeTreeTableMetadata::parse(metadata_str);
-    old_metadata.checkEquals(metadata_from_zk);
+    old_metadata.checkEquals(metadata_from_zk, getColumns(), global_context);
 
     Coordination::Stat columns_stat;
     auto columns_from_zk = ColumnsDescription::parse(zookeeper->get(zookeeper_prefix + "/columns", &columns_stat));
diff --git a/tests/integration/test_replicated_parse_zk_metadata/__init__.py b/tests/integration/test_replicated_parse_zk_metadata/__init__.py
diff --git a/tests/integration/test_replicated_parse_zk_metadata/test.py b/tests/integration/test_replicated_parse_zk_metadata/test.py
@@ -0,0 +1,48 @@
+import pytest
+
+from helpers.cluster import ClickHouseCluster
+
+cluster = ClickHouseCluster(__file__)
+node = cluster.add_instance('node', with_zookeeper=True)
+
+@pytest.fixture(scope='module', autouse=True)
+def started_cluster():
+    try:
+        cluster.start()
+        yield cluster
+    finally:
+        cluster.shutdown()
+
+def test_replicated_engine_parse_metadata_on_attach():
+    node.query(
+    '''
+    CREATE TABLE data (
+        key Int,
+        INDEX key_idx0 key+0 TYPE minmax GRANULARITY 1,
+        INDEX key_idx1 key+1 TYPE minmax GRANULARITY 1
+    )
+    ENGINE = ReplicatedMergeTree('/ch/tables/default/data', 'node')
+    ORDER BY key;
+    ''')
+    node.query('DETACH TABLE data')
+
+    zk = cluster.get_kazoo_client('zoo1')
+    # Add **extra space between indices**, to check that it will be re-parsed
+    # and successfully accepted by the server.
+    #
+    # This metadata was obtain from the server without #11325
+    zk.set('/ch/tables/default/data/replicas/node/metadata', """
+metadata format version: 1
+date column: 
+sampling expression: 
+index granularity: 8192
+mode: 0
+sign column: 
+primary key: key
+data format version: 1
+partition key: 
+indices:  key_idx0 key + 0 TYPE minmax GRANULARITY 1,  key_idx1 key + 1 TYPE minmax GRANULARITY 1
+granularity bytes: 10485760
+
+""".lstrip())
+    node.query('ATTACH TABLE data')

Original file line number	Diff line number	Diff line change
`@@ -210,7 +210,7 @@ void ReplicatedMergeTreeTableMetadata::checkImmutableFieldsEquals(const Replicat`
`210`	`210`
`211`	`211`	`}`
`212`	`212`
`213`		`-void ReplicatedMergeTreeTableMetadata::checkEquals(const ReplicatedMergeTreeTableMetadata & from_zk) const`
	`213`	`+void ReplicatedMergeTreeTableMetadata::checkEquals(const ReplicatedMergeTreeTableMetadata & from_zk, const ColumnsDescription & columns, const Context & context) const`
`214`	`214`	`{`
`215`	`215`
`216`	`216`	`checkImmutableFieldsEquals(from_zk);`
`@@ -232,20 +232,24 @@ void ReplicatedMergeTreeTableMetadata::checkEquals(const ReplicatedMergeTreeTabl`
`232`	`232`	`ErrorCodes::METADATA_MISMATCH);`
`233`	`233`	`}`
`234`	`234`
`235`		`- if (skip_indices != from_zk.skip_indices)`
	`235`	`+ String parsed_zk_skip_indices = IndicesDescription::parse(from_zk.skip_indices, columns, context).toString();`
	`236`	`+ if (skip_indices != parsed_zk_skip_indices)`
`236`	`237`	`{`
`237`	`238`	`throw Exception(`
`238`	`239`	`"Existing table metadata in ZooKeeper differs in skip indexes."`
`239`	`240`	`" Stored in ZooKeeper: " + from_zk.skip_indices +`
	`241`	`+ ", parsed from ZooKeeper: " + parsed_zk_skip_indices +`
`240`	`242`	`", local: " + skip_indices,`
`241`	`243`	`ErrorCodes::METADATA_MISMATCH);`
`242`	`244`	`}`
`243`	`245`
`244`		`- if (constraints != from_zk.constraints)`
	`246`	`+ String parsed_zk_constraints = ConstraintsDescription::parse(from_zk.constraints).toString();`
	`247`	`+ if (constraints != parsed_zk_constraints)`
`245`	`248`	`{`
`246`	`249`	`throw Exception(`
`247`	`250`	`"Existing table metadata in ZooKeeper differs in constraints."`
`248`	`251`	`" Stored in ZooKeeper: " + from_zk.constraints +`
	`252`	`+ ", parsed from ZooKeeper: " + parsed_zk_constraints +`
`249`	`253`	`", local: " + constraints,`
`250`	`254`	`ErrorCodes::METADATA_MISMATCH);`
`251`	`255`	`}`
Original file line number	Diff line number	Diff line change
`@@ -60,7 +60,7 @@ struct ReplicatedMergeTreeTableMetadata`
`60`	`60`	`}`
`61`	`61`	`};`
`62`	`62`
`63`		`- void checkEquals(const ReplicatedMergeTreeTableMetadata & from_zk) const;`
	`63`	`+ void checkEquals(const ReplicatedMergeTreeTableMetadata & from_zk, const ColumnsDescription & columns, const Context & context) const;`
`64`	`64`
`65`	`65`	`Diff checkAndFindDiff(const ReplicatedMergeTreeTableMetadata & from_zk) const;`
`66`	`66`