Skip to content
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

Fix formula for new_size in WriteBufferFromVector(AppendModeTag). #6208

Conversation

vitlibar
Copy link
Member

I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en

Category (leave one):

  • Bug Fix

It's another approach to fix #6195

@@ -35,8 +35,6 @@ class WriteBuffer : public BufferBase
*/
inline void next()
{
if (!offset())
return;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems WriteBuffer::nextIfAtEnd() should only check that the cursor reaches the end of the buffer before calling WriteBuffer::nextImpl().

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will it lead to writing zero bytes into file; compressing block of data with zero size, etc?

Copy link
Member Author

@vitlibar vitlibar Aug 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you're right and it's better not to leave a WriteBuffer with empty size after initialization. So I've removed my changes in WriteBuffer.h and just corrected formula in WriteBufferFromVector(AppendModeTag).

@@ -57,7 +57,7 @@ class WriteBufferFromVector : public WriteBuffer
: WriteBuffer(nullptr, 0), vector(vector_)
{
size_t old_size = vector.size();
vector.resize(std::max(vector.size() + initial_size, vector.capacity()));
vector.resize(std::max(initial_size, vector.capacity()));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change in WriteBuffer::next() fixes the hanging. We can still add initial_size to vector.size() here but it seems it's better not to do it in this constructor in advance because WriteBuffer::nextImpl() also can increase the size of the vector.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This commend isn't actual anymore. Here I only corrected the formula for consistency with WriteBufferFromVector::nextImpl().

@vitlibar vitlibar force-pushed the write-buffer-call-next-impl-even-if-zero-offset branch from 870c7d0 to b51898f Compare August 1, 2019 12:22
@vitlibar vitlibar changed the title Call WriteBuffer::nextImpl() even if offset() == 0. Fix formula for new_size in WriteBufferFromVector(AppendModeTag). Aug 1, 2019
@vitlibar vitlibar force-pushed the write-buffer-call-next-impl-even-if-zero-offset branch from b51898f to d7891ec Compare August 1, 2019 12:27
@alexey-milovidov alexey-milovidov merged commit e9c2077 into ClickHouse:master Aug 3, 2019
@KochetovNicolai KochetovNicolai added the pr-bugfix Pull request with bugfix, not backported by default label Sep 19, 2019
KochetovNicolai pushed a commit that referenced this pull request Sep 19, 2019
…en-if-zero-offset

Fix formula for new_size in WriteBufferFromVector(AppendModeTag).

(cherry picked from commit e9c2077)
KochetovNicolai pushed a commit that referenced this pull request Sep 19, 2019
…en-if-zero-offset

Fix formula for new_size in WriteBufferFromVector(AppendModeTag).

(cherry picked from commit e9c2077)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-bugfix Pull request with bugfix, not backported by default
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Query using JSONExtractRaw over a boolean value hangs with 100% CPU usage
3 participants