Expose Peak memory usage in query statistics. #51946
Conversation
nikitamikhaylov
added
the
can be tested
|
This is an automated comment for commit d4d381d with description of existing statuses. It's updated for the latest CI running
|
|
Please see here the error I think it is related to the PR. |
| 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); | ||
| } |
There was a problem hiding this comment.
Please explain those lines to me. I do not understand it.
There was a problem hiding this comment.
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).
Columns:
static const NamesAndTypesList column_names_and_types = {
{"host_name", std::make_shared<DataTypeString>()},
{"current_time", std::make_shared<DataTypeDateTime>()},
{"thread_id", std::make_shared<DataTypeUInt64>()},
{"type", TypeEnum},
{"name", std::make_shared<DataTypeString>()},
{"value", std::make_shared<DataTypeInt64>()},
};
There was a problem hiding this comment.
That is Ok.
The diff itself looks strange.
Looks like one write is not enough and you added two :)
There was a problem hiding this comment.
Oh, my bad.
That is right. You add one more row here.
There was a problem hiding this comment.
I don't understand, what exactly is confusing? The first block write current_memory_usage. Insert() appends new value at the end of column (column's size is increased by 1). i++ in block ends is not necessary, i replaced to i;
| @@ -99,8 +99,8 @@ void ProgressIndication::writeFinalProgress() | |||
| if (elapsed_ns) | |||
| std::cout << " (" << formatReadableQuantity(progress.read_rows * 1000000000.0 / elapsed_ns) << " rows/s., " | |||
| << formatReadableSizeWithDecimalSuffix(progress.read_bytes * 1000000000.0 / elapsed_ns) << "/s.)"; | |||
| else | |||
| std::cout << ". "; | |||
| auto peak_memory_usage = getMemoryUsage().peak; | |||
There was a problem hiding this comment.
I have one concern here how a new client works with an old server. Client would print 0 as peak_memory_usage
There was a problem hiding this comment.
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.
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.
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).
src/Server/HTTPHandler.cpp
Outdated
| 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.
Please clarify to me
- when do you want to collect
peak_memory_usagefor the query - when do you want to send it to the client
- which clients should receive
peak_memory_usageand which are not suppose to see it.
The questions arose because I see the inconsistency in the logic, however I might be wrong.
I see that
- 'peak_memory_usage' collected regularly for the query (this is why the data race occurs)
- it is sent only at the end (only at finishSendHeaders)
- only http protocol is affected
There was a problem hiding this comment.
when do you want to collect peak_memory_usage for the query
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.
when do you want to send it to the client
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.
which clients should receive peak_memory_usage and which are not suppose to see it.
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.
(this is why the data race occurs)
It looks like I forgot the mutex, now I'll try to fix it.
What does it look like (over curl):
curl -v 'http://localhost:8123/?send_progress_in_http_headers=1&query=SELECT%20number%20FROM%20numbers(100000000)%20FORMAT%20Null;'
...
< Keep-Alive: timeout=10
< X-ClickHouse-Progress: {"read_rows":"3989949","read_bytes":"31919592","written_rows":"0","written_bytes":"0","total_rows_to_read":"20000000","result_rows":"0","result_bytes":"0"}
< X-ClickHouse-Progress: {"read_rows":"8241534","read_bytes":"65932272","written_rows":"0","written_bytes":"0","total_rows_to_read":"20000000","result_rows":"0","result_bytes":"0"}
< X-ClickHouse-Progress: {"read_rows":"12493119","read_bytes":"99944952","written_rows":"0","written_bytes":"0","total_rows_to_read":"20000000","result_rows":"0","result_bytes":"0"}
< X-ClickHouse-Progress: {"read_rows":"16744704","read_bytes":"133957632","written_rows":"0","written_bytes":"0","total_rows_to_read":"20000000","result_rows":"0","result_bytes":"0"}
< X-ClickHouse-Summary: {"read_rows":"20015154","read_bytes":"160121232","written_rows":"0","written_bytes":"0","total_rows_to_read":"20000000","result_rows":"20000000","result_bytes":"160432128","peak_memory_usage":"0"}
....
in log
023.07.14 14:19:56.455771 [ 58613 ] {f97e1eb1-7d9b-44b7-930c-2e9efcef50ca} <Debug> MemoryTracker: Peak memory usage (for query): 0.00 B.
There was a problem hiding this comment.
only http protocol is affected
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.
Yes, I see now, sendProfileEvents works for all other protocols. Good.
There was a problem hiding this comment.
Now I understand why changes in http look so outstanding here. It because CH does't send ProfileEvents over http.
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 writeHeaderProgress() call as well. It make sense actually. Because one short peak might not be displayed in memory usage. But it may lead to the OOM.
I think that could make the code more straight.
There was a problem hiding this comment.
What do you think about sending peak_memory_usage with each writeHeaderProgress() call?
In general I like this PR, and could approve it as it.
But I think it would be even better if just send that info with each progress header, not only the last one.
There was a problem hiding this comment.
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.
Did (send memory in progress)
|
As it labeled "New Feature". The automatics checks that there are changes in docs files. |
|
I will remove it from clickhouse-client, because it isn't beautiful, and |
|
It's also non-obvious how it is related to distributed queries. Is it the maximum per server peak? |
I don’t know for sure, but rather it’s from only one host (it is the same value as in the log). |
|
@alexey-milovidov as proposed by @den-crane in #44805 there is the second, shorter variant where is added as Mem: at the end: What do you think about this variant? Also there is another variant: implement new configuration option (working name) void ProgressIndication::writeFinalProgress()
{
/// skip
if (show_memory_usage && peak_memory_usage >= 0)
std::cout << "\nPeak memory usage (for query) " << formatReadableSizeWithBinarySuffix(peak_memory_usage) << ".";
}Another variant: the whole text output in <clickhouse_client_final_progress_format>Processed {rows}, {bytes} ({rows_speed}. {bytes_speed}), Mem: {peak_memory}</clickhouse_client_final_progress_format>Default value: if What do you think? |
|
I think we will get it! But we need to figure out these details:
The (for query) looks redundant in clickhouse-client, because everything else is also "for query".
We should also disambiguate it on the peak memory usage indication with the same logic as is used for the progress bar. |
This is too complex (too many options), let's not do it. |
|
@alexey-milovidov OK. Thank you for explanation. Unfortunately Dmitry Kardymon (@kardymonds) does not work anymore in our company. So Alexey Gerasimchuck (@Demilivor) will finish the required changes the next week. In new PR it would be nice to mention Dmitry Kardymon as original author of the PR. |
@alexey-milovidov Does it mean I should also add peak memory usage to the progress bar?
|
| append_callback([&used_output](const Progress & progress) { used_output.out->onProgress(progress); }); | ||
| append_callback([&used_output](const Progress & progress) | ||
| { | ||
| const auto& thread_group = CurrentThread::getGroup(); |
There was a problem hiding this comment.
Wrong style: missing whitespace.
| @@ -22,6 +22,9 @@ struct ThreadEventData | |||
| UInt64 user_ms = 0; | |||
| UInt64 system_ms = 0; | |||
| UInt64 memory_usage = 0; | |||
|
|
|||
| // -1 used as flag 'is not show for old servers' | |||
There was a problem hiding this comment.
is not show -> is not shown
| @@ -89,6 +89,8 @@ class WriteBufferFromHTTPServerResponse final : public BufferWithOwnMemory<Write | |||
| /// but not finish them with \r\n, allowing to send more headers subsequently. | |||
| void startSendHeaders(); | |||
|
|
|||
| // Used for write the header X-ClickHouse-Progress / X-ClickHouse-Summary | |||
There was a problem hiding this comment.
Used for write -> Used for writing
| accumulated_progress.writeJSON(progress_string_writer); | ||
|
|
||
| writeCString("{", progress_string_writer); | ||
| accumulated_progress.writeJSON(progress_string_writer, false); |
| @@ -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.
Default values are harmful.
Review #51946 and partially revert it
Close #44805
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Added Peak Memory Usage (for query) to client final statistics, and to http header.