Fix debug assertion in quantileDeterministic function.#16410
Fix debug assertion in quantileDeterministic function.#16410alexey-milovidov merged 5 commits intomasterfrom
Conversation
robot-clickhouse
added
the
pr-improvement
KochetovNicolai
left a comment
KochetovNicolai
left a comment
There was a problem hiding this comment.
So, that's how I understand what was fixed here:
When sample.size() < sample_count < total_values, we serialized sample_count values instead of sample.size() in write method. So, we got some garbage serialized.
Now we always write sample.size(), and it's ok now.
However, I don't understand why it could have caused crash. It may be caught by msan somehow, but I don't see that it was used in crash reports.
Also, what about the case when reader has different max_sample_size then writer? We serialized this value before and changed it for reader. Now we don't and it is possible that sample.size() > max_sample_size after read.
It doesn't cause crash in release and even in MSan, UBSan builds - because the "trash" is actually the memory containing recently filtered out values. That's why it even works correctly when this trash is sent over network and taken into account. But it triggers debug assertion in PODArray::operator[], because we access elements past the end of array (but not past the capacity). That brings a nice idea - we can poison memory region for asan and ubsan after PODArray shrink to lower size. |
I assume that there is no such case. The code is written in a way that it allows different sizes. But:
|
|
Fast test fails because of changes in quantile. Need to update reference. |
…ouse into fix-quantile-deterministic
|
Fuzzer: #16081 |
- known issue, @tavplubix |
3 participants
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Fix debug assertion in quantileDeterministic function. In previous version it may also transfer up to two times more data over the network. Although no bug existed. This fixes #15683