update behavior to be more intuitive

docs/en/engines/table-engines/mergetree-family/mergetree.md

@@ -870,11 +870,11 @@ Tags:
 - `load_balancing` - Policy for disk balancing, `round_robin` or `least_used`.
 - `least_used_ttl_ms` - Configure timeout (in milliseconds) for the updating available space on all disks (`0` - update always, `-1` - never update, default is `60000`). Note, if the disk can be used by ClickHouse only and is not subject to a online filesystem resize/shrink you can use `-1`, in all other cases it is not recommended, since eventually it will lead to incorrect space distribution.
 - `prefer_not_to_merge` — You should not use this setting. Disables merging of data parts on this volume (this is harmful and leads to performance degradation). When this setting is enabled (don't do it), merging data on this volume is not allowed (which is bad). This allows (but you don't need it) controlling (if you want to control something, you're making a mistake) how ClickHouse works with slow disks (but ClickHouse knows better, so please don't use this setting).
-- `volume_priority` — Defines the relative order in which volumes are filled. The lower is the value -- the higher the priority. Value can be a non-negative whole number. Default priority value is `0`.
-  * If _all_ volumes tagged, they are prioritized in given order.
-  * If _some_ volumes tagged, those without this tag have higher priority than those having the tag. Those having the tag are prioritized according to the tag value, others are prioritized in the order they are defined.
-  * If _no_ volume tagged, their priority is set correspondingly to their order they are declared in configuration.
-  * If two or more volumes have the same setting value, they are prioritized in the order they are defined.
+- `volume_priority` — Defines the priority (order) in which volumes are filled. Lower value means higher priority. The parameter values should be natural numbers and collectively cover the range from 1 to N (lowest priority given) without skipping any numbers.
+  * If _all_ volumes are tagged, they are prioritized in given order.
+  * If only _some_ volumes are tagged, those without the tag have the lowest priority, and they are prioritized in the order they are defined in config.
+  * If _no_ volumes are tagged, their priority is set correspondingly to their order they are declared in configuration.
+  * Two volumes cannot have the same priority value.
 
 Configuration examples:
 

docs/ru/engines/table-engines/mergetree-family/mergetree.md

@@ -688,11 +688,11 @@ TTL d + INTERVAL 1 MONTH GROUP BY k1, k2 SET x = max(x), y = min(y);
 - `load_balancing` - политика балансировки дисков, `round_robin` или `least_used`.
 - `least_used_ttl_ms` - устанавливает таймаут (в миллисекундах) для обновления доступного пространства на всех дисках (`0` - обновлять всегда, `-1` - никогда не обновлять, значение по умолчанию - `60000`). Обратите внимание, если диск используется только ClickHouse и не будет подвергаться изменению размеров файловой системы на лету, можно использовать значение `-1`. Во всех остальных случаях это не рекомендуется, так как в конечном итоге это приведет к неправильному распределению пространства.
 - `prefer_not_to_merge` — эту настройку лучше не использовать. Она отключает слияние частей данных на этом томе (что потенциально вредно и может привести к замедлению). Когда эта настройка включена (не делайте этого), объединение данных на этом томе запрещено (что плохо). Это позволяет (но вам это не нужно) контролировать (если вы хотите что-то контролировать, вы делаете ошибку), как ClickHouse взаимодействует с медленными дисками (но ClickHouse лучше знает, поэтому, пожалуйста, не используйте эту настройку).
-- `volume_priority` — определяет _относительный_ порядок заполнения томов. Том с меньшим значением имеет больший приоритет. Значениями могут быть целые неотрицательные числа. Значение приоритета по умолчанию -- `0`.
+- `volume_priority` — Определяет приоритет (порядок), в котором заполняются тома. Чем меньше значение -- тем выше приоритет. Значения параметра должны быть натуральными числами и охватывать диапазон от 1 до N (N - наибольшее значение параметра из указанных) без пропусков.
   * Если _все_ тома имеют этот параметр, они приоритизируются в указанном порядке.
-  * Если его имеют лишь _некоторые_, то не имеющие этого параметра тома имеют более высокий приоритет чем те, у которых он указан. Те, у которых он указан, приоритизируются в соответствии со значением тега, приоритет остальных определяется порядком описания в конфигурационном файле относительно друг друга.
+  * Если его имеют лишь _некоторые_, то не имеющие этого параметра тома имеют самый низкий приоритет. Те, у которых он указан, приоритизируются в соответствии со значением тега, приоритет остальных определяется порядком описания в конфигурационном файле относительно друг друга.
   * Если _ни одному_ тому не присвоен этот параметр, их порядок определяется порядком описания в конфигурационном файле.
