Skip to content

Fix LOGICAL_ERROR with max_read_buffer_size=0 during reading marks#40705

Merged
kssenii merged 1 commit intoClickHouse:masterfrom
azat:stress/max_read_buffer_size
Aug 28, 2022
Merged

Fix LOGICAL_ERROR with max_read_buffer_size=0 during reading marks#40705
kssenii merged 1 commit intoClickHouse:masterfrom
azat:stress/max_read_buffer_size

Conversation

@azat
Copy link
Copy Markdown
Member

@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
Copy Markdown
Member 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

Development

Successfully merging this pull request may close these issues.

Logical error: 'Too large alignment. Cannot have required_alignment greater than buf_size: 4096 > 0. It is a bug

3 participants