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 LOGICAL_ERROR with max_read_buffer_size=0 during reading marks #40705

Merged
merged 1 commit into from
Aug 28, 2022

Conversation

azat
Copy link
Collaborator

@azat azat commented Aug 27, 2022

Changelog category (leave one):

  • Bug Fix (user-visible misbehavior in official stable or prestable release)

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

Fix LOGICAL_ERROR with max_read_buffer_size=0 during reading marks

The problem is that the buffer size validated after marks reading in
MergeTreeReaderStream::init(), since it requires to read marks first.

And later it is passed to AsynchronousReadBufferFromFileDescriptor,
which throws LOGICAL_ERROR because buffer_size < alignment.

Fix this my simply disallow such values for max_read_buffer_size (I
thougt about modifying createReadBufferFromFileBase(), but it is not
used for remote reads -- remote_fs_buffer_size).

Fixes: #40669 (cc @davenger )

The problem is that the buffer size validated after marks reading in
MergeTreeReaderStream::init(), since it requires to read marks first.

And later it is passed to AsynchronousReadBufferFromFileDescriptor,
which throws LOGICAL_ERROR because buffer_size < alignment.

Fix this my simply disallow such values for max_read_buffer_size (I
thougt about modifying createReadBufferFromFileBase(), but it is not
used for remote reads -- remote_fs_buffer_size).

Fixes: ClickHouse#40669
Signed-off-by: Azat Khuzhin <[email protected]>
@robot-ch-test-poll robot-ch-test-poll added the pr-bugfix Pull request with bugfix, not backported by default label Aug 27, 2022
@azat
Copy link
Collaborator Author

azat commented Aug 27, 2022

Stateless tests flaky check (address) — fail: 16, passed: 81

  • 02052_last_granula_adjust_LOGICAL_ERROR - old test fails because of concurrency I guess, but it does not fails before, so I don't think that it worth adding no-parallel now

Stress test (undefined) — Backward compatibility check: OOM killer (or signal 9) in clickhouse-server.log

Real OOM

@kssenii kssenii self-assigned this Aug 28, 2022
@kssenii kssenii merged commit a0bc5b6 into ClickHouse:master Aug 28, 2022
@azat azat deleted the stress/max_read_buffer_size branch August 28, 2022 13:03
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
3 participants