Settings 'max_memory_usage_for_user' and 'max_memory_usage_for_all_queries':...

Settings 'max_memory_usage_for_user' and 'max_memory_usage_for_all_queries': do not interfere between different queries [#CLICKHOUSE-2796].

dbms/include/DB/Common/MemoryTracker.h

@@ -19,7 +19,7 @@ class MemoryTracker
 {
 	std::atomic<Int64> amount {0};
 	std::atomic<Int64> peak {0};
-	Int64 limit {0};
+	std::atomic<Int64> limit {0};
 
 	/// To test exception safety of calling code, memory tracker throws an exception on each memory allocation with specified probability.
 	double fault_probability = 0;
@@ -65,9 +65,14 @@ public:
 
 	void setLimit(Int64 limit_)
 	{
-		limit = limit_;
+		limit.store(limit_, std::memory_order_relaxed);
 	}
 
+	/** Set limit if it was not set.
+	  * Otherwise, set limit to new value, if new value is greater than previous limit.
+	  */
+	void setOrRaiseLimit(Int64 value);
+
 	void setFaultProbability(double value)
 	{
 		fault_probability = value;

dbms/src/Common/MemoryTracker.cpp

@@ -42,6 +42,8 @@ void MemoryTracker::alloc(Int64 size)
 	if (!next)
 		CurrentMetrics::add(metric, size);
 
+	Int64 current_limit = limit.load(std::memory_order_relaxed);
+
 	/// Using non-thread-safe random number generator. Joint distribution in different threads would not be uniform.
 	/// In this case, it doesn't matter.
 	if (unlikely(fault_probability && drand48() < fault_probability))
@@ -54,12 +56,12 @@ void MemoryTracker::alloc(Int64 size)
 			message << " " << description;
 		message << ": fault injected. Would use " << formatReadableSizeWithBinarySuffix(will_be)
 			<< " (attempt to allocate chunk of " << size << " bytes)"
-			<< ", maximum: " << formatReadableSizeWithBinarySuffix(limit);
+			<< ", maximum: " << formatReadableSizeWithBinarySuffix(current_limit);
 
 		throw DB::Exception(message.str(), DB::ErrorCodes::MEMORY_LIMIT_EXCEEDED);
 	}
 
-	if (unlikely(limit && will_be > limit))
+	if (unlikely(current_limit && will_be > current_limit))
 	{
 		free(size);
 
@@ -69,7 +71,7 @@ void MemoryTracker::alloc(Int64 size)
 			message << " " << description;
 		message << " exceeded: would use " << formatReadableSizeWithBinarySuffix(will_be)
 			<< " (attempt to allocate chunk of " << size << " bytes)"
-			<< ", maximum: " << formatReadableSizeWithBinarySuffix(limit);
+			<< ", maximum: " << formatReadableSizeWithBinarySuffix(current_limit);
 
 		throw DB::Exception(message.str(), DB::ErrorCodes::MEMORY_LIMIT_EXCEEDED);
 	}
@@ -98,9 +100,18 @@ void MemoryTracker::reset()
 	if (!next)
 		CurrentMetrics::sub(metric, amount);
 
-	amount = 0;
-	peak = 0;
-	limit = 0;
+	amount.store(0, std::memory_order_relaxed);
+	peak.store(0, std::memory_order_relaxed);
+	limit.store(0, std::memory_order_relaxed);
+}
+
+
+void MemoryTracker::setOrRaiseLimit(Int64 value)
+{
+	/// This is just atomic set to maximum.
+	Int64 old_value = limit.load(std::memory_order_relaxed);
+	while (old_value < value && !limit.compare_exchange_weak(old_value, value))
+		;
 }
 
 

dbms/src/Interpreters/ClusterProxy/Query.cpp

@@ -33,6 +33,13 @@ BlockInputStreams Query::execute()
 	/// Does not matter on remote servers, because queries are sent under different user.
 	new_settings.max_concurrent_queries_for_user = 0;
 	new_settings.limits.max_memory_usage_for_user = 0;
+	/// This setting is really not for user and should not be sent to remote server.
+	new_settings.limits.max_memory_usage_for_all_queries = 0;
+
+	/// Set as unchanged to avoid sending to remote server.
+	new_settings.max_concurrent_queries_for_user.changed = false;
+	new_settings.limits.max_memory_usage_for_user.changed = false;
+	new_settings.limits.max_memory_usage_for_all_queries.changed = false;
 
 	/// Network bandwidth limit, if needed.
 	ThrottlerPtr throttler;

dbms/src/Interpreters/ProcessList.cpp

@@ -83,13 +83,20 @@ ProcessList::EntryPtr ProcessList::insert(
 
 			if (current_memory_tracker)
 			{
-				/// Отслеживаем суммарное потребление оперативки на одновременно выполняющиеся запросы одного пользователя.
-				user_process_list.user_memory_tracker.setLimit(settings.limits.max_memory_usage_for_user);
+				/// Limits are only raised (to be more relaxed) or set to something instead of zero,
+				///  because settings for different queries will interfere each other:
+				///  setting from one query effectively sets values for all other queries.
+
+				/// Track memory usage for all simultaneously running queries from single user.
+				user_process_list.user_memory_tracker.setOrRaiseLimit(settings.limits.max_memory_usage_for_user);
 				user_process_list.user_memory_tracker.setDescription("(for user)");
 				current_memory_tracker->setNext(&user_process_list.user_memory_tracker);
 
-				/// Отслеживаем суммарное потребление оперативки на все одновременно выполняющиеся запросы.
-				total_memory_tracker.setLimit(settings.limits.max_memory_usage_for_all_queries);
+				/// Track memory usage for all simultaneously running queries.
+				/// You should specify this value in configuration for default profile,
+				///  not for specific users, sessions or queries,
+				///  because this setting is effectively global.
+				total_memory_tracker.setOrRaiseLimit(settings.limits.max_memory_usage_for_all_queries);
 				total_memory_tracker.setDescription("(total)");
 				user_process_list.user_memory_tracker.setNext(&total_memory_tracker);
 			}