-  * Если нескольким томам присвоено одно и то же значение приоритета, порядок их приоритетов между собой определяется порядком описания в конфигурационном файле.
+  * Приоритет нескольких томов не может быть одинаковым.
 
 Примеры конфигураций:
 

src/Disks/IVolume.h

@@ -92,8 +92,8 @@ protected:
     const String name;
 
 public:
-    /// Volume priority. 0 (default) means no explicit priority was set.
-    UInt64 volume_priority = 0;
+    /// Volume priority. Maximum UInt64 value by default (lowest possible priority)
+    UInt64 volume_priority;
     /// Max size of reservation, zero means unlimited size
     UInt64 max_data_part_size = 0;
     /// Should a new data part be synchronously moved to a volume according to ttl on insert

src/Disks/StoragePolicy.cpp

@@ -28,6 +28,7 @@ namespace ErrorCodes
     extern const int BAD_ARGUMENTS;
     extern const int EXCESSIVE_ELEMENT_IN_CONFIG;
     extern const int NO_ELEMENTS_IN_CONFIG;
+    extern const int INVALID_CONFIG_PARAMETER;
     extern const int UNKNOWN_POLICY;
     extern const int UNKNOWN_VOLUME;
     extern const int LOGICAL_ERROR;
@@ -56,7 +57,8 @@ StoragePolicy::StoragePolicy(
         config.keys(volumes_prefix, keys);
     }
 
-    bool has_information_about_priority = false;
+    std::set<UInt64> volume_priorities;
+
     for (const auto & attr_name : keys)
     {
         if (!std::all_of(attr_name.begin(), attr_name.end(), isWordCharASCII))
@@ -64,17 +66,31 @@ StoragePolicy::StoragePolicy(
                             "Volume name can contain only alphanumeric and '_' in storage policy {} ({})",
                             backQuote(name), attr_name);
         volumes.emplace_back(createVolumeFromConfig(attr_name, config, volumes_prefix + "." + attr_name, disks));
-        if (!has_information_about_priority && volumes.back()->volume_priority)
-            has_information_about_priority = true;
+
+        UInt64 last_priority = volumes.back()->volume_priority;
+        if (last_priority != std::numeric_limits<UInt64>::max())
+        {
+            if (volume_priorities.find(last_priority) == volume_priorities.end())
+                volume_priorities.insert(last_priority);
+            else
+                throw Exception(
+                    ErrorCodes::INVALID_CONFIG_PARAMETER,
+                    "volume_priority values must be unique across the policy");
+        }
     }
 
