Fix incorrect attach query in threadpool readers, get rid of static threadpools for reads/writes, make threadpool size for reads/writes configurable by kssenii · Pull Request #41011 · ClickHouse/ClickHouse

kssenii · 2022-09-05T18:29:43Z

Changelog category (leave one):

Improvement

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

Improvements for reading from remote filesystems, made threadpool size for reads/writes configurable. Closes #41070.

kssenii · 2022-09-05T20:45:17Z

the hypothesis is confirmed - here is the same problem with threadpools as in #40821 ☹️ It was not visible before only because of this incorrect second thread status object

 ==95127==ERROR: AddressSanitizer: heap-use-after-free on address 0x61400004751c at pc 0x00000dbb5bad bp 0x7fcdfb1f5970 sp 0x7fcdfb1f5968 READ of size 4 at 0x61400004751c thread T176 (ThreadPool)
#0 0xdbb5bac in MemoryTracker::free(long) build_docker/../src/Common/MemoryTracker.cpp:312:49
#1 0xdbb5a38 in MemoryTracker::free(long) build_docker/../src/Common/MemoryTracker.cpp:353:22
#2 0xdbab827 in DB::ThreadStatus::~ThreadStatus() build_docker/../src/Common/ThreadStatus.cpp:148:20
#3 0xdcf4774 in ThreadFromGlobalPoolImpl::ThreadFromGlobalPoolImpl >::scheduleImpl(std::__1::function, int, std::__1::optional, bool)::'lambda0'()>(void&&)::'lambda'()::operator()() build_docker/../src/Common/ThreadPool.h:194:9
#4 0xdcf4774 in decltype(static_cast(fp)()) std::__1::__invoke::ThreadFromGlobalPoolImpl >::scheduleImpl(std::__1::function, int, std::__1::optional, bool)::'lambda0'()>(void&&)::'lambda'()&>(void&&) build_docker/../contrib/libcxx/include/type_traits:3640:23
#5 0xdce406b in std::__1::__function::__policy_func::operator()() const build_docker/../contrib/libcxx/include/__functional/function.h:843:16
#6 0xdce406b in std::__1::function::operator()() const build_docker/../contrib/libcxx/include/__functional/function.h:1184:12
#7 0xdce406b in ThreadPoolImpl::worker(std::__1::__list_iterator) build_docker/../src/Common/ThreadPool.cpp:294:17

tavplubix · 2022-09-05T22:06:00Z

Let's also add assertions to ctor and dtor of ThreadStatus: chassert(!current_thread) and chassert(current_thread == this)

tavplubix

LGTM, but it would be great to add assertions into ThreadStatus (can be done in separate PR)

src/Common/ThreadStatus.cpp

Co-authored-by: Alexander Tokmakov <[email protected]>

src/Common/ThreadStatus.cpp

…ad-status

src/Common/ThreadStatus.cpp

…ad-status

kssenii · 2022-09-27T09:19:29Z

AST fuzzer (asan)

#41745

