New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Expose Peak memory usage in query statistics. #51946
Changes from 13 commits
a2c9e26
a41ec12
b276f45
ffa4f37
7e99295
6f4d6fd
bdfaffb
54838e9
901089b
3fcfbb1
09ba975
1a7ee40
7704469
feebad3
2bc7bb0
1d4d829
e24883d
68a501a
62f9a95
810137e
d4d381d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -32,7 +32,7 @@ struct ProgressValues | |
|
||
void read(ReadBuffer & in, UInt64 server_revision); | ||
void write(WriteBuffer & out, UInt64 client_revision) const; | ||
void writeJSON(WriteBuffer & out) const; | ||
void writeJSON(WriteBuffer & out, bool add_braces = true) const; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Default values are harmful. |
||
}; | ||
|
||
struct ReadProgress | ||
|
@@ -118,7 +118,7 @@ struct Progress | |
void write(WriteBuffer & out, UInt64 client_revision) const; | ||
|
||
/// Progress in JSON format (single line, without whitespaces) is used in HTTP headers. | ||
void writeJSON(WriteBuffer & out) const; | ||
void writeJSON(WriteBuffer & out, bool add_braces = true) const; | ||
|
||
/// Each value separately is changed atomically (but not whole object). | ||
bool incrementPiecewiseAtomically(const Progress & rhs); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -86,9 +86,16 @@ static void dumpMemoryTracker(ProfileEventsSnapshot const & snapshot, DB::Mutabl | |
columns[i++]->insert(static_cast<UInt64>(snapshot.current_time)); | ||
columns[i++]->insert(static_cast<UInt64>(snapshot.thread_id)); | ||
columns[i++]->insert(Type::GAUGE); | ||
|
||
columns[i++]->insertData(MemoryTracker::USAGE_EVENT_NAME, strlen(MemoryTracker::USAGE_EVENT_NAME)); | ||
columns[i++]->insert(snapshot.memory_usage); | ||
columns[i]->insert(snapshot.memory_usage); | ||
|
||
i = 0; | ||
columns[i++]->insertData(host_name.data(), host_name.size()); | ||
columns[i++]->insert(static_cast<UInt64>(snapshot.current_time)); | ||
columns[i++]->insert(static_cast<UInt64>(snapshot.thread_id)); | ||
columns[i++]->insert(Type::GAUGE); | ||
columns[i++]->insertData(MemoryTracker::PEAK_USAGE_EVENT_NAME, strlen(MemoryTracker::PEAK_USAGE_EVENT_NAME)); | ||
columns[i]->insert(snapshot.peak_memory_usage); | ||
} | ||
Comment on lines
+90
to
99
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please explain those lines to me. I do not understand it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The data (ProfileEvents and memory events) between the server and the client passes through row in the sql block (DB::Block). I added one more row with my data (peak memory).
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, my bad. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand, what exactly is confusing? The first block write current_memory_usage. Insert() |
||
|
||
void getProfileEvents( | ||
|
@@ -121,6 +128,7 @@ void getProfileEvents( | |
group_snapshot.thread_id = 0; | ||
group_snapshot.current_time = time(nullptr); | ||
group_snapshot.memory_usage = thread_group->memory_tracker.get(); | ||
group_snapshot.peak_memory_usage = thread_group->memory_tracker.getPeak(); | ||
auto group_counters = thread_group->performance_counters.getPartiallyAtomicSnapshot(); | ||
auto prev_group_snapshot = last_sent_snapshots.find(0); | ||
group_snapshot.counters = | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,7 +3,7 @@ | |
#include <IO/HTTPCommon.h> | ||
#include <IO/Progress.h> | ||
#include <IO/WriteBufferFromString.h> | ||
|
||
#include <IO/WriteHelpers.h> | ||
|
||
namespace DB | ||
{ | ||
|
@@ -35,7 +35,12 @@ void WriteBufferFromHTTPServerResponse::writeHeaderSummary() | |
return; | ||
|
||
WriteBufferFromOwnString progress_string_writer; | ||
accumulated_progress.writeJSON(progress_string_writer); | ||
|
||
writeCString("{", progress_string_writer); | ||
accumulated_progress.writeJSON(progress_string_writer, false); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This looks like trash. |
||
writeCString(",\"peak_memory_usage\":\"", progress_string_writer); | ||
writeText(peak_memory_usage, progress_string_writer); | ||
writeCString("\"}", progress_string_writer); | ||
|
||
if (response_header_ostr) | ||
*response_header_ostr << "X-ClickHouse-Summary: " << progress_string_writer.str() << "\r\n" << std::flush; | ||
|
@@ -169,6 +174,11 @@ void WriteBufferFromHTTPServerResponse::onProgress(const Progress & progress) | |
} | ||
} | ||
|
||
void WriteBufferFromHTTPServerResponse::onMemoryUsage(Int64 usage) | ||
{ | ||
peak_memory_usage = usage; | ||
} | ||
|
||
WriteBufferFromHTTPServerResponse::~WriteBufferFromHTTPServerResponse() | ||
{ | ||
finalize(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -818,7 +818,13 @@ void HTTPHandler::processQuery( | |
|
||
/// While still no data has been sent, we will report about query execution progress by sending HTTP headers. | ||
/// Note that we add it unconditionally so the progress is available for `X-ClickHouse-Summary` | ||
append_callback([&used_output](const Progress & progress) { used_output.out->onProgress(progress); }); | ||
append_callback([&used_output](const Progress & progress) | ||
{ | ||
used_output.out->onProgress(progress); | ||
auto thread_group = CurrentThread::getGroup(); | ||
auto peak_memory_usage = thread_group->memory_tracker.getPeak(); | ||
used_output.out->onMemoryUsage(peak_memory_usage); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please clarify to me
The questions arose because I see the inconsistency in the logic, however I might be wrong.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It is already collected (it was implemented) for the query, I just pass it to http. I don't know exactly how it works. The server already displays this value in its log.
I actually need to update once by the end, but there is no such moment here and I send it along with the progress statistics.
I did it for everyone (because I thought it was for information, and no one would be offended). But I don't have exact information on how clients use the X-ClickHouse-Summary field.
It looks like I forgot the mutex, now I'll try to fix it. What does it look like (over curl):
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The task was also about clickhouse-client, and it has a binary protocol. In general, this task consists of two independent tasks: http + clickhouse-client, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I see now, sendProfileEvents works for all other protocols. Good. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now I understand why changes in http look so outstanding here. It because CH does't send Interesting question. It is addressed not to you actually. What is the point to send progress as header? If CH is unable to buffer all response then no more headers are sent after data transmission started. Maybe CH really is able to buffer all response in the most cases. Here I propose to send peak_memory_usage with each There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you think about sending peak_memory_usage with each In general I like this PR, and could approve it as it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm already doing it now (I thought about doing it during development, but then I refused). Today I hope to fix it (writeHeaderProgress + do not display for old servers). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did (send memory in progress) |
||
}); | ||
|
||
if (settings.readonly > 0 && settings.cancel_http_readonly_queries_on_client_close) | ||
{ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have one concern here how a new client works with an old server. Client would print
0
aspeak_memory_usage
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to me that this entire line is for information, and it is unlikely that the client will rely on this value.
Try not to display the value for old servers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would right and heavy. May be just enough not to log it when it zero. Just because memory usage never is zero.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added check (not show if old server). I use '-1' (as default) because memory can be zero (I don't know why it works like this; on small queries always 0).