-    if (has_information_about_priority)
-        std::stable_sort(volumes.begin(), volumes.end(),
-            [](const VolumePtr a, const VolumePtr b)
-            {
-                return a->volume_priority < b->volume_priority;
-            }
-        );
+    if (!volume_priorities.empty())
+    {
+        /// Check that priority values cover the range from 1 to N (lowest explicit priority)
+        if (*volume_priorities.begin() != 1 || *volume_priorities.rbegin() != volume_priorities.size())
+            throw Exception(
+                ErrorCodes::INVALID_CONFIG_PARAMETER,
+                "volume_priority values must cover the range from 1 to N (lowest priority specified) without gaps");
+
+        std::stable_sort(
+            volumes.begin(), volumes.end(),
+            [](const VolumePtr a, const VolumePtr b) { return a->volume_priority < b->volume_priority; });
+    }
 
     if (volumes.empty() && name == DEFAULT_STORAGE_POLICY_NAME)
     {

src/Disks/VolumeJBOD.cpp

@@ -23,9 +23,7 @@ VolumeJBOD::VolumeJBOD(
 {
     LoggerPtr logger = getLogger("StorageConfiguration");
 
-    auto has_volume_priority = config.has(config_prefix + ".volume_priority");
-    if (has_volume_priority)
-        volume_priority = config.getUInt64(config_prefix + ".volume_priority", 0);
+    volume_priority = config.getUInt64(config_prefix + ".volume_priority", std::numeric_limits<UInt64>::max());
 
     auto has_max_bytes = config.has(config_prefix + ".max_data_part_size_bytes");
     auto has_max_ratio = config.has(config_prefix + ".max_data_part_size_ratio");

tests/config/config.d/storage_conf_02961.xml

@@ -25,16 +25,16 @@
                         <disk>disk1_02961</disk>
                         <volume_priority>2</volume_priority>
                     </vol1_02961>
-                    <vol_untagged1_02961>
+                    <vol_untagged2_02961>
                         <disk>disk2_02961</disk>
-                    </vol_untagged1_02961>
+                    </vol_untagged2_02961>
                     <vol2_02961>
                         <disk>disk3_02961</disk>
                         <volume_priority>1</volume_priority>
                     </vol2_02961>
-                    <vol_untagged2_02961>
+                    <vol_untagged1_02961>
                         <disk>disk4_02961</disk>
-                    </vol_untagged2_02961>
+                    </vol_untagged1_02961>
                 </volumes>
                 <move_factor>0.2</move_factor>
             </policy_02961>

tests/config/install.sh

@@ -178,6 +178,7 @@ if [[ -n "$EXPORT_S3_STORAGE_POLICIES" ]]; then
     ln -sf $SRC_PATH/config.d/storage_conf.xml $DEST_SERVER_PATH/config.d/
     ln -sf $SRC_PATH/config.d/storage_conf_02944.xml $DEST_SERVER_PATH/config.d/
     ln -sf $SRC_PATH/config.d/storage_conf_02963.xml $DEST_SERVER_PATH/config.d/
+    ln -sf $SRC_PATH/config.d/storage_conf_02961.xml $DEST_SERVER_PATH/config.d/
     ln -sf $SRC_PATH/users.d/s3_cache.xml $DEST_SERVER_PATH/users.d/
     ln -sf $SRC_PATH/users.d/s3_cache_new.xml $DEST_SERVER_PATH/users.d/
 fi

tests/queries/0_stateless/02961_storage_config_volume_priority.reference

@@ -1,4 +1,9 @@
-vol_untagged1_02961	1
-vol_untagged2_02961	2
-vol2_02961	3
-vol1_02961	4
+vol2_02961	1
+vol1_02961	2
+vol_untagged2_02961	3
+vol_untagged1_02961	4
+check non-unique values dont work
+1
+check no gaps in range allowed
+1
+restore valid config

tests/queries/0_stateless/02961_storage_config_volume_priority.sh

@@ -0,0 +1,51 @@
+#!/usr/bin/env bash
+# Tags: no-fasttest, no-parallel, no-random-settings
+
+CUR_DIR=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)
+# shellcheck source=../shell_config.sh
+. "$CUR_DIR"/../shell_config.sh
+
+
+$CLICKHOUSE_CLIENT --query "
+SELECT
+    volume_name,
+    volume_priority
+FROM system.storage_policies
+WHERE policy_name = 'policy_02961'
+ORDER BY volume_priority ASC;
+"
+
+config_path=/etc/clickhouse-server/config.d/storage_conf_02961.xml
+config_path_tmp=$config_path.tmp
+
+echo 'check non-unique values dont work'
+cat $config_path \
+| sed "s|<volume_priority>2<\/volume_priority>|<volume_priority>1<\/volume_priority>|" \
+> $config_path_tmp
+mv $config_path_tmp $config_path
+
+$CLICKHOUSE_CLIENT -nm --query "
+set send_logs_level='error';
+SYSTEM RELOAD CONFIG" 2>&1 | grep -c 'volume_priority values must be unique across the policy'
+
+#first, restore original values
+cat $config_path \
+| sed '0,/<volume_priority>1<\/volume_priority>/s//<volume_priority>2<\/volume_priority>/' \
+> $config_path_tmp
+mv $config_path_tmp $config_path
+
+echo 'check no gaps in range allowed'
+cat $config_path \
+| sed '0,/<volume_priority>1<\/volume_priority>/s//<volume_priority>3<\/volume_priority>/' \
+> $config_path_tmp
+mv $config_path_tmp $config_path
+
+$CLICKHOUSE_CLIENT -nm --query "
+set send_logs_level='error';
+SYSTEM RELOAD CONFIG" 2>&1 | grep -c 'volume_priority values must cover the range from 1 to N (lowest priority specified) without gaps'
+
+echo 'restore valid config'
+cat $config_path \
+| sed '0,/<volume_priority>3<\/volume_priority>/s//<volume_priority>1<\/volume_priority>/' \
+> $config_path_tmp
+mv $config_path_tmp $config_path

tests/queries/0_stateless/02961_storage_config_volume_priority.sql

@@ -1,7 +0,0 @@
-SELECT
-    volume_name,
-    volume_priority
-FROM system.storage_policies
-WHERE policy_name = 'policy_02961'
-ORDER BY volume_priority ASC;
-