2022.09.27 00:00:25.522497 [ 147 ] {} <Trace> BaseDaemon: Received signal -3
2022.09.27 00:00:25.523133 [ 402 ] {} <Fatal> BaseDaemon: ########################################
2022.09.27 00:00:25.523205 [ 402 ] {} <Fatal> BaseDaemon: (version 22.10.1.1, build id: 25DB6D4071A889C5095A909B9C8A817ADA945C3D) (from thread 183) (query_id: 9afecae0-dce0-4383-b96a-d1a770f9e59d::202209_5_9_1) (query: ) Received signal Unknown signal (-3)
2022.09.27 00:00:25.523308 [ 402 ] {} <Fatal> BaseDaemon: Sanitizer trap.
2022.09.27 00:00:25.523502 [ 402 ] {} <Fatal> BaseDaemon: Stack trace: 0xde118a7 0xe3c675a 0xdd0b306 0xdcee1e7 0xdcf0ffc 0xdcf1ad8 0x30770370 0x30767cff 0x306156e1 0x3061550e 0x304b2092 0x30206881 0x31a4b274 0x31d0c977 0x3183bc2a 0x3183b959 0x318473d7 0x31fc352b 0x31870568 0x31873b96 0xdf4ff20 0xdf59c6d 0xdf49e20 0xdf5390d 0x7f1ea9143609 0x7f1ea9068133
2022.09.27 00:00:25.565247 [ 402 ] {} <Fatal> BaseDaemon: 0.1. inlined from ./build_docker/../src/Common/StackTrace.cpp:332: StackTrace::tryCapture()
2022.09.27 00:00:25.565318 [ 402 ] {} <Fatal> BaseDaemon: 0. ./build_docker/../src/Common/StackTrace.cpp:293: StackTrace::StackTrace() @ 0xde118a7 in /workspace/clickhouse
2022.09.27 00:00:25.650307 [ 402 ] {} <Fatal> BaseDaemon: 1. ./build_docker/../src/Daemon/BaseDaemon.cpp:415: sanitizerDeathCallback() @ 0xe3c675a in /workspace/clickhouse
2022.09.27 00:00:29.821548 [ 402 ] {} <Fatal> BaseDaemon: 2. __sanitizer::Die() @ 0xdd0b306 in /workspace/clickhouse
2022.09.27 00:00:33.784548 [ 402 ] {} <Fatal> BaseDaemon: 3. ? @ 0xdcee1e7 in /workspace/clickhouse
2022.09.27 00:00:37.739243 [ 402 ] {} <Fatal> BaseDaemon: 4. __asan::ReportGenericError(unsigned long, unsigned long, unsigned long, unsigned long, bool, unsigned long, unsigned int, bool) @ 0xdcf0ffc in /workspace/clickhouse
2022.09.27 00:00:41.640965 [ 402 ] {} <Fatal> BaseDaemon: 5. __asan_report_load1 @ 0xdcf1ad8 in /workspace/clickhouse
2022.09.27 00:00:42.405360 [ 402 ] {} <Fatal> BaseDaemon: 6. ./build_docker/../src/Columns/ColumnVector.h:407: void DB::TargetSpecific::AVX512VBMI::vectorIndexImpl<DB::PODArray<char8_t, 4096ul, Allocator<false, false>, 15ul, 16ul>, char8_t>(DB::PODArray<char8_t, 4096ul, Allocator<false, false>, 15ul, 16ul> const&, DB::PODArray<char8_t, 4096ul, Allocator<false, false>, 15ul, 16ul> const&, unsigned long, DB::PODArray<char8_t, 4096ul, Allocator<false, false>, 15ul, 16ul>&) @ 0x30770370 in /workspace/clickhouse
2022.09.27 00:00:42.931621 [ 402 ] {} <Fatal> BaseDaemon: 7. ./build_docker/../src/Columns/ColumnVector.h:530: COW<DB::IColumn>::immutable_ptr<DB::IColumn> DB::ColumnVector<char8_t>::indexImpl<char8_t>(DB::PODArray<char8_t, 4096ul, Allocator<false, false>, 15ul, 16ul> const&, unsigned long) const @ 0x30767cff in /workspace/clickhouse
2022.09.27 00:00:43.682371 [ 402 ] {} <Fatal> BaseDaemon: 8. ./build_docker/../src/Columns/ColumnsCommon.h:0: COW<DB::IColumn>::immutable_ptr<DB::IColumn> DB::selectIndexImpl<DB::ColumnVector<char8_t> >(DB::ColumnVector<char8_t> const&, DB::IColumn const&, unsigned long) @ 0x306156e1 in /workspace/clickhouse
2022.09.27 00:00:44.231611 [ 402 ] {} <Fatal> BaseDaemon: 9. ./build_docker/../src/Columns/ColumnVector.cpp:577: DB::ColumnVector<char8_t>::index(DB::IColumn const&, unsigned long) const @ 0x3061550e in /workspace/clickhouse
2022.09.27 00:00:44.328537 [ 402 ] {} <Fatal> BaseDaemon: 10.1. inlined from ./build_docker/../contrib/boost/boost/smart_ptr/intrusive_ptr.hpp:194: boost::intrusive_ptr<DB::IColumn const>::operator*() const
2022.09.27 00:00:44.328571 [ 402 ] {} <Fatal> BaseDaemon: 10. ./build_docker/../src/Columns/ColumnLowCardinality.cpp:201: DB::ColumnLowCardinality::insertRangeFrom(DB::IColumn const&, unsigned long, unsigned long) @ 0x304b2092 in /workspace/clickhouse

kssenii · 2022-09-27T09:21:57Z

Stress test (debug) — Cannot start clickhouse-server

