Fix stack overflow in recursive Buffer tables. by alexey-milovidov · Pull Request #40643 · ClickHouse/ClickHouse

alexey-milovidov · 2022-08-25T20:41:28Z

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 stack overflow in recursive Buffer tables. This closes #40637

alexey-milovidov · 2022-08-25T20:43:38Z

src/Storages/StorageBuffer.cpp

-    if (underlying)
-        underlying_rows = underlying->totalRows(settings);
-    if (!underlying_rows)
-        return underlying_rows;


Here was another bug, CC @azat.

Indeed, nice catch! (Thread can be resolved).

azat · 2022-08-25T20:50:58Z

src/Storages/StorageBuffer.cpp

 {
    std::optional<UInt64> underlying_rows;
-    auto underlying = DatabaseCatalog::instance().tryGetTable(destination_id, getContext());
+    auto destination = DatabaseCatalog::instance().tryGetTable(destination_id, getContext());


Maybe add a helper to get destination, like in #40640?

azat · 2022-08-26T07:20:32Z

tests/queries/0_stateless/02391_recursive_buffer.sql

+DROP TABLE IF EXISTS test;
+CREATE TABLE test (key UInt32) Engine = Buffer(currentDatabase(), test, 16, 10, 100, 10000, 1000000, 10000000, 100000000);
+SELECT * FROM test; -- { serverError 269 }
+SELECT * FROM system.tables WHERE table = 'test' AND database = currentDatabase() FORMAT Null; -- { serverError 269 }


Note, that concurrent run this test with select * from system.tables queries (in some other tests) may produce INFINITE_LOOP errors there, but maybe this test should be run in parallel, and instead tests that do select * from system.tables should not.

azat · 2022-08-26T07:59:25Z

Stateless tests (ubsan) — Tests are not finished, fail: 1, passed: 63, skipped: 2

Simple queries takes too much time, I'm pretty sure that there are some hung tasks reported by the kernel, maybe dmesg should be captured for all tests (just in case).

Stress test (debug) — Backward compatibility check: Error message in clickhouse-server.log (see bc_check_error_messages.txt)

Will take a look at OOM

Stress test (thread) — Sanitizer assert (in stderr.log)

==================
WARNING: ThreadSanitizer: data race (pid=666)
  Read of size 8 at 0x7b44023c74e0 by thread T2702 (mutexes: write M0):
    #0 <null> <null> (clickhouse+0x19f9e75b) (BuildId: 9ba41c32de4e62863b82c2387fcabcd793540e90)

Bad report, #40655 should help

There is a data-race between reading in system.filesystem_cache, and changing the segments for FileSegment::range().
But the fix will conflict with #40586, so I will wait then (cc @kssenii).

  Read of size 8 at 0x7b44023c74e0 by thread T2702 (mutexes: write M0):
azat@s1:~/ch/tmp/40643$ llvm-symbolizer --exe ./clickhouse $(awk -F '[(+)]' '{print $3}' tsan-read.log) | head -n15
DB::FileSegment::Range::size() const
./build_docker/../src/Common/FileSegment.h:98:38
DB::FileSegment::getSnapshot(std::__1::shared_ptr<DB::FileSegment> const&, std::__1::lock_guard<std::__1::mutex>&)
./build_docker/../src/Common/FileSegment.cpp:667:31

DB::FileCache::getSnapshot() const
./build_docker/../src/Common/FileCache.cpp:1067:37

DB::StorageSystemFilesystemCache::fillData(std::__1::vector<COW<DB::IColumn>::mutable_ptr<DB::IColumn>, std::__1::allocator<COW<DB::IColumn>::mutable_ptr<DB::IColumn> > >&, std::__1::shared_ptr<DB::Context const>, DB::SelectQueryInfo const&) const
./build_docker/../src/Storages/System/StorageSystemFilesystemCache.cpp:43:37

DB::IStorageSystemOneBlock<DB::StorageSystemFilesystemCache>::read(std::__1::vector<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::allocator<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > > > const&, std::__1::shared_ptr<DB::StorageSnapshot> const&, DB::SelectQueryInfo&, std::__1::shared_ptr<DB::Context const>, DB::QueryProcessingStage::Enum, unsigned long, unsigned int)
./build_docker/../src/Storages/System/IStorageSystemOneBlock.h:54:9

DB::IStorage::read(DB::QueryPlan&, std::__1::vector<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::allocator<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > > > const&, std::__1::shared_ptr<DB::StorageSnapshot> const&, DB::SelectQueryInfo&, std::__1::shared_ptr<DB::Context const>, DB::QueryProcessingStage::Enum, unsigned long, unsigned int)

  Previous write of size 8 at 0x7b44023c74e0 by thread T1807:

azat@s1:~/ch/tmp/40643$ llvm-symbolizer --exe ./clickhouse $(awk -F '[(+)]' '{print $3}' tsan-write.log) | head -n15
DB::FileSegmentRangeWriter::completeFileSegment(DB::FileSegment&)
./build_docker/../src/Common/FileSegment.cpp:827:36

DB::FileSegmentRangeWriter::finalize()
./build_docker/../src/Common/FileSegment.cpp:917:5

DB::CachedOnDiskWriteBufferFromFile::finalizeImpl()
./build_docker/../src/Disks/IO/CachedOnDiskWriteBufferFromFile.cpp:175:23

DB::WriteBuffer::finalize()
./build_docker/../src/IO/WriteBuffer.h:125:13

DB::MergedBlockOutputStream::Finalizer::Impl::finish()
./build_docker/../src/Storages/MergeTree/MergedBlockOutputStream.cpp:102:15
DB::MergedBlockOutputStream::Finalizer::finish()

kssenii · 2022-08-26T10:52:20Z

There is a data-race between reading in system.filesystem_cache, and changing the segments for FileSegment::range().
But the fix will conflict with #40586, so I will wait then (cc @kssenii).

yeah, I fixed it in a different pr #40608.

azat · 2022-08-27T12:13:29Z

Stress test (undefined) — Killed by signal (in clickhouse-server.log)

Fix LOGICAL_ERROR with max_read_buffer_size=0 during reading marks #40705

alexey-milovidov added 2 commits August 25, 2022 21:26

Fix stack overflow on recursive Buffer tables and system.tables

ebe7d2d

Add a test

c2deed2

alexey-milovidov mentioned this pull request Aug 25, 2022

More robust protection from recursive Buffer #40640

Closed

robot-ch-test-poll2 added the pr-bugfix Pull request with bugfix, not backported by default label Aug 25, 2022

alexey-milovidov commented Aug 25, 2022

View reviewed changes

azat reviewed Aug 25, 2022

View reviewed changes

alexey-milovidov added 3 commits August 26, 2022 02:53

Fix test

829f9f6

Suggestions from @azat

5b5a163

Fix error

f63fa3b

azat reviewed Aug 26, 2022

View reviewed changes

azat approved these changes Aug 26, 2022

View reviewed changes

azat mentioned this pull request Aug 26, 2022

Fix stress test after #40420 #40608

Merged

alexey-milovidov assigned azat Aug 26, 2022

alexey-milovidov added 2 commits August 26, 2022 16:08

Update 02391_recursive_buffer.sql

03ed472

Merge branch 'master' into fix-recursive-buffer

6940bda

alexey-milovidov merged commit 7569cbb into master Aug 27, 2022

alexey-milovidov deleted the fix-recursive-buffer branch August 27, 2022 16:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix stack overflow in recursive Buffer tables.#40643

Fix stack overflow in recursive Buffer tables.#40643
alexey-milovidov merged 7 commits intomasterfrom
fix-recursive-buffer

alexey-milovidov commented Aug 25, 2022

Uh oh!

alexey-milovidov Aug 25, 2022

Uh oh!

azat Aug 25, 2022 •

edited

Loading

Uh oh!

azat Aug 25, 2022

Uh oh!

azat Aug 26, 2022

Uh oh!

azat commented Aug 26, 2022

Uh oh!

kssenii commented Aug 26, 2022

Uh oh!

azat commented Aug 27, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

alexey-milovidov commented Aug 25, 2022

Changelog category (leave one):

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

Uh oh!

alexey-milovidov Aug 25, 2022

Choose a reason for hiding this comment

Uh oh!

azat Aug 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

azat Aug 25, 2022

Choose a reason for hiding this comment

Uh oh!

azat Aug 26, 2022

Choose a reason for hiding this comment

Uh oh!

azat commented Aug 26, 2022

Uh oh!

kssenii commented Aug 26, 2022

Uh oh!

azat commented Aug 27, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

azat Aug 25, 2022 •

edited

Loading