clickhouse-server: ../src/Disks/IO/AsynchronousReadIndirectBufferFromRemoteFS.cpp:228: virtual bool DB::AsynchronousReadIndirectBufferFromRemoteFS::nextImpl(): Assertion `file_offset_of_buffer_end <= impl->getFileSize()' failed.

this error is related to #41783

kssenii · 2022-09-27T09:24:30Z

Stress test (ubsan) — Cannot start clickhouse-server
Stress test (asan) — Cannot start clickhouse-server

Not sure but looks the reason is:

2022.09.27 02:32:34.830712 [ 338626 ] {} <Error> Application: Caught exception while loading metadata: Code: 80. DB::Exception: Unknown Index type 'annoy'. Available index types: hypothesis, bloom_filter, tokenbf_v1, ngrambf_v1, set, minmax: Cannot attach table `test_1`.`02354_annoy` from metadata file /var/lib/clickhouse/metadata/test_1/02354_annoy.sql from query ATTACH TABLE test_1.`02354_annoy` (`id` Int32, `embedding` Array(Float32), INDEX annoy_index embedding TYPE annoy(100) GRANULARITY 1) ENGINE = MergeTree ORDER BY id SETTINGS index_granularity = 5. (INCORRECT_QUERY), Stack trace (when copying this message, always include the lines below):

tavplubix · 2022-09-27T09:31:04Z

Not sure but looks the reason is

#41729

lesandie · 2022-09-30T08:00:01Z

Hi @kssenii

about this make threadpool size for reads/writes configurable are there any new clickhouse setting related?

I'm testing master with s3 table function ... related about this #40128

kssenii · 2022-09-30T09:26:33Z

Yes, settings are in server config threadpool_remote(local)_fs_reader_pool_size and threadpool_remote(local)_fs_reader_queue_size, but these thread pools are used only for s3 as merge tree engine storage. For s3 table function you should use user setting max_download_threads.

Fix

8f064a1

robot-ch-test-poll2 added the pr-not-for-changelog This PR should not be mentioned in the changelog label Sep 5, 2022

tavplubix self-assigned this Sep 7, 2022

tavplubix mentioned this pull request Sep 7, 2022

Maybe fix use-after-free inside S3 upload thread #35305

Merged

Get rid of static threadpools

e51313b

kssenii force-pushed the fix-thread-status branch from 894c98d to e51313b Compare September 7, 2022 15:48

kssenii changed the title ~~Fix incorrect attach query in threadpool readers~~ Fix incorrect attach query in threadpool readers, get rid of static threadpools for reads/writes, make threadpool size for reads/writes configurable Sep 7, 2022

Fix style check and typos check

e06ed09

kssenii mentioned this pull request Sep 7, 2022

Allow to load marks in threadpool in advance #40821

Merged

kssenii added 2 commits September 7, 2022 20:17

Fix thread status destruction order for clickhouse-local

66d65df

Fix

3dc19b4

tavplubix approved these changes Sep 8, 2022

View reviewed changes

Add assertion

6a88543

tavplubix reviewed Sep 9, 2022

View reviewed changes

src/Common/ThreadStatus.cpp Outdated Show resolved Hide resolved

Update src/Common/ThreadStatus.cpp

86f74fc

Co-authored-by: Alexander Tokmakov <[email protected]>

tavplubix reviewed Sep 9, 2022

View reviewed changes

src/Common/ThreadStatus.cpp Outdated Show resolved Hide resolved

tavplubix and others added 4 commits September 9, 2022 21:07

Update src/Common/ThreadStatus.cpp

b987e18

Merge remote-tracking branch 'upstream/master' into fix-thread-status

52ef375

Merge remote-tracking branch 'origin/fix-thread-status' into fix-thre…

c4c044c

…ad-status

Remove assertion

f0ffc78

kssenii commented Sep 21, 2022

View reviewed changes

src/Common/ThreadStatus.cpp Outdated Show resolved Hide resolved

kssenii and others added 6 commits September 23, 2022 15:21

Merge remote-tracking branch 'upstream/master' into fix-thread-status

ab702e4

Fix threadpool reader (for local fs)

3072672

Merge remote-tracking branch 'upstream/master' into fix-thread-status

0a801da

Merge branch 'master' into fix-thread-status

50f7ce6

Update S3ObjectStorage.cpp

c53f463

Fix integration tests

f128110

kssenii requested a review from tavplubix September 26, 2022 10:50

kssenii added 2 commits September 26, 2022 12:51

Merge remote-tracking branch 'origin/fix-thread-status' into fix-thre…

e4d6afa

…ad-status

Fix integration tests

9217766

tavplubix approved these changes Sep 27, 2022

View reviewed changes

kssenii merged commit caf6a99 into ClickHouse:master Sep 27, 2022

lesandie mentioned this pull request Oct 11, 2022

Using INSERT INTO FUNCTION s3 fails with a timeout #40128

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix incorrect attach query in threadpool readers, get rid of static threadpools for reads/writes, make threadpool size for reads/writes configurable#41011

Fix incorrect attach query in threadpool readers, get rid of static threadpools for reads/writes, make threadpool size for reads/writes configurable#41011
kssenii merged 19 commits intoClickHouse:masterfrom
kssenii:fix-thread-status

kssenii commented Sep 5, 2022 •

edited

Loading

Uh oh!

kssenii commented Sep 5, 2022 •

edited

Loading

Uh oh!

tavplubix commented Sep 5, 2022

Uh oh!

tavplubix left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kssenii commented Sep 27, 2022

Uh oh!

kssenii commented Sep 27, 2022

Uh oh!

kssenii commented Sep 27, 2022

Uh oh!

tavplubix commented Sep 27, 2022

Uh oh!

lesandie commented Sep 30, 2022

Uh oh!

kssenii commented Sep 30, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

kssenii commented Sep 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changelog category (leave one):

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

Uh oh!

kssenii commented Sep 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tavplubix commented Sep 5, 2022

Uh oh!

tavplubix left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kssenii commented Sep 27, 2022

Uh oh!

kssenii commented Sep 27, 2022

Uh oh!

kssenii commented Sep 27, 2022

Uh oh!

tavplubix commented Sep 27, 2022

Uh oh!

lesandie commented Sep 30, 2022

Uh oh!

kssenii commented Sep 30, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

kssenii commented Sep 5, 2022 •

edited

Loading

kssenii commented Sep 5, 2022 •

edited